diff --git a/docs/ENGINE_REFACTORING.md b/docs/ENGINE_REFACTORING.md index f57a5e3..37dae73 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 f23026f..83ae8e2 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,89 @@ 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 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 +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/examples/expectations/window_functions.json b/examples/expectations/window_functions.json index 5adfd37..bae886a 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 8deeec3..fad69a9 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/src/sql/window_context.rs b/src/sql/window_context.rs index 6c888bb..061e366 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 b64dc37..1aa5c27 100644 --- a/tests/comparison/corpus/09_window.toml +++ b/tests/comparison/corpus/09_window.toml @@ -199,19 +199,51 @@ 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. + +[[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 --- diff --git a/tests/test_window_context.rs b/tests/test_window_context.rs index e88af9d..8db6e5c 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"); +}