-
Notifications
You must be signed in to change notification settings - Fork 64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't use zero cost estimate for cached subtrees #1783
Conversation
This continues #1736, which changed the size estimate for cached subtrees from the exact size to the estimate size. Analogous to that, now also change the cost estimate for cached subtrees from zero to the normal cost estimate. Introduce a runtime parameter `zero-cost-for-cached-subtree` to revert to the old behavior if needed (the default is `false`).
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1783 +/- ##
=======================================
Coverage 90.03% 90.04%
=======================================
Files 395 395
Lines 37904 37909 +5
Branches 4263 4264 +1
=======================================
+ Hits 34128 34134 +6
+ Misses 2479 2478 -1
Partials 1297 1297 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a minor change that's still missing, otherwise good to go
test/OperationTest.cpp
Outdated
// change (see the `getCostEstimate` function for details on why). | ||
{ | ||
RuntimeParameters().set<"zero-cost-for-cached-subtree">(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This parameter should be reset to its original value. This makes a difference for the coverage build, where a single binary containing all the unit tests is built and so this flag will be set for all unit tests that follow until it is changed back. This can lead to unwanted side effects.
There's a function setRuntimeParameterForTest
in RuntimeParametersTestHelpers.h
that does just this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏼
Conformance check passed ✅No test result changes. |
|
This continues #1736, which changed the size estimate for cached subtrees from the exact size to the estimate size. Analogous to that, now also change the cost estimate for cached subtrees from zero to the normal cost estimate. That way, the cache no longer influences query planning (but can still improve query processing times because cached subtrees do not have to be computed again).
Introduce a runtime parameter
zero-cost-estimate-for-cached-subtree
to revert to the old behavior if desired (the default isfalse
).