Repository navigation
fix: sliding window min() returns wrong value for all-NULL windows - #23874
Merged
neilconway merged 5 commits intoJul 27, 2026
Merged
Conversation
github-actions
Bot
added
sqllogictest
neilconway
changed the title
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.68% 80.67% -0.01%
==========================================
Files 1095 1095
Lines 372458 372490 +32
Branches 372458 372490 +32
==========================================
- Hits 300500 300498 -2
- Misses 54032 54047 +15
- Partials 17926 17945 +19 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
viirya
approved these changes
Jul 25, 2026
viirya
left a comment
viirya
left a comment
Member
There was a problem hiding this comment.
Nice fix, and the refactor is a real improvement over just patching the bug. Three things I like:
- Dropping the cached
self.min/self.maxin favor of readingmoving_min.min()/moving_max.max()directly (returning the typed NULLempty_valuewhen empty) removes the whole class of "cache out of sync with the underlying structure" bugs — which is exactly what caused the original stale-value bug. - The
retract_batchoptimization (len - logical_null_count()instead of per-rowtry_from_array+is_null) is both faster and cleaner, and it correctly relies on the invariant that NULLs are never pushed, so pop count == non-null count. - Making min and max symmetric (both skip NULLs on update, both count NULLs on retract) is a good consistency win.
The tests nail the key cases: the all-NULL window returning NULL, re-picking up a later non-null value, and — importantly — retracting a NULL row popping nothing (non_null = 0), which is the invariant the whole approach rests on. LGTM.
…-empty-window # Conflicts: # datafusion/functions-aggregate/src/min_max.rs
Contributor
Author
|
@viirya Thanks for the review and for the positive feedback! |
neilconway
enabled auto-merge
July 27, 2026 21:03
kosiew
pushed a commit
to kosiew/datafusion
that referenced
this pull request
Aug 12, 2026
…pache#23874) ## Which issue does this PR close? - Closes apache#23872 ## Rationale for this change `SlidingMinAccumulator::update_batch` skipped 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 the `MovingMin` data structure by the `SlidingMinAccumulator` would not contain any values. This resulted in incorrectly returning a stale non-NULL value for a sliding `min()` 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, `SlidingMaxAccumulator` included NULL values but `SlidingMinAccumulator` omitted them, in part because omitting NULLs made the original `retract_batch` implementation more expensive. This PR optimizes `retract_batch`, so we can now use the same scheme for both the min and max sliding accumulators: * Omit NULLs on `update_batch` (this improves on the prior behavior of `max`) * Efficiently account for NULLs in `retract_batch` (this improves on the prior behavior of `min`) * Ensure correct results for all-NULL window frames (this fixes the prior bug in `min`). ## What changes are included in this PR? * Fix bug in `min()` over all-NULL window frames * Optimize `SlidingMinAccumulator::retract_batch` (avoid materializing values just to count NULLs) * Optimize `SlidingMinAccumulator::update_batch` (omit NULLs), also improving symmetry with `min` * Optimize both accumulators to stop caching the current `min` / `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 underlying `MovingMin` / `MovingMax` data structure ## Are these changes tested? Yes, new tests added. ## Are there any user-facing changes? No, aside from the bug fix.
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.