-
Notifications
You must be signed in to change notification settings - Fork 61
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 cache-max-size-lazy-result
setting for the root operation, even when it's lazy
#1709
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1709 +/- ##
=======================================
Coverage 89.97% 89.97%
=======================================
Files 395 395
Lines 37689 37703 +14
Branches 4241 4242 +1
=======================================
+ Hits 33911 33924 +13
Misses 2479 2479
- Partials 1299 1300 +1 ☔ View full report in Codecov by Sentry. |
cache_max_...
settings for root operation, also when it's lazy
cache_max_...
settings for root operation, also when it's lazycache_max_size...
settings for the root operation, also when it's lazy
cache_max_size...
settings for the root operation, also when it's lazycache_max_size_...
settings for the root operation, also when it's lazy
cache_max_size_...
settings for the root operation, also when it's lazycache-max-...
settings for the root operation, also when it's lazy
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 looks good, thanks a lot for the quick fix! I tried it with the example query from #1638, and that works fine now. I have a small question about the test coverage but that might be void.
src/engine/Operation.cpp
Outdated
isRoot ? cache.getMaxSizeSingleEntry() | ||
: std::min(RuntimeParameters().get<"lazy-result-max-cache-size">(), | ||
cache.getMaxSizeSingleEntry()); |
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.
In the modified tests, isRoot
is always false
, so I wonder why CodeCov ist not complaining about incomplete coverage here.
So the reason why the test coverage is fine is because there are probably dozens of tests for classes inheriting from |
@hannahbast I added the proper unit tests |
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.
Thanks a lot + this looks good + will merge this now
cache-max-...
settings for the root operation, also when it's lazycache-max-size-lazy-result
setting for the root operation, even when it's lazy
Conformance check passed ✅No test result changes. |
|
Since #1638, QLever has the runtime parameter
lazy-result-max-cache-size
, with a deliberately small default value of 5 MB. However, the final result (of the root operation) should be cached not depending on that size, but depending on the regularcache-max-size
,cache-max-size-single-entry
, andcache-max-num-entries
, even if the last operation is (output-)lazy. This is now indeed so. Fixes #1701 (which provides a good example of a query, where the old behavior is weird and the new behavior makes much more sense).Use the occasion to give the runtime parameter a more consistent name, namely
cache-max-size-lazy-result
.