fix: reject negative INTERVAL offsets in RANGE window frames - #24903
Open
edubraqd wants to merge 1 commit into
Open
fix: reject negative INTERVAL offsets in RANGE window frames#24903edubraqd wants to merge 1 commit into
INTERVAL offsets in RANGE window frames#24903edubraqd wants to merge 1 commit into
Conversation
ROWS and GROUPS frame offsets are parsed as `UInt64`, so a negative offset is
a planning error. RANGE offsets are only typed once the ORDER BY type is known,
and an interval literal such as `INTERVAL '-1 day'` carries its sign inside the
string, so it was accepted. The resulting frame starts after it ends, which the
execution code does not expect:
SELECT count(*) OVER (ORDER BY x RANGE BETWEEN INTERVAL '-1 month' PRECEDING
AND CURRENT ROW) FROM ...
sliding_aggregate.rs:216: attempt to subtract with overflow
... RANGE BETWEEN INTERVAL '-1 day' PRECEDING AND INTERVAL '-1 day' FOLLOWING
window_state.rs:65: attempt to subtract with overflow
Release builds do not panic but return wrong results instead. Check the
coerced RANGE offsets in `coerce_window_frame` and reject negative ones with
the same kind of planning error ROWS / GROUPS already produce.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
github-actions
Bot
added
optimizer
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
1 participant
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?
INTERVALoffsets in RANGE window frames are accepted and panic at execution #24902.Rationale for this change
ROWS and GROUPS frame offsets are parsed as
UInt64, so a negative offset is a planning error. RANGE offsets are only typed once the ORDER BY type is known, and an interval literal such asINTERVAL '-1 day'carries its sign inside the string, so it was accepted. The resulting frame starts after it ends, which the execution code does not expect: it panics with "attempt to subtract with overflow" in debug builds (sliding_aggregate.rs,window_state.rs) and returns wrong results in release builds.What changes are included in this PR?
After the RANGE offsets have been coerced in
coerce_window_frame, compare each finite offset with the zero of its type and reject negative ones with a planning error, matching what ROWS / GROUPS already do at parse time (and what PostgreSQL does).Are these changes tested?
Yes.
window.sltgains three negative-offset cases (PRECEDING,FOLLOWING, both) that now fail at planning, plus a non-negative interval offset that still runs.Are there any user-facing changes?
Queries with a negative interval offset in a RANGE frame now fail at planning instead of panicking or returning wrong results.