fix: sliding window min() returns wrong value for all-NULL windows#23874
Open
neilconway wants to merge 3 commits into
Open
fix: sliding window min() returns wrong value for all-NULL windows#23874neilconway wants to merge 3 commits into
min() returns wrong value for all-NULL windows#23874neilconway wants to merge 3 commits into
Conversation
min() returns wrong value for all-NULL windows
The sliding accumulators cached the current min/max in a ScalarValue field, refreshed after every update_batch and retract_batch. The cache cost up to two extra ScalarValue clones per output row and was the root cause of the stale-value bug fixed in the previous commit: a cached value can go stale, a derived one cannot. Replace the cache with an immutable precomputed typed NULL that fixes the accumulator's data type and serves as the result for frames with no non-null values; evaluate() and state() now read the current extremum directly from the window structure.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23874 +/- ##
==========================================
- Coverage 80.69% 80.69% -0.01%
==========================================
Files 1090 1090
Lines 370357 370389 +32
Branches 370357 370389 +32
==========================================
+ Hits 298859 298873 +14
Misses 53684 53684
- Partials 17814 17832 +18 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
min()returns stale value when all non-NULLvalues leave the frame #23872Rationale for this change
SlidingMinAccumulator::update_batchskipped NULL values. This meant that if all the non-NULL values in a window frame were retracted, the window frame would not be empty but theMovingMindata structure by theSlidingMinAccumulatorwould not contain any values. This resulted in incorrectly returning a stale non-NULL value for a slidingmin()over a window frame consisting of only NULL values.Along the way, optimize the min and max sliding window accumulators to make them both more efficient and more symmetric with one another. In the original coding,
SlidingMaxAccumulatorincluded NULL values butSlidingMinAccumulatoromitted them, in part because omitting NULLs made the originalretract_batchimplementation more expensive. This PR optimizesretract_batch, so we can now use the same scheme for both the min and max sliding accumulators:update_batch(this improves on the prior behavior ofmax)retract_batch(this improves on the prior behavior ofmin)min).What changes are included in this PR?
min()over all-NULL window framesSlidingMinAccumulator::retract_batch(avoid materializing values just to count NULLs)SlidingMinAccumulator::update_batch(omit NULLs), also improving symmetry withminmin/max; this saves a few clones, but perhaps more importantly it is simpler and avoids the risk of inconsistency between the cached value and the underlyingMovingMin/MovingMaxdata structureAre these changes tested?
Yes, new tests added.
Are there any user-facing changes?
No, aside from the bug fix.