Repository navigation
Conversation
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?
LimitPushPastWindowspanics with "attempt to add with overflow" for a largeROWS ... FOLLOWINGbound #26658.Rationale for this change
A
ROWSwindow frame with a hugeFOLLOWINGbound (for example18446744073709551615 FOLLOWING) combined withORDER BY ... LIMITpanics withattempt to add with overflowin debug builds. In release builds the addition wraps, soLimitPushPastWindowscould give theSortExecbelow the window a fetch that is far too small and drop rows the window needs.After fixing the optimizer rule, the same query panicked a second time in
WindowFrameContext::calculate_range_rows(window_state.rs), which computes the frame end asidx + n + 1with the same unchecked bound. So both places need fixing for the query in the issue to return its result.What changes are included in this PR?
limit_pushdown_past_window.rs: usesaturating_addwhen growing the limit by the window lookahead (fetch + lookahead,lookahead + max_rel) and when addingskipto aGlobalLimitExecfetch. A saturated fetch is still correct, it just means the sort keeps all rows. This matches howinput_fetchinsort_pushdown.rsalready handles the same kind of sum.window_state.rs: usesaturating_addfor theFOLLOWING nstart and end of aROWSframe. The result is already clamped to the partition length, so saturating gives the right range.This touches the same lines in
limit_pushdown_past_window.rsas the refactor in #26660, which leaves these sums to #26658. The change here is small, so whichever lands second should be easy to rebase.What is the testing strategy for this PR?
window_limits.slt: the query from the issue (sum(x) OVER (... ROWS BETWEEN CURRENT ROW AND 18446744073709551615 FOLLOWING) ... LIMIT 2), and alead(x, 1)variant that adds its own lookahead on top of the huge frame bound. Without this change the first case panics atlimit_pushdown_past_window.rs:198; with only the optimizer fix it panics atwindow_state.rs:287.window*.sltsqllogictests,cargo test -p datafusion-expr --lib window_state,cargo test -p datafusion-physical-optimizer --lib limit_pushdown_past_window,cargo fmt --allandcargo clippy -p datafusion-expr -p datafusion-physical-optimizer --all-targets --all-features -- -D warningspass.Are there any user-facing changes?
No API changes. Queries with very large
ROWS ... FOLLOWINGbounds now return results instead of panicking (or, in release builds, possibly returning too few rows).This PR was prepared with AI assistance. I reviewed the change and ran the tests listed above.