Skip to content

fix: clear stale sliding aggregate state for empty RANGE frames - #24185

Open
lyne7-sc wants to merge 1 commit into
apache:mainfrom
lyne7-sc:fix-sliding-empty-frame
Open

fix: clear stale sliding aggregate state for empty RANGE frames#24185
lyne7-sc wants to merge 1 commit into
apache:mainfrom
lyne7-sc:fix-sliding-empty-frame

Conversation

@lyne7-sc

@lyne7-sc lyne7-sc commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

Sliding aggregate window functions can return incorrect results when a bounded RANGE frame transitions from non-empty to empty and then back to non-empty.

The empty frame does not clear the previous accumulator state, so stale values are included in subsequent results.

What changes are included in this PR?

  • Retract the previous frame from the accumulator when the current frame is empty.
  • Add a slt covering

Are these changes tested?

Yes.

Are there any user-facing changes?

Bug fix only.

@github-actions github-actions Bot added physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt) labels Aug 8, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 71.42857% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.05%. Comparing base (634f0b3) to head (26e1d35).
⚠️ Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
...sion/physical-expr/src/window/sliding_aggregate.rs 71.42% 1 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #24185      +/-   ##
==========================================
- Coverage   81.06%   81.05%   -0.01%     
==========================================
  Files        1107     1106       -1     
  Lines      382191   382266      +75     
  Branches   382191   382266      +75     
==========================================
+ Hits       309805   309863      +58     
- Misses      54083    54089       +6     
- Partials    18303    18314      +11     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@viirya viirya left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified the fix end-to-end locally — LGTM.

Reproduced the bug on current main with the issue's query: pre-fix output is 40 110 2 10 100 (the k=10 frame's v=100 leaks into k=40's result via SUM/COUNT/MAX), post-fix it's the correct 40 10 1 10 10. So the diagnosis in #24184 is spot on: the empty k=30 frame (k ∈ [20,25], no rows) returned default_value without touching the accumulator, leaving it holding the previous frame's rows while last_range advanced to the empty range — so the next non-empty frame's incremental retract_bound = cur.start - last.start never removed the stale rows.

The fix restores the invariant that the accumulator holds exactly last_range: on an empty frame it retracts the whole prior [last_range.start, last_range.end), using the same filter-mask slicing as the update path. Correct and minimal.

A couple of things I checked that might be useful:

  • The plain (non-sliding) sibling doesn't need the same change. PlainAggregateWindowExpr::get_aggregate_result_inside_range has the identical cur_range.start == cur_range.end early-return, but it's only ever used for is_ever_expanding() frames (start pinned at UNBOUNDED PRECEDING). Such a frame can only be empty as a leading prefix and stays non-empty once it grows, so the non-empty→empty→non-empty sequence that triggers this bug isn't reachable there. Good that the fix is scoped to sliding only.
  • No regressions in the sliding-window suite. (The only failures I saw in window*.slt were the pre-existing window_topk_pushdown.slt ones that also fail on main without this patch — they're a missing-test-data/plan-shape thing, unrelated to this change.)

Nice catch and clean fix.

@lyne7-sc

lyne7-sc commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

@viirya thanks for your review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sliding RANGE aggregates retain stale state after an empty frame

3 participants