fix(window): give RANGE frames peer-group semantics (P24) - #56
Merged
Conversation
A RANGE frame was evaluated as ROWS. `get_frame_rows` carried the comment "RANGE frame - based on ORDER BY values (not yet fully implemented) / For now, treat like ROWS" above a verbatim copy of the ROWS arm. RANGE is defined over *peer groups* - all rows tying on every ORDER BY key - where ROWS counts physical rows. On distinct keys the two coincide, so this only shows up against ties, which is why it survived until null_edges.csv existed. With `SUM(score) OVER (ORDER BY score)` over scores 10,20,30,50,50,... the first 50 saw 110 instead of 160: its own peer excluded from its own frame. The damaging half is the default frame. `SUM(x) OVER (ORDER BY y)` with no explicit frame means RANGE UNBOUNDED PRECEDING .. CURRENT ROW, and is the common way to write a running total - silently wrong wherever y has duplicates. Both halves were one defect. The parser already synthesised the correct default frame, so the entire divergence lived in the evaluator and one fix closed both corpus cases. - OrderedPartition gains `peer_bounds`, computed in a single linear pass over the already-sorted rows, so all-equal keys cost no more than all-distinct. - CURRENT ROW resolves to the first row of the peer group as a start bound and the last as an end bound. That asymmetry is the whole mechanism, so win_range_frame_peer_start covers the start side the original cases missed. - Sorting and peer detection must agree exactly or frames land mid-group; both now route through one `compare_by_sort_cols`, and peers are the rows it calls Equal. With no ORDER BY that makes the partition a single peer group, which is what the standard specifies, with no special case. - RANGE with a numeric offset is value-based, not positional, and is now rejected at WindowContext construction instead of silently returning the ROWS answer. Filed as P33; ROWS offsets are untouched. Parity 129 -> 133 AGREE (+2 fixed, +2 new coverage), GAP 13 -> 14 for P33. Explicit ROWS frames verified unchanged. Four unit tests cover peer groups from both bound sides, the ROWS contrast, and the offset rejection. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…P24 bug The Examples Test Suite was the only failing CI job. Its formal test window_functions compares against examples/expectations/window_functions.json, captured back when RANGE was evaluated as ROWS, so it had frozen the defect: query 9's LAST_VALUE(sales_amount) OVER (PARTITION BY region ORDER BY month) recorded each row's OWN amount. With no explicit frame the default is RANGE UNBOUNDED PRECEDING AND CURRENT ROW, so the frame ends at the last PEER - every row tying on month - and both salespeople in a region/month report the same value. Checked against DuckDB before touching anything: it returns the new output on all 24 rows, so the capture was wrong and the fix is right. Re-captured on that basis. The diff is confined to last_sale_in_frame (12 values); no other field moved, so the re-capture bakes in nothing else. The suite's other failures are pre-existing smoke-test noise (missing fixtures, unreachable URLs) and do not fail the job - only FORMAL mismatches do, which is why this job passed on main. - Re-capture examples/expectations/window_functions.json. - Correct the comment above query 9, which documented the old row-at-a-time reading and would have sent the next reader the wrong way. - Add corpus case win_last_value_default_frame: SUM reads the whole frame, LAST_VALUE only its final row, so it pins the end bound where the SUM cases cannot. Parity 133 -> 134 AGREE. - Record the pattern in SQL_PARITY.md: when a fix breaks a captured test, establish which side is right against the reference engine before touching either. Re-capturing reflexively would have silently re-frozen the bug. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
A RANGE frame was evaluated as ROWS.
get_frame_rowscarried the comment "RANGE frame - based on ORDER BY values (not yet fully implemented) / For now, treat like ROWS" above a verbatim copy of the ROWS arm.RANGE is defined over peer groups - all rows tying on every ORDER BY key - where ROWS counts physical rows. On distinct keys the two coincide, so this only shows up against ties, which is why it survived until null_edges.csv existed. With
SUM(score) OVER (ORDER BY score)over scores 10,20,30,50,50,... the first 50 saw 110 instead of 160: its own peer excluded from its own frame.The damaging half is the default frame.
SUM(x) OVER (ORDER BY y)with no explicit frame means RANGE UNBOUNDED PRECEDING .. CURRENT ROW, and is the common way to write a running total - silently wrong wherever y has duplicates.Both halves were one defect. The parser already synthesised the correct default frame, so the entire divergence lived in the evaluator and one fix closed both corpus cases.
peer_bounds, computed in a single linear pass over the already-sorted rows, so all-equal keys cost no more than all-distinct.compare_by_sort_cols, and peers are the rows it calls Equal. With no ORDER BY that makes the partition a single peer group, which is what the standard specifies, with no special case.Parity 129 -> 133 AGREE (+2 fixed, +2 new coverage), GAP 13 -> 14 for P33. Explicit ROWS frames verified unchanged. Four unit tests cover peer groups from both bound sides, the ROWS contrast, and the offset rejection.