From 9439fcc51f5baca548a4a99f7497c37240fc0a64 Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Sun, 30 Aug 2026 12:08:29 +0100 Subject: [PATCH 1/2] fix(window): give RANGE frames peer-group semantics (P24) 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 --- docs/ENGINE_REFACTORING.md | 1 + docs/SQL_PARITY.md | 62 ++++++- src/sql/window_context.rs | 241 ++++++++++++++++++------- tests/comparison/corpus/09_window.toml | 39 +++- tests/test_window_context.rs | 138 ++++++++++++++ 5 files changed, 405 insertions(+), 76 deletions(-) diff --git a/docs/ENGINE_REFACTORING.md b/docs/ENGINE_REFACTORING.md index f57a5e34..37dae735 100644 --- a/docs/ENGINE_REFACTORING.md +++ b/docs/ENGINE_REFACTORING.md @@ -438,3 +438,4 @@ AGREE count — which makes it safe to land well before the semantics change. | 2026-08-22 | R10 filed; slice 1a landed: `data::trilean` with the 3VL truth tables and 13 unit tests. Unwired — no behaviour change, parity unmoved at 125/159 | #51 | | 2026-08-22 | R10 slice 1b: WHERE evaluator converted to `Result`; `is_true()` collapse at the single row-filter boundary. `Unknown` still never constructed, so no behaviour change — parity unmoved at 125/159 | #53 | | 2026-08-22 | R10 slice 1c: UNKNOWN produced at the leaves via `compare_trilean`. Closes parity P18/P19 — 125 → **129 AGREE**; new finding P32 (`NOT LIKE` parse gap) pinned, not fixed | — | +| 2026-08-30 | `RANGE` window frames given peer-group semantics (`OrderedPartition::peer_bounds`); sorting and peer detection unified on one comparator. Closes parity P24 — 129 → **133 AGREE**; new finding P33 (`RANGE` with a numeric offset) now a deliberate hard error rather than a silent ROWS answer | — | diff --git a/docs/SQL_PARITY.md b/docs/SQL_PARITY.md index f23026f8..dcc87f7b 100644 --- a/docs/SQL_PARITY.md +++ b/docs/SQL_PARITY.md @@ -81,7 +81,7 @@ Suggested fix order, by silent blast radius: | ~~5~~ | ~~[P29](#p29) boolean operator after `IN (...)`~~ | ✅ **Fixed 2026-08-08** — same bug as P30, one change closed both | | ~~4~~ | ~~[P28](#p28) `INTO #tmp` stages unfiltered rows~~ | ✅ **Fixed 2026-08-16** — turned out to stage the *whole source table*, and the sweep it prescribed found [P31](#p31) | | ~~7~~ | ~~[P18](#p18)/[P19](#p19) three-valued logic~~ | ✅ **Fixed 2026-08-22** — 125 → 129 AGREE. Delivered as three slices ([R10](ENGINE_REFACTORING.md#r10)); the two no-op ones landed first, so the semantics change reviewed on its own | -| 8 | [P24](#p24) `RANGE` treated as `ROWS` | Silent, hits the common `SUM(x) OVER (ORDER BY y)` running-total form | +| ~~8~~ | ~~[P24](#p24) `RANGE` treated as `ROWS`~~ | ✅ **Fixed 2026-08-30** — 129 → 133 AGREE (+2 fixed, +2 new coverage). One defect, not two: the parser already emitted the right default frame, so fixing peer groups closed both cases. Spun off [P33](#p33) | | 9 | [P14](#p14), [P16](#p16), [P17](#p17), [P20](#p20), [P23](#p23), P13 stage 2 | Smaller, self-contained, decisions already taken | | 10 | [P22](#p22), [P25](#p25), [P26](#p26), [P15](#p15), [P32](#p32) | Hard errors — visible, so less urgent than any of the above | | — | [P27](#p27) `OR` in `JOIN ... ON` | **Reclassified 2026-08-08 — not a quick win.** `JoinCondition` is a `Vec` implicitly AND-ed, so there is nowhere in the AST to put an `OR`; it needs join conditions to become an expression, which reaches the join execution code. Sequence it with the R-log, not here | @@ -887,24 +887,72 @@ annotation be removed. through as the out-of-range fallback. ### P24 — A `RANGE` frame is treated as `ROWS` -- **Status:** 🔴 OPEN +- **Status:** 🟢 FIXED 2026-08-30 — 129 → 133 AGREE (+2 fixed, +2 new coverage) - **Corpus:** `09_window.toml :: win_range_frame_with_ties`, - `win_default_frame_ordered` (both DIFFER). Baselines: the three explicit - `ROWS` frame cases (all AGREE). + `win_default_frame_ordered` (both now AGREE; `expect` dropped). New cases: + `win_range_frame_peer_start`, `win_range_frame_partitioned_ties` (AGREE), + `win_range_numeric_offset` (GAP — see [P33](#p33)). Baselines: the three + explicit `ROWS` frame cases, all still AGREE. - **Observed:** with ties in the ORDER BY key, `RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW` must include **all peer rows** at the current - value. At `score = 50` (two peers) DuckDB returns 160; we return 110 — one + value. At `score = 50` (two peers) DuckDB returns 160; we returned 110 — one peer only, i.e. ROWS behaviour. - **The damaging half is the default frame.** With an `ORDER BY` in the window and no explicit frame, the SQL default is `RANGE UNBOUNDED PRECEDING AND CURRENT ROW`. `SUM(x) OVER (ORDER BY y)` is a far more common way to write a - running total than any explicit frame, and it is silently wrong wherever `y` - has duplicates. Explicit `ROWS` frames are unaffected and already correct. + running total than any explicit frame, and it was silently wrong wherever `y` + had duplicates. Explicit `ROWS` frames were unaffected and already correct. - **Only detectable because the fixture has ties** — on distinct keys ROWS and RANGE coincide, which is why this survived until `null_edges.csv` existed. - **Decision:** **Fix.** Implement peer-group semantics for `RANGE`, and make the no-frame-with-ORDER-BY default resolve to `RANGE` rather than `ROWS`. +**How it was fixed.** Both halves turned out to be one defect. The parser +*already* synthesised the correct default frame — `recursive_parser.rs` has +emitted `RANGE UNBOUNDED PRECEDING .. CURRENT ROW` for an ORDER BY without an +explicit frame since before this entry was filed. The whole divergence lived in +`window_context.rs::get_frame_rows`, whose `FrameUnit::Range` arm carried the +comment *"not yet fully implemented — for now, treat like ROWS"* and duplicated +the ROWS arm verbatim. So the second half of the decision above needed no work, +and fixing the first half closed both corpus cases at once. + +`OrderedPartition` now carries `peer_bounds`, computed in one linear pass over +the already-sorted rows, so a partition of all-equal keys costs no more than one +of all-distinct keys. `CURRENT ROW` resolves to the **first** row of the peer +group as a start bound and the **last** as an end bound — the asymmetry is the +entire mechanism, and `win_range_frame_peer_start` exists because the two +original cases only exercised the end bound. + +Sorting and peer detection must agree exactly or frames land mid-group, so both +now route through one `compare_by_sort_cols`; peers are precisely the rows it +calls `Equal`. A pleasant consequence: with no `ORDER BY` every row is a peer of +every other, so a bare `RANGE` frame spans the partition, which is what the +standard specifies, without a special case. + +**A lesson worth generalising: the entry's own prescription was half stale.** +"Make the default resolve to RANGE" described a defect that had already been +fixed elsewhere, and following it literally would have meant editing a parser +that was already right. Re-read the *code* each entry points at before scoping +the work — a finding records what was true when it was filed, and the codebase +moves underneath it. This is the P28 lesson ("a finding inherits its probe's +blind spot") in a different key: there the write-up was too narrow, here it was +out of date. + +### P33 — A `RANGE` frame with a numeric offset is rejected +- **Status:** 🔴 OPEN — hard error, deliberate, low urgency +- **Corpus:** `09_window.toml :: win_range_numeric_offset` (GAP). +- **Observed:** `RANGE BETWEEN 1 PRECEDING AND CURRENT ROW` → "RANGE frames with + a numeric offset are not supported." DuckDB evaluates it. +- **Created by the [P24](#p24) fix, deliberately.** A numeric offset under RANGE + is defined on ORDER BY *values* — "every row whose key is within 1 of mine" — + not on positions, and needs single-key, numeric/temporal arithmetic that peer + groups do not provide. Before P24 this form silently returned the ROWS answer. + Rejecting it converts a silent wrong answer into a visible error, which is a + strict improvement and the same trade the P13 stage-1 work made. +- **Decision:** **Fix eventually.** Self-contained and well-specified; wants the + ORDER BY key restricted to one numeric or temporal column, then a value-window + scan. Ranks below any silent finding, being a hard error. + ### P25 — A window's `ORDER BY` accepts only a plain column - **Status:** 🔴 OPEN - **Corpus:** `09_window.toml :: win_order_by_expression` (GAP). diff --git a/src/sql/window_context.rs b/src/sql/window_context.rs index 6c888bba..061e3669 100644 --- a/src/sql/window_context.rs +++ b/src/sql/window_context.rs @@ -54,11 +54,19 @@ pub struct OrderedPartition { /// Quick lookup: row_index -> position in partition row_positions: HashMap, + + /// Peer-group bounds by position: rows that tie on every ORDER BY key. + /// `peer_bounds[pos] == (first, last)` inclusive positions of the group + /// containing `pos`. RANGE frames are defined over these groups rather than + /// over physical rows, which is the whole difference between RANGE and ROWS. + /// With no ORDER BY every row is a peer of every other, so the single group + /// spans the partition - which is exactly what SQL specifies there. + peer_bounds: Vec<(usize, usize)>, } impl OrderedPartition { - /// Create a new ordered partition from row indices - fn new(rows: Vec) -> Self { + /// Create a new ordered partition from rows already sorted by `sort_cols` + fn new(rows: Vec, table: &DataTable, sort_cols: &[(usize, bool)]) -> Self { // Build position lookup let row_positions: HashMap = rows .iter() @@ -66,10 +74,45 @@ impl OrderedPartition { .map(|(pos, &row_idx)| (row_idx, pos)) .collect(); + let peer_bounds = Self::compute_peer_bounds(&rows, table, sort_cols); + Self { rows, row_positions, + peer_bounds, + } + } + + /// Walk the sorted rows once, grouping runs that compare equal on every + /// ORDER BY key. Linear, so a partition of all-equal keys costs no more + /// than one of all-distinct keys. + fn compute_peer_bounds( + rows: &[usize], + table: &DataTable, + sort_cols: &[(usize, bool)], + ) -> Vec<(usize, usize)> { + let mut bounds = vec![(0usize, 0usize); rows.len()]; + let mut group_start = 0usize; + + for pos in 1..=rows.len() { + let ends_group = pos == rows.len() + || WindowContext::compare_by_sort_cols(table, rows[pos - 1], rows[pos], sort_cols) + != std::cmp::Ordering::Equal; + + if ends_group { + for b in bounds.iter_mut().take(pos).skip(group_start) { + *b = (group_start, pos - 1); + } + group_start = pos; + } } + + bounds + } + + /// Inclusive positions of the peer group containing `pos` + fn peer_bounds_at(&self, pos: usize) -> Option<(usize, usize)> { + self.peer_bounds.get(pos).copied() } /// Navigate to offset from current position @@ -134,6 +177,8 @@ impl WindowContext { /// Create a new window context with a full window specification pub fn new_with_spec(view: Arc, spec: WindowSpec) -> Result { + Self::validate_frame(&spec)?; + let overall_start = Instant::now(); let partition_by = spec.partition_by.clone(); let order_by = spec.order_by.clone(); @@ -241,13 +286,14 @@ impl WindowContext { let mut partitions = BTreeMap::new(); let partition_count = partition_map.len(); + let sort_cols = Self::resolve_sort_columns(source_table, &order_by)?; for (key, mut rows) in partition_map { // Sort rows within partition - if !order_by.is_empty() { - Self::sort_rows(&mut rows, source_table, &order_by)?; + if !sort_cols.is_empty() { + Self::sort_rows(&mut rows, source_table, &sort_cols); } - partitions.insert(key, OrderedPartition::new(rows)); + partitions.insert(key, OrderedPartition::new(rows, source_table, &sort_cols)); } info!( @@ -275,16 +321,45 @@ impl WindowContext { }) } + /// Reject window frames we would otherwise answer wrongly. + /// + /// A numeric offset under RANGE (`RANGE BETWEEN 1 PRECEDING AND CURRENT + /// ROW`) is defined on ORDER BY *values*, not row positions - "every row + /// whose key is within 1 of mine". We only implement the peer-group bounds + /// (UNBOUNDED / CURRENT ROW). Erroring here is deliberate: the alternative + /// is silently computing the ROWS answer, which is the defect this + /// peer-group work exists to remove. + fn validate_frame(spec: &WindowSpec) -> Result<()> { + let Some(frame) = &spec.frame else { + return Ok(()); + }; + if frame.unit != FrameUnit::Range { + return Ok(()); + } + + let bounds = std::iter::once(&frame.start).chain(frame.end.iter()); + for bound in bounds { + if matches!(bound, FrameBound::Preceding(_) | FrameBound::Following(_)) { + return Err(anyhow!( + "RANGE frames with a numeric offset are not supported. Use ROWS for an offset counted in rows, or RANGE with UNBOUNDED PRECEDING / CURRENT ROW / UNBOUNDED FOLLOWING." + )); + } + } + + Ok(()) + } + /// Create a single partition from the entire view fn create_single_partition( view: &DataView, order_by: &[OrderByItem], ) -> Result { let mut rows: Vec = view.get_visible_rows(); + let sort_cols = Self::resolve_sort_columns(view.source(), order_by)?; - if !order_by.is_empty() { + if !sort_cols.is_empty() { let sort_start = Instant::now(); - Self::sort_rows(&mut rows, view.source(), order_by)?; + Self::sort_rows(&mut rows, view.source(), &sort_cols); debug!( "Single partition sort took {:.2}ms ({} rows)", sort_start.elapsed().as_secs_f64() * 1000.0, @@ -292,15 +367,15 @@ impl WindowContext { ); } - Ok(OrderedPartition::new(rows)) + Ok(OrderedPartition::new(rows, view.source(), &sort_cols)) } - /// Sort row indices according to ORDER BY specification - fn sort_rows(rows: &mut Vec, table: &DataTable, order_by: &[OrderByItem]) -> Result<()> { - let prep_start = Instant::now(); - - // Get column indices for ORDER BY columns - let sort_cols: Vec<(usize, bool)> = order_by + /// Resolve ORDER BY items to (column index, ascending) pairs + fn resolve_sort_columns( + table: &DataTable, + order_by: &[OrderByItem], + ) -> Result> { + order_by .iter() .map(|col| { // Extract column name from expression (currently only supports simple columns) @@ -316,57 +391,63 @@ impl WindowContext { let ascending = matches!(col.direction, SortDirection::Asc); Ok((idx, ascending)) }) - .collect::>>()?; - - debug!( - "Sort preparation took {:.2}μs ({} sort columns)", - prep_start.elapsed().as_micros(), - sort_cols.len() - ); - - let sort_start = Instant::now(); + .collect() + } - // Sort rows based on column values - rows.sort_by(|&a, &b| { - for &(col_idx, ascending) in &sort_cols { - let val_a = table.get_value(a, col_idx); - let val_b = table.get_value(b, col_idx); - - match (val_a, val_b) { - (None, None) => continue, - (None, Some(_)) => { - return if ascending { - std::cmp::Ordering::Less - } else { - std::cmp::Ordering::Greater - } + /// Order two rows by the resolved ORDER BY columns. + /// + /// `Ordering::Equal` means the rows are *peers*: RANGE frames are built on + /// this, so sorting and peer detection must agree exactly - hence one + /// function serving both. + fn compare_by_sort_cols( + table: &DataTable, + a: usize, + b: usize, + sort_cols: &[(usize, bool)], + ) -> std::cmp::Ordering { + for &(col_idx, ascending) in sort_cols { + let val_a = table.get_value(a, col_idx); + let val_b = table.get_value(b, col_idx); + + match (val_a, val_b) { + (None, None) => continue, + (None, Some(_)) => { + return if ascending { + std::cmp::Ordering::Less + } else { + std::cmp::Ordering::Greater } - (Some(_), None) => { - return if ascending { - std::cmp::Ordering::Greater - } else { - std::cmp::Ordering::Less - } + } + (Some(_), None) => { + return if ascending { + std::cmp::Ordering::Greater + } else { + std::cmp::Ordering::Less } - (Some(v_a), Some(v_b)) => { - // DataValue only implements PartialOrd, not Ord - let ord = v_a.partial_cmp(&v_b).unwrap_or(std::cmp::Ordering::Equal); - if ord != std::cmp::Ordering::Equal { - return if ascending { ord } else { ord.reverse() }; - } + } + (Some(v_a), Some(v_b)) => { + // DataValue only implements PartialOrd, not Ord + let ord = v_a.partial_cmp(v_b).unwrap_or(std::cmp::Ordering::Equal); + if ord != std::cmp::Ordering::Equal { + return if ascending { ord } else { ord.reverse() }; } } } - std::cmp::Ordering::Equal - }); + } + std::cmp::Ordering::Equal + } + + /// Sort row indices according to ORDER BY specification + fn sort_rows(rows: &mut [usize], table: &DataTable, sort_cols: &[(usize, bool)]) { + let sort_start = Instant::now(); + + rows.sort_by(|&a, &b| Self::compare_by_sort_cols(table, a, b, sort_cols)); debug!( "Actual sort operation took {:.2}μs ({} rows)", sort_start.elapsed().as_micros(), rows.len() ); - - Ok(()) } /// Get value at offset from current row (for LAG/LEAD) @@ -547,15 +628,28 @@ impl WindowContext { (start, end) } FrameUnit::Range => { - // RANGE frame - based on ORDER BY values (not yet fully implemented) - // For now, treat like ROWS - let start = - self.calculate_frame_position(&frame.start, current_pos, partition.rows.len()); + // RANGE frame - bounds land on peer-group edges, not physical + // rows. CURRENT ROW means "the whole tie group at this ORDER BY + // value": as a start bound its first row, as an end bound its + // last. On distinct keys every group is a single row and this + // coincides with ROWS, which is why the difference only shows up + // against data containing ties. + let (peer_first, peer_last) = partition + .peer_bounds_at(current_pos as usize) + .unwrap_or((current_pos as usize, current_pos as usize)); + + let start = self.calculate_range_frame_position( + &frame.start, + peer_first as i64, + partition.rows.len(), + ); let end = match &frame.end { - Some(bound) => { - self.calculate_frame_position(bound, current_pos, partition.rows.len()) - } - None => current_pos, + Some(bound) => self.calculate_range_frame_position( + bound, + peer_last as i64, + partition.rows.len(), + ), + None => peer_last as i64, // Default to CURRENT ROW }; (start, end) } @@ -588,6 +682,31 @@ impl WindowContext { } } + /// Calculate absolute position from a RANGE frame bound. + /// + /// `peer_edge` is the edge of the current row's peer group appropriate to + /// the bound's side - first position for a start bound, last for an end + /// bound - so CURRENT ROW extends to cover the whole tie group. + /// + /// Numeric offsets (`RANGE 1 PRECEDING`) are value-based in SQL rather than + /// positional, and are rejected up front in `new_with_spec`; the arm here is + /// unreachable in practice and falls back to positional rather than + /// inventing an answer. + fn calculate_range_frame_position( + &self, + bound: &FrameBound, + peer_edge: i64, + partition_size: usize, + ) -> i64 { + match bound { + FrameBound::UnboundedPreceding => 0, + FrameBound::UnboundedFollowing => partition_size as i64 - 1, + FrameBound::CurrentRow => peer_edge, + FrameBound::Preceding(n) => peer_edge - n, + FrameBound::Following(n) => peer_edge + n, + } + } + /// Calculate sum of a column within the window frame for the given row pub fn get_frame_sum(&self, row_index: usize, column: &str) -> Option { let frame_rows = self.get_frame_rows(row_index); diff --git a/tests/comparison/corpus/09_window.toml b/tests/comparison/corpus/09_window.toml index b64dc376..536a20b7 100644 --- a/tests/comparison/corpus/09_window.toml +++ b/tests/comparison/corpus/09_window.toml @@ -199,19 +199,42 @@ expect = "DIFFER" id = "win_range_frame_with_ties" data = "null_edges.csv" sql = "SELECT id, score, SUM(score) OVER (ORDER BY score RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW) AS v FROM null_edges WHERE score IS NOT NULL ORDER BY id" -expect = "DIFFER" -# P24. At score 50 (a tie) RANGE must include BOTH peer rows -> 160; we return -# 110, i.e. only one of them, which is ROWS behaviour. The ties in the fixture -# are the entire reason this is detectable. +# P24 fixed 2026-08-30. At score 50 (a tie) RANGE includes BOTH peer rows -> 160. +# The ties in the fixture are the entire reason this is detectable: on distinct +# keys RANGE and ROWS coincide, so keep them. [[case]] id = "win_default_frame_ordered" data = "null_edges.csv" sql = "SELECT id, score, SUM(score) OVER (ORDER BY score) AS v FROM null_edges WHERE score IS NOT NULL ORDER BY id" -expect = "DIFFER" -# P24, and the more damaging half: with an ORDER BY and no explicit frame the -# default is RANGE UNBOUNDED PRECEDING TO CURRENT ROW. Users write this form far -# more often than an explicit RANGE, and it is silently wrong on ties. +# P24 fixed 2026-08-30, and the more damaging half: with an ORDER BY and no +# explicit frame the default is RANGE UNBOUNDED PRECEDING TO CURRENT ROW. Users +# write this form far more often than an explicit RANGE, so it must stay pinned. + +[[case]] +id = "win_range_frame_peer_start" +data = "null_edges.csv" +sql = "SELECT id, score, SUM(score) OVER (ORDER BY score RANGE BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING) AS v FROM null_edges WHERE score IS NOT NULL ORDER BY id" +# P24 from the other side: CURRENT ROW as a *start* bound must open at the FIRST +# row of the peer group, where as an end bound it closes at the last. The +# UNBOUNDED PRECEDING cases above only exercise the end bound. + +[[case]] +id = "win_range_frame_partitioned_ties" +data = "null_edges.csv" +sql = "SELECT id, team, score, SUM(score) OVER (PARTITION BY team ORDER BY score) AS v FROM null_edges WHERE score IS NOT NULL AND team IS NOT NULL ORDER BY id" +# P24 with PARTITION BY: peer groups must not span a partition boundary. Team +# beta holds the tie (70, 70) that makes this bite. + +[[case]] +id = "win_range_numeric_offset" +data = "null_edges.csv" +sql = "SELECT id, score, SUM(score) OVER (ORDER BY score RANGE BETWEEN 1 PRECEDING AND CURRENT ROW) AS v FROM null_edges WHERE score IS NOT NULL ORDER BY id" +expect = "GAP" +# P33. A numeric offset under RANGE is value-based ("keys within 1 of mine"), +# not positional, and we implement only the peer-group bounds. Rejected +# explicitly as of 2026-08-30 rather than silently answering as ROWS, which is +# the defect P24 removed. # --- P25 / P26: window placements we reject --- diff --git a/tests/test_window_context.rs b/tests/test_window_context.rs index e88af9d0..8db6e5c0 100644 --- a/tests/test_window_context.rs +++ b/tests/test_window_context.rs @@ -210,3 +210,141 @@ fn test_window_context_order_by_desc() { Some(DataValue::Integer(40)), // Previous in DESC order is 40 ); } + +// --- P24: RANGE frames follow peer groups, ROWS follows physical rows --- + +use sql_cli::sql::parser::ast::{FrameBound, FrameUnit, WindowFrame, WindowSpec}; + +/// Scores with deliberate ties, mirroring `data/null_edges.csv`: +/// sorted ascending they are 10, 20, 30, 50, 50, 70, 70, 90. +fn tied_scores_view() -> Arc { + let mut table = DataTable::new("scores"); + table.add_column(DataColumn::new("id")); + table.add_column(DataColumn::new("score")); + + for (id, score) in [ + (1, 50), + (2, 50), + (4, 70), + (5, 30), + (6, 70), + (7, 90), + (8, 10), + (9, 20), + ] { + table + .add_row(DataRow::new(vec![ + DataValue::Integer(id), + DataValue::Integer(score), + ])) + .unwrap(); + } + + Arc::new(DataView::new(Arc::new(table))) +} + +fn order_by_score() -> Vec { + vec![OrderByItem { + expr: SqlExpression::Column(ColumnRef { + name: "score".to_string(), + quote_style: QuoteStyle::None, + table_prefix: None, + }), + direction: SortDirection::Asc, + }] +} + +fn spec_with_frame(frame: Option) -> WindowSpec { + WindowSpec { + partition_by: vec![], + order_by: order_by_score(), + frame, + } +} + +/// Running SUM per source row index, in the fixture's row order. +fn running_sums(spec: WindowSpec) -> Vec { + let context = WindowContext::new_with_spec(tied_scores_view(), spec).unwrap(); + (0..8) + .map(|row| match context.get_frame_sum(row, "score") { + Some(DataValue::Integer(n)) => n, + other => panic!("row {row}: expected an integer sum, got {other:?}"), + }) + .collect() +} + +#[test] +fn range_frame_includes_every_peer_at_the_current_value() { + // RANGE UNBOUNDED PRECEDING .. CURRENT ROW: the two 50s both see 160 + // (10+20+30+50+50), and the two 70s both see 300. + let sums = running_sums(spec_with_frame(Some(WindowFrame { + unit: FrameUnit::Range, + start: FrameBound::UnboundedPreceding, + end: Some(FrameBound::CurrentRow), + }))); + + // rows in fixture order: id 1,2,4,5,6,7,8,9 / score 50,50,70,30,70,90,10,20 + assert_eq!(sums, vec![160, 160, 300, 60, 300, 390, 10, 30]); +} + +#[test] +fn rows_frame_still_counts_physical_rows() { + // The same query with ROWS must NOT merge the ties: the first 50 sees only + // itself (110), the second sees both (160). This is the behaviour RANGE was + // wrongly inheriting. + let sums = running_sums(spec_with_frame(Some(WindowFrame { + unit: FrameUnit::Rows, + start: FrameBound::UnboundedPreceding, + end: Some(FrameBound::CurrentRow), + }))); + + assert_eq!(sums, vec![110, 160, 230, 60, 300, 390, 10, 30]); +} + +#[test] +fn current_row_as_a_start_bound_opens_at_the_first_peer() { + // RANGE CURRENT ROW .. UNBOUNDED FOLLOWING: both 50s see 50+50+70+70+90. + let sums = running_sums(spec_with_frame(Some(WindowFrame { + unit: FrameUnit::Range, + start: FrameBound::CurrentRow, + end: Some(FrameBound::UnboundedFollowing), + }))); + + // Descending totals from each peer group's first row (total is 390): + // 10 -> 390, 20 -> 380, 30 -> 360, 50/50 -> 330, 70/70 -> 230, 90 -> 90 + assert_eq!(sums, vec![330, 330, 230, 360, 230, 90, 390, 380]); +} + +#[test] +fn a_range_frame_with_a_numeric_offset_is_rejected_not_guessed() { + // Value-based offsets are unimplemented. Erroring is the point: silently + // answering as ROWS is precisely the P24 defect. + let result = WindowContext::new_with_spec( + tied_scores_view(), + spec_with_frame(Some(WindowFrame { + unit: FrameUnit::Range, + start: FrameBound::Preceding(1), + end: Some(FrameBound::CurrentRow), + })), + ); + let err = match result { + Ok(_) => panic!("RANGE with a numeric offset should be rejected"), + Err(e) => e.to_string(), + }; + + assert!( + err.contains("RANGE frames with a numeric offset"), + "unexpected error: {err}" + ); + + // The same offset under ROWS is positional and must still work. + WindowContext::new_with_spec( + tied_scores_view(), + spec_with_frame(Some(WindowFrame { + unit: FrameUnit::Rows, + start: FrameBound::Preceding(1), + end: Some(FrameBound::CurrentRow), + })), + ) + .expect("ROWS with a numeric offset should remain supported"); +} From 51fb39515b416ba42e09de0d57a811ee246a1143 Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Sun, 30 Aug 2026 13:07:18 +0100 Subject: [PATCH 2/2] fix(examples): re-capture window_functions expectation frozen on the 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 --- docs/SQL_PARITY.md | 17 +++++++++++++++ examples/expectations/window_functions.json | 24 ++++++++++----------- examples/window_functions.sql | 6 +++++- tests/comparison/corpus/09_window.toml | 9 ++++++++ 4 files changed, 43 insertions(+), 13 deletions(-) diff --git a/docs/SQL_PARITY.md b/docs/SQL_PARITY.md index dcc87f7b..83ae8e27 100644 --- a/docs/SQL_PARITY.md +++ b/docs/SQL_PARITY.md @@ -929,6 +929,23 @@ calls `Equal`. A pleasant consequence: with no `ORDER BY` every row is a peer of every other, so a bare `RANGE` frame spans the partition, which is what the standard specifies, without a special case. +**A captured expectation had frozen the bug — the third instance of this +pattern.** `examples/expectations/window_functions.json` stored `LAST_VALUE(x) +OVER (PARTITION BY region ORDER BY month)` returning each row's *own* amount, +which is the ROWS answer; the fix broke that "passing" test. DuckDB agrees with +the new output on all 24 rows, so the capture was wrong, not the fix, and it was +re-captured. The suite's other 22 failures are pre-existing smoke-test noise — +missing fixtures and unreachable URLs — and only FORMAL mismatches fail the job. + +This is the same shape as the P29/P30 note above ("two had *passing* [unit tests] +asserting the broken behaviour"), and worth stating as a rule: **when a fix +breaks a golden/captured test, establish which side is right against the +reference engine before touching either.** Re-capturing is the correct move only +once the reference has confirmed the new output; done reflexively it would have +silently re-frozen the defect. Both `examples/window_functions.sql`'s comment and +a new corpus case (`win_last_value_default_frame`) now record the real semantics, +so the next person meets the rule rather than the artefact. + **A lesson worth generalising: the entry's own prescription was half stale.** "Make the default resolve to RANGE" described a defect that had already been fixed elsewhere, and following it literally would have meant editing a parser diff --git a/examples/expectations/window_functions.json b/examples/expectations/window_functions.json index 5adfd374..bae886af 100644 --- a/examples/expectations/window_functions.json +++ b/examples/expectations/window_functions.json @@ -837,21 +837,21 @@ ], [ { - "last_sale_in_frame": 15000, + "last_sale_in_frame": 12000, "month": "2024-01", "region": "North", "sales_amount": 15000, "salesperson": "Alice" }, { - "last_sale_in_frame": 18000, + "last_sale_in_frame": 14000, "month": "2024-02", "region": "North", "sales_amount": 18000, "salesperson": "Alice" }, { - "last_sale_in_frame": 22000, + "last_sale_in_frame": 16000, "month": "2024-03", "region": "North", "sales_amount": 22000, @@ -879,21 +879,21 @@ "salesperson": "Bob" }, { - "last_sale_in_frame": 20000, + "last_sale_in_frame": 17000, "month": "2024-01", "region": "South", "sales_amount": 20000, "salesperson": "Charlie" }, { - "last_sale_in_frame": 19000, + "last_sale_in_frame": 21000, "month": "2024-02", "region": "South", "sales_amount": 19000, "salesperson": "Charlie" }, { - "last_sale_in_frame": 25000, + "last_sale_in_frame": 23000, "month": "2024-03", "region": "South", "sales_amount": 25000, @@ -921,21 +921,21 @@ "salesperson": "Diana" }, { - "last_sale_in_frame": 13000, + "last_sale_in_frame": 11000, "month": "2024-01", "region": "East", "sales_amount": 13000, "salesperson": "Eve" }, { - "last_sale_in_frame": 15000, + "last_sale_in_frame": 13000, "month": "2024-02", "region": "East", "sales_amount": 15000, "salesperson": "Eve" }, { - "last_sale_in_frame": 19000, + "last_sale_in_frame": 14000, "month": "2024-03", "region": "East", "sales_amount": 19000, @@ -963,21 +963,21 @@ "salesperson": "Frank" }, { - "last_sale_in_frame": 24000, + "last_sale_in_frame": 16000, "month": "2024-01", "region": "West", "sales_amount": 24000, "salesperson": "Grace" }, { - "last_sale_in_frame": 26000, + "last_sale_in_frame": 18000, "month": "2024-02", "region": "West", "sales_amount": 26000, "salesperson": "Grace" }, { - "last_sale_in_frame": 28000, + "last_sale_in_frame": 20000, "month": "2024-03", "region": "West", "sales_amount": 28000, diff --git a/examples/window_functions.sql b/examples/window_functions.sql index 8deeec31..fad69a97 100644 --- a/examples/window_functions.sql +++ b/examples/window_functions.sql @@ -96,7 +96,11 @@ FROM test; GO -- 9. LAST_VALUE - Show last sale in each region --- Note: LAST_VALUE by default only looks at rows up to current row +-- Note: with no explicit frame the default is RANGE UNBOUNDED PRECEDING AND +-- CURRENT ROW, so the frame ends at the last PEER row - every row tying on the +-- ORDER BY key (month), not the current row itself. Both salespeople in a +-- region/month therefore report the same value. Use ROWS for the row-at-a-time +-- reading, or CURRENT ROW AND UNBOUNDED FOLLOWING for the partition's last sale. SELECT region, salesperson, diff --git a/tests/comparison/corpus/09_window.toml b/tests/comparison/corpus/09_window.toml index 536a20b7..1aa5c271 100644 --- a/tests/comparison/corpus/09_window.toml +++ b/tests/comparison/corpus/09_window.toml @@ -236,6 +236,15 @@ expect = "GAP" # explicitly as of 2026-08-30 rather than silently answering as ROWS, which is # the defect P24 removed. +[[case]] +id = "win_last_value_default_frame" +data = "null_edges.csv" +sql = "SELECT id, score, LAST_VALUE(score) OVER (ORDER BY score) AS v FROM null_edges WHERE score IS NOT NULL ORDER BY id" +# P24 via a different frame consumer. SUM reads the whole frame, LAST_VALUE only +# its final row, so it pins the end bound precisely: with ties it must report the +# last PEER, not the current row. An examples/ expectation had captured the old +# ROWS answer here, which is why this is worth a case of its own. + # --- P25 / P26: window placements we reject --- [[case]]