Repository navigation
feat: Framing for FIRST_VALUE, LAST_VALUE. - #19301
Conversation
This patch implements framing support for the FIRST_VALUE, LAST_VALUE window functions and updates all relevant tests. New tests are also added for the processors themselves.
| return; | ||
| } | ||
|
|
||
| final WindowFrame.Groups groupsFrame = frame.unwrap(WindowFrame.Groups.class); |
There was a problem hiding this comment.
[P1] Bounded RANGE frames still execute with GROUPS semantics
The new framed path treats every non-ROWS frame as WindowFrame.Groups, and Windowing maps Calcite's isRows == false windows into that representation. That is not correct for bounded SQL RANGE frames, which must use value-distance on the ORDER BY expression. Queries like FIRST_VALUE(x) OVER (ORDER BY t RANGE BETWEEN 1 PRECEDING AND CURRENT ROW) will still return wrong answers even though this PR now enables framed FIRST_VALUE/LAST_VALUE.
There was a problem hiding this comment.
Processing won't get this far due to validation. There's a test for it in CalciteQueryTest#testUnSupportedRangeBounds, expecting an error message like:
Order By with RANGE clause currently supports only UNBOUNDED or CURRENT ROW.
I added a comment here about that.
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
The changes LGTM, no correctness issues found.
|
This pull request has been marked as stale due to 60 days of inactivity. |
|
This pull request/issue has been closed due to lack of activity. If you think that |
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
Reviewed 79 of 79 changed files. The original bounded RANGE concern is fixed on the SQL path because validation rejects offset-based non-ROWS frames before the GROUPS-based implementation. No additional high-confidence PR-caused issues found.
This is an automated review by Codex GPT-5.6-Luna(max)
3 participants
This patch implements framing support for the FIRST_VALUE, LAST_VALUE window functions and updates all relevant tests. New tests are also added for the processors themselves.