From 3bc02ccfa0eb8273ff4ceabdea0c5ec1494a6045 Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Sat, 5 Sep 2026 10:02:01 +0100 Subject: [PATCH] =?UTF-8?q?fix(parity):=20NULL=20ordering=20in=20ORDER=20B?= =?UTF-8?q?Y=20=E2=80=94=20P17=20+=20P13=20stage=202?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes both halves of the ORDER BY comparator's NULL rule in one slice, since they are the same comparator and both decisions were taken together (2026-08-02). Parity 141 -> 152 AGREE; eleven cases in one change. Default: NULLs now sort LAST in both directions, following the reference engine. Previously NULL sorted as the minimum value, so ASC put them first. The standard leaves this implementation-defined and the major engines disagree, so this is a recorded choice, not a correction — see the CHANGELOG's behaviour-change note. Explicit NULLS FIRST / NULLS LAST is now parsed and honoured, per ORDER BY item. NULLS, FIRST and LAST are matched contextually rather than promoted to keywords: all three are plausible column names, and reserving them would break unrelated queries against arbitrary CSV headers. A malformed clause errors rather than being ignored, per P13 stage 1's rule. One comparator, datavalue_compare::compare_for_order_by, now serves both sort sites — that shared function, not the corpus cases, is what stops them drifting apart again. The window site was worse than "a different rule": it compared through DataValue's derived PartialOrd, where Null is the last variant and so sorted as the MAXIMUM before DESC reversed it into first place. The same derived ordering compared cross-type values by variant rather than value, so Integer never met Float numerically; routing it through the shared comparator fixed that too. compare_datavalues is deliberately untouched — it is shared with aggregates and TUI sorting, where NULL-as-minimum is a different question. Also here: - examples/jsonl_logs.sql: [SKIP] dropped; both forms now return the same top 5. - corpus: renamed order_by_nulls_first_limit_nullfree, which had collided with the null_edges case of the same name so --check reported one id twice. docs/SQL_PARITY.md P13/P17 updated; P37 moves to NEXT. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_015fQ6qjYnQjmUAXgaQMk4Qq --- CHANGELOG.md | 39 ++++++ docs/SQL_PARITY.md | 111 +++++++++++---- examples/jsonl_logs.sql | 13 +- src/data/data_view.rs | 20 +-- src/data/datavalue_compare.rs | 136 +++++++++++++++++++ src/data/query_engine.rs | 6 +- src/query_plan/ilike_to_like_transformer.rs | 1 + src/query_plan/order_by_alias_transformer.rs | 5 +- src/sql/parser/ast.rs | 47 ++++++- src/sql/parser/ast_formatter.rs | 7 + src/sql/parser/formatter.rs | 5 +- src/sql/recursive_parser.rs | 118 +++++++++++++++- src/sql/window_context.rs | 51 +++---- tests/comparison/corpus/08_ordering.toml | 57 ++++---- tests/comparison/corpus/09_window.toml | 16 +-- tests/test_multi_column_order_by.rs | 5 +- tests/test_window_context.rs | 6 +- 17 files changed, 518 insertions(+), 125 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 70e4382..da90465 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,45 @@ All notable changes to SQL CLI will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +### ⚠️ Behaviour change + +#### **`ORDER BY` now places NULLs last by default, in both directions** +Previously NULL sorted as the smallest value, so `ORDER BY score` put NULLs +first and `ORDER BY score DESC` put them last. NULLs now sort **last either +way**, matching DuckDB (and this project's reference engine for parity). + +Standard SQL leaves NULL placement implementation-defined and the major engines +genuinely disagree — SQLite and MySQL treat NULL as smallest, PostgreSQL as +largest, DuckDB pins NULLS LAST — so this is a recorded choice, not a bug fix. +**If you rely on NULLs appearing first in an ascending sort, add an explicit +clause** (see below); the previous behaviour is still available, it is just no +longer the default. + +The same rule now applies to a window's internal `ORDER BY`, which used to +disagree with the top-level one: `FIRST_VALUE(score) OVER (ORDER BY score DESC)` +returned NULL over a partition containing one, and now returns the largest +non-NULL value. + +### ✨ Features + +#### **`NULLS FIRST` / `NULLS LAST` in `ORDER BY`** +```sql +SELECT id, score FROM data ORDER BY score NULLS FIRST; +SELECT id, score FROM data ORDER BY score DESC NULLS FIRST, id; +``` +Per item, so each sort key can differ. The placement is absolute — `NULLS LAST` +means last in the output whichever direction the values sort in. + +Before this, the clause was not parsed at all. Until 2026-08-02 it was silently +discarded *along with every clause after it*, so `ORDER BY amount DESC NULLS +LAST LIMIT 3` quietly returned every row instead of 3; since then it has been a +parse error. Both are now gone. + +`NULLS`, `FIRST` and `LAST` are **not** reserved words — they are recognised +only in that position, so columns named `first`, `last` or `nulls` keep working. + ## [1.69.1] - 2026-04-14 ### Improvements diff --git a/docs/SQL_PARITY.md b/docs/SQL_PARITY.md index 17cb89b..59215cb 100644 --- a/docs/SQL_PARITY.md +++ b/docs/SQL_PARITY.md @@ -65,10 +65,10 @@ session apiece to fix properly, so **discovery is paused and the effort moves to picking them off**. Widen the corpus again when the open list is short, or opportunistically when a fix needs a case that doesn't exist yet. -Corpus coverage today: tiers 01–10, **177 cases** (141 AGREE / 13 DIFFER / -20 GAP / 1 OURS_ONLY / 2 BOTH_ERR as of 2026-09-04 — five `NULLS FIRST`/`LAST` -cases were added that day as the acceptance criteria for [P13](#p13) stage 2, -which is why GAP moved 15 → 20 without anything regressing). **Tier 10 +Corpus coverage today: tiers 01–10, **177 cases** (152 AGREE / 10 DIFFER / +12 GAP / 1 OURS_ONLY / 2 BOTH_ERR as of 2026-09-05, after the NULL-ordering +slice closed [P13](#p13) stage 2 and [P17](#p17) together — eleven cases in one +change, the largest single movement so far). **Tier 10 (aggregate & NULL edges) is deliberately partial** — it holds the P14 and P18–P20 cases and their baselines, but was never built out the way tiers 08 and 09 were. Finish it during a lull; the aggregate-function surface (`STDDEV`, `DISTINCT` aggregates, `FILTER`, @@ -86,8 +86,8 @@ Suggested fix order, by silent blast radius: | ~~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`~~ | ✅ **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) | | ~~9a~~ | ~~[P16](#p16) `ORDER BY ` ignored~~ | ✅ **Fixed 2026-08-31** — 134 → 139 AGREE. The literal was being promoted into a hidden *constant* column, so the sort ran on a column where every row tied | -| **NEXT** | [P17](#p17) + [P13](#p13) stage 2 — NULL ordering | **Queued 2026-09-04 for the next session.** One slice: both are the ORDER BY comparator's NULL rule and both decisions were taken together on 2026-08-02, so it is implementation, not deliberation. Largest movement available — 2 DIFFER + 3 GAP + `win_first_value_unfiltered`. Three comparator sites, and they currently disagree with *each other* (the window's internal sort puts NULLs first on DESC, the outer sort puts them last). Land the five new `null_edges` cases as the acceptance criteria first — see [P13](#p13) | -| **6** | [P37](#p37) window in `WHERE` returns 0 rows | **Silent — take this first.** Zero rows, success exit code, no error. Reads as "no matching data", not as a defect (the [P30](#p30) trap). Shares a fix site with [P15](#p15), so the two are one piece of work. Not taken for the 2026-09-05 session only because the NEXT row above is one already-decided comparator change; this stays the top *silent-bug* priority | +| ~~9c~~ | ~~[P17](#p17) + [P13](#p13) stage 2 — NULL ordering~~ | ✅ **Fixed 2026-09-05** — 141 → **152 AGREE**, eleven cases in one change. Both halves were the same comparator's NULL rule, so they were taken as one slice. The two sorts that disagreed with each other now *share* one function (`compare_for_order_by`), which is the part that stops the divergence recurring; the window site turned out to be sorting NULL as the **maximum** via a derived `PartialOrd`, not merely following a different rule | +| **NEXT** | [P37](#p37) window in `WHERE` returns 0 rows | **Silent — take this next.** Zero rows, success exit code, no error. Reads as "no matching data", not as a defect (the [P30](#p30) trap). Shares a fix site with [P15](#p15), so the two are one piece of work. Deferred once already (2026-09-05) for the NULL-ordering slice above; it is now the top priority outright | | 9b | [P14](#p14), [P20](#p20), [P23](#p23) | Smaller, self-contained, decisions already taken | | 10 | [P22](#p22), [P25](#p25), [P26](#p26), [P15](#p15), [P32](#p32), [P38](#p38) | Hard errors — visible, so less urgent than any of the above | | 10b | [P39](#p39) `x/0` errors, voiding the whole statement | Hard error like row 10, but the only one whose blast radius is the *query* rather than the cell. Settle the four inconsistent call sites as one decision; it currently has no live probe (see the entry) | @@ -457,12 +457,14 @@ annotation be removed. input. ### P13 — Unparsed trailing tokens are silently discarded, taking later clauses with them -- **Status:** 🟡 STAGE 1 DONE (2026-08-02) — the parser now rejects trailing - input. Stage 2 (implement `NULLS FIRST` / `LAST`) outstanding. -- **Corpus:** `08_ordering.toml :: trailing_garbage_token` (OURS_ONLY — the root +- **Status:** 🟢 FIXED — stage 1 (reject trailing input) 2026-08-02, stage 2 + (`NULLS FIRST` / `NULLS LAST`) 2026-09-05 in one slice with [P17](#p17). +- **Corpus:** `08_ordering.toml :: trailing_garbage_token` (BOTH_ERR — the root cause, pinned directly), `order_by_nulls_last_limit` and - `order_by_nulls_first_limit` (DIFFER — the instance users actually hit). - Controls: `order_by_limit`, `order_by_nulls_last_no_limit` (both AGREE). + `order_by_nulls_first_limit_nullfree` (the instance users actually hit; both + AGREE since stage 2). Controls: `order_by_limit`, + `order_by_nulls_last_no_limit`. The stage-2 acceptance criteria are the five + `null_edges.csv` cases listed below, not these. - **Observed:** `ORDER BY amount DESC NULLS LAST LIMIT 3` returns **all 20 rows** instead of 3. No error. The `LIMIT` is simply gone. - **Root cause — broader than it first looks.** `NULLS` is not the issue; there @@ -494,9 +496,12 @@ annotation be removed. AGREE. - **Acceptance test for stage 2:** `examples/jsonl_logs.sql` carries the `ORDER BY latency_ms DESC NULLS LAST LIMIT 5` statement that motivated this, - marked `-- [SKIP]` (2026-09-02) so the examples smoke run stays green, next - to the NULLS-free version that does run. Drop the directive when stage 2 - lands. Note the JSONL fixture *does* contain NULLs inside the filtered set + which was marked `-- [SKIP]` (2026-09-02) so the examples smoke run stayed + green, next to the NULLS-free version that did run. **The `[SKIP]` was dropped + 2026-09-05** and both statements now return the identical top 5 — the + filtered set contains a row with a NULL `latency_ms`, so `NULLS LAST` is doing + real work there and the default agreeing with it is the P17 change visible + end-to-end. Note the JSONL fixture *does* contain NULLs inside the filtered set (16 rows pass `status IS NOT NULL`, one of which has a NULL `latency_ms`), so unlike the corpus it can pin real NULL-ordering semantics, not just the lost `LIMIT`. @@ -522,6 +527,7 @@ annotation be removed. clause. `order_by_nulls_first_limit` carries both halves of P13 at once and is the sharpest single check: the answer is exactly ids 3, 10, 11 (all NULL-scored), so a dropped clause returns a different *set*, not a subtly different order. + All five closed 2026-09-05, `order_by_nulls_first_limit` included. - **Stage 1, as built (2026-08-02).** `Parser::parse` now ends with `expect_end_of_statement()`, which accepts an optional trailing `;` and trailing comments and otherwise errors with the offending token and its @@ -544,6 +550,24 @@ annotation be removed. `GO` semantics are untouched, and statement *scope* is unaffected — the executor builds one `ExecutionContext` per script, so `INTO #tmp` stays visible across both separators (verified explicitly). +- **Stage 2, as built (2026-09-05).** `OrderByItem` gained a `nulls: + NullsOrder` field (`Unspecified` / `First` / `Last`), and + `OrderByItem::nulls_first()` is the single place the default lives — see + [P17](#p17) for the comparator work it feeds. + - **`NULLS`, `FIRST` and `LAST` were deliberately *not* made keywords.** They + are matched contextually, as identifiers, in the one position they can + appear. All three are plausible column names — `first` and `last` + especially — and reserving them would have broken queries that have nothing + to do with NULL ordering, in a codebase that reads user CSVs with arbitrary + headers. `SELECT nulls FROM t ORDER BY nulls` still parses, and there is a + test that says so. + - **A malformed clause errors rather than being ignored**, which is stage 1's + rule applied to the feature stage 1 was blocking: `NULLS SIDEWAYS` names the + offending token. + - **The formatters round-trip what was typed, not what was resolved.** That is + why `Unspecified` is a distinct variant from `Last` even though the two mean + the same thing to the comparator — printing `NULLS LAST` onto a query the + user never wrote it in would be a silent edit. - **What stage 1 exposed.** Beyond the above: `OR` in a `JOIN ... ON` clause is not parsed ([P27](#p27)), and a stray `end` token had been sitting unnoticed in `examples/case_when.sql`. Two examples (`prime_numbers`, @@ -776,18 +800,18 @@ practice — "top N by total" is where silently returning group order is most likely to be believed. ### P17 — Default NULL placement differs on `ASC` -- **Status:** 🔴 OPEN — **decision made 2026-08-02: follow the reference engine - (NULLS LAST in both directions), plus explicit `NULLS FIRST`/`LAST` from P13 - stage 2.** Queued 2026-09-04 as **one slice with [P13](#p13) stage 2** — same - comparators, and the decision on both was taken in the same sitting. +- **Status:** 🟢 FIXED 2026-09-05, in one slice with [P13](#p13) stage 2 — same + comparators, and the decision on both was taken in the same sitting + (2026-08-02). Parity 141 → **152 AGREE**; eleven cases closed at once. - **Corpus:** `08_ordering.toml :: order_by_null_default_asc_numeric`, - `order_by_null_default_asc_string` (DIFFER); `order_by_null_default_desc` - (AGREE). Second site: `09_window.toml :: win_first_value_unfiltered` (DIFFER). -- **What proves the default actually moved:** the two ASC `DIFFER` cases flip to - AGREE. Note `order_by_null_default_desc` cannot help — it AGREEs today for the - wrong reason and will keep AGREEing afterwards. The explicit-clause case - `order_by_nulls_last_asc_numeric` (added 2026-09-04, see P13) is the one that - pins the new default and the explicit form converging on the same answer. + `order_by_null_default_asc_string` (were DIFFER, now AGREE); + `order_by_null_default_desc` (AGREE throughout). Second site: + `09_window.toml :: win_first_value_unfiltered` (was DIFFER, now AGREE). +- **What proved the default actually moved:** the two ASC `DIFFER` cases flipped + to AGREE. Note `order_by_null_default_desc` could not help — it AGREEd before + for the wrong reason and still AGREEs, now for the right one. The + explicit-clause case `order_by_nulls_last_asc_numeric` (added 2026-09-04, see + P13) pins the new default and the explicit form converging on one answer. - **Two sites, and they disagree with each other.** Added 2026-08-02 while fixing P21. Besides the main `ORDER BY` path, a window's *internal* `ORDER BY` sorts in `window_context.rs::sort_rows` — and it places NULLs **first on @@ -824,11 +848,42 @@ likely to be believed. 2. Implement explicit `NULLS FIRST` / `NULLS LAST` (P13 stage 2), after which the default matters much less because users can override it. This is a user-visible behaviour change on `ORDER BY ` over NULL-bearing - data; call it out in the changelog when it lands. + data; called out in `CHANGELOG.md` under `[Unreleased]`. Note that file had + gone stale (newest entry 1.69.1 against a 1.83.5 `Cargo.toml`) and no earlier + parity fix was logged in it — an `[Unreleased]` section was opened rather than + inventing a version number. - **Note:** `order_by_null_default_desc` AGREEs *for the wrong reason* — the two different rules coincide there. It is kept as a case precisely to document - that. Under the decision above it will keep AGREEing, now for the right reason; - the two ASC cases flip DIFFER → AGREE and their `expect` should be dropped. + that. +- **Fix, as built (2026-09-05).** One comparator now serves every `ORDER BY` in + the engine: `datavalue_compare::compare_for_order_by(a, b, ascending, + nulls_first)`. Both call sites — `DataView::apply_multi_sort` and + `window_context::compare_by_sort_cols` — delegate to it, which is what + actually stops the two from drifting apart again; the corpus cases only prove + it for one shape each. Three things worth recording: + 1. **NULL placement is applied *before* direction and is never reversed by + it.** `NULLS LAST` means last in the output whichever way the values sort. + A comparator that reversed the NULL arm along with the values would pass + every ASC case and fail the DESC ones, so both directions are asserted. + 2. **The window site was worse than "a different rule".** It compared + `DataValue`s through their *derived* `PartialOrd`, which orders by variant + index — and `Null` is the last variant, so NULL sorted as the **maximum**, + then got reversed by DESC into first place. The same derived ordering also + compared cross-type values by variant rather than by value, so + `Integer(100)` sorted below `Float(1.0)` in a window's internal sort. + Routing this site through the shared comparator fixed that too; it has its + own regression test. + 3. **`compare_datavalues` was deliberately left alone.** It still sorts NULL + as the minimum, because it is shared with aggregates, `MIN`/`MAX` and TUI + column sorting, where that is not the same question. The `ORDER BY` rule + lives in one wrapper rather than in the general-purpose comparator. +- **A third comparator exists and was *not* changed:** + `csv_datasource.rs::sort_results` sorts `serde_json::Value`s and places NULLs + first. It is unreachable — it hangs off `CsvApiClient`, which `buffer.rs` + keeps only "for API compatibility" and never calls, and the `DataSourceAdapter` + that would reach it has no callers either. Left as-is rather than fixed + blind; noted here so that whoever revives that path knows it needs the same + rule. Reviving it without this is a silent divergence, not a compile error. ### P18 — `= NULL` matches NULL rows instead of yielding UNKNOWN - **Status:** 🟢 FIXED 2026-08-22, with P19 — branch diff --git a/examples/jsonl_logs.sql b/examples/jsonl_logs.sql index 20b857e..ae550ef 100644 --- a/examples/jsonl_logs.sql +++ b/examples/jsonl_logs.sql @@ -73,13 +73,12 @@ ORDER BY latency_ms DESC LIMIT 5; GO --- [SKIP] --- The same query as originally written, with an explicit null-ordering --- clause. SKIPPED: NULLS FIRST / NULLS LAST is not parsed -- parity issue --- P13 stage 2 (see docs/SQL_PARITY.md). There is no NULLS handling anywhere --- in src/sql/. Before P13 stage 1 the clause was silently swallowed along --- with the LIMIT below it, so this returned every row instead of 5; it is a --- parse error today. Drop the [SKIP] when stage 2 lands. +-- The same query as originally written, with an explicit null-ordering clause. +-- This is the query that motivated P13: before stage 1 the NULLS clause was +-- silently swallowed along with the LIMIT below it, so it returned every row +-- instead of 5; after stage 1 it was a parse error; since stage 2 (2026-09-05) +-- it runs. One row in the filtered set has a NULL latency_ms, so NULLS LAST is +-- doing real work here -- it is what keeps that row out of the top 5. SELECT method, path, status, latency_ms FROM READ_JSONL('data/app_logs.jsonl') WHERE status IS NOT NULL diff --git a/src/data/data_view.rs b/src/data/data_view.rs index 0cd499d..275966e 100644 --- a/src/data/data_view.rs +++ b/src/data/data_view.rs @@ -8,7 +8,9 @@ use tracing::{debug, info}; use crate::data::data_provider::DataProvider; use crate::data::datatable::{DataRow, DataTable, DataValue}; -use crate::data::datavalue_compare::{compare_datavalues, compare_optional_datavalues}; +use crate::data::datavalue_compare::{ + compare_datavalues, compare_for_order_by, compare_optional_datavalues, +}; /// Sort order for columns #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -1068,14 +1070,14 @@ impl DataView { } /// Apply multi-column sorting - /// Each tuple contains (`source_column_index`, ascending) - pub fn apply_multi_sort(&mut self, sort_columns: &[(usize, bool)]) -> Result<()> { + /// Each tuple contains (`source_column_index`, ascending, `nulls_first`) + pub fn apply_multi_sort(&mut self, sort_columns: &[(usize, bool, bool)]) -> Result<()> { if sort_columns.is_empty() { return Ok(()); } // Validate all column indices first - for (col_idx, _) in sort_columns { + for (col_idx, _, _) in sort_columns { if *col_idx >= self.source.column_count() { return Err(anyhow::anyhow!( "Source column index {} out of bounds", @@ -1087,15 +1089,17 @@ impl DataView { let source = &self.source; self.visible_rows.sort_by(|&a, &b| { // Compare by each column in order until we find a difference - for (col_idx, ascending) in sort_columns { + for (col_idx, ascending, nulls_first) in sort_columns { let val_a = source.get_value(a, *col_idx); let val_b = source.get_value(b, *col_idx); - let cmp = compare_optional_datavalues(val_a, val_b); + // Direction and NULL placement are applied together here: the + // NULL rule is absolute and must not be flipped by DESC. + let cmp = compare_for_order_by(val_a, val_b, *ascending, *nulls_first); // If values are different, return the comparison if cmp != std::cmp::Ordering::Equal { - return if *ascending { cmp } else { cmp.reverse() }; + return cmp; } // If equal, continue to next column } @@ -1105,7 +1109,7 @@ impl DataView { }); // Update sort state to reflect the primary sort column - if let Some((primary_col, ascending)) = sort_columns.first() { + if let Some((primary_col, ascending, _)) = sort_columns.first() { // Find the visible column index for the primary sort column if let Some(visible_idx) = self.visible_columns.iter().position(|&x| x == *primary_col) { diff --git a/src/data/datavalue_compare.rs b/src/data/datavalue_compare.rs index b1e98ec..cb4f321 100644 --- a/src/data/datavalue_compare.rs +++ b/src/data/datavalue_compare.rs @@ -101,6 +101,62 @@ pub fn compare_datavalues(a: &DataValue, b: &DataValue) -> Ordering { } } +/// Is this cell NULL for ordering purposes? +/// +/// A missing cell (`None`, e.g. a short row) and an explicit `DataValue::Null` +/// are the same thing to `ORDER BY` - both are the SQL NULL. +#[must_use] +pub fn is_null_for_ordering(v: Option<&DataValue>) -> bool { + matches!(v, None | Some(DataValue::Null)) +} + +/// Compare two cells for `ORDER BY`, applying the direction and the NULL rule. +/// +/// This is the single comparator behind every `ORDER BY` in the engine - the +/// top-level one and a window's internal one - so that they cannot drift apart +/// again (see P17). +/// +/// Two rules, deliberately independent of each other: +/// - `ascending` reverses the comparison of two non-NULL values. +/// - `nulls_first` places NULLs **absolutely**, at the head or the tail of the +/// result. It is *not* reversed by `DESC`: `NULLS LAST` means last in the +/// output whichever direction the values are sorted in, which is what both +/// the SQL standard's explicit clause and DuckDB's default mean. +#[must_use] +pub fn compare_for_order_by( + a: Option<&DataValue>, + b: Option<&DataValue>, + ascending: bool, + nulls_first: bool, +) -> Ordering { + match (is_null_for_ordering(a), is_null_for_ordering(b)) { + (true, true) => return Ordering::Equal, + (true, false) => { + return if nulls_first { + Ordering::Less + } else { + Ordering::Greater + } + } + (false, true) => { + return if nulls_first { + Ordering::Greater + } else { + Ordering::Less + } + } + (false, false) => {} + } + + // Both non-NULL: the NULL arms of `compare_datavalues` are unreachable here. + let cmp = compare_optional_datavalues(a, b); + if ascending { + cmp + } else { + cmp.reverse() + } +} + /// Compare `DataValues` with optional values (handling None) #[must_use] pub fn compare_optional_datavalues(a: Option<&DataValue>, b: Option<&DataValue>) -> Ordering { @@ -216,4 +272,84 @@ mod tests { Ordering::Less ); } + + // ===== ORDER BY comparator (P17 / P13 stage 2) ===== + + const NUM: DataValue = DataValue::Integer(5); + + fn cmp(a: Option<&DataValue>, b: Option<&DataValue>, asc: bool, nf: bool) -> Ordering { + compare_for_order_by(a, b, asc, nf) + } + + #[test] + fn null_placement_is_absolute_not_reversed_by_desc() { + // The whole point of the rule: NULLS LAST means last in the output, in + // BOTH directions. A comparator that reversed the NULL arm along with + // the values would pass the ASC half of this test and fail the DESC half. + for ascending in [true, false] { + assert_eq!( + cmp(Some(&DataValue::Null), Some(&NUM), ascending, false), + Ordering::Greater, + "NULLS LAST, ascending={ascending}" + ); + assert_eq!( + cmp(Some(&DataValue::Null), Some(&NUM), ascending, true), + Ordering::Less, + "NULLS FIRST, ascending={ascending}" + ); + } + } + + #[test] + fn default_is_nulls_last_in_both_directions() { + // P17: the recorded choice. `nulls_first = false` is what + // `OrderByItem::nulls_first()` returns for an unspecified clause. + assert_eq!( + cmp(Some(&NUM), Some(&DataValue::Null), true, false), + Ordering::Less + ); + assert_eq!( + cmp(Some(&NUM), Some(&DataValue::Null), false, false), + Ordering::Less + ); + } + + #[test] + fn missing_cell_and_explicit_null_are_the_same_null() { + // A short row yields None; a parsed empty field yields DataValue::Null. + // ORDER BY must not tell them apart, or NULL placement would depend on + // how the row happened to be stored. + assert_eq!( + cmp(None, Some(&DataValue::Null), true, false), + Ordering::Equal + ); + assert_eq!(cmp(None, Some(&NUM), true, false), Ordering::Greater); + assert_eq!(cmp(None, Some(&NUM), true, true), Ordering::Less); + } + + #[test] + fn direction_still_reverses_non_null_values() { + let ten = DataValue::Integer(10); + assert_eq!(cmp(Some(&NUM), Some(&ten), true, false), Ordering::Less); + assert_eq!(cmp(Some(&NUM), Some(&ten), false, false), Ordering::Greater); + // ...and NULL placement does not disturb that. + assert_eq!(cmp(Some(&NUM), Some(&ten), false, true), Ordering::Greater); + } + + #[test] + fn order_by_compares_mixed_numerics_by_value() { + // The window comparator used to reach `DataValue`'s derived PartialOrd, + // which orders by variant: Integer always sorted before Float, and Null + // (the last variant) sorted as the maximum. Both paths now share this + // function, so this is the regression guard for that. + assert_eq!( + cmp( + Some(&DataValue::Integer(100)), + Some(&DataValue::Float(1.0)), + true, + false + ), + Ordering::Greater + ); + } } diff --git a/src/data/query_engine.rs b/src/data/query_engine.rs index 82ed732..63aca2b 100644 --- a/src/data/query_engine.rs +++ b/src/data/query_engine.rs @@ -3200,7 +3200,7 @@ impl QueryEngine { order_by_columns: &[OrderByItem], _exec_context: Option<&ExecutionContext>, ) -> Result { - // Build list of (source_column_index, ascending) tuples + // Build list of (source_column_index, ascending, nulls_first) tuples let mut sort_columns = Vec::new(); for order_col in order_by_columns { @@ -3209,7 +3209,7 @@ impl QueryEngine { if let SqlExpression::NumberLiteral(literal) = &order_col.expr { let col_index = Self::resolve_order_by_ordinal(&view, literal)?; let ascending = matches!(order_col.direction, SortDirection::Asc); - sort_columns.push((col_index, ascending)); + sort_columns.push((col_index, ascending, order_col.nulls_first())); continue; } @@ -3270,7 +3270,7 @@ impl QueryEngine { })?; let ascending = matches!(order_col.direction, SortDirection::Asc); - sort_columns.push((col_index, ascending)); + sort_columns.push((col_index, ascending, order_col.nulls_first())); } // Apply multi-column sorting diff --git a/src/query_plan/ilike_to_like_transformer.rs b/src/query_plan/ilike_to_like_transformer.rs index 8d1f5a0..9ee5b10 100644 --- a/src/query_plan/ilike_to_like_transformer.rs +++ b/src/query_plan/ilike_to_like_transformer.rs @@ -151,6 +151,7 @@ impl ILikeToLikeTransformer { .map(|item| OrderByItem { expr: self.transform_expression(item.expr), direction: item.direction, + nulls: item.nulls, }) .collect() } diff --git a/src/query_plan/order_by_alias_transformer.rs b/src/query_plan/order_by_alias_transformer.rs index 44b2cc9..1042c1d 100644 --- a/src/query_plan/order_by_alias_transformer.rs +++ b/src/query_plan/order_by_alias_transformer.rs @@ -402,7 +402,7 @@ impl OrderByAliasTransformer { #[cfg(test)] mod tests { use super::*; - use crate::sql::parser::ast::{ColumnRef, OrderByItem, QuoteStyle, SortDirection}; + use crate::sql::parser::ast::{ColumnRef, NullsOrder, OrderByItem, QuoteStyle, SortDirection}; #[test] fn test_extract_aggregate_from_order_column() { @@ -467,6 +467,7 @@ mod tests { stmt.order_by = Some(vec![OrderByItem { expr: SqlExpression::NumberLiteral("2".to_string()), direction: SortDirection::Desc, + nulls: NullsOrder::Unspecified, }]); let stmt = OrderByAliasTransformer::new() @@ -494,6 +495,7 @@ mod tests { stmt.order_by = Some(vec![OrderByItem { expr: SqlExpression::NumberLiteral("1.5".to_string()), direction: SortDirection::Asc, + nulls: NullsOrder::Unspecified, }]); let stmt = OrderByAliasTransformer::new() @@ -525,6 +527,7 @@ mod tests { right: Box::new(SqlExpression::NumberLiteral("1".to_string())), }, direction: SortDirection::Asc, + nulls: NullsOrder::Unspecified, }]); let stmt = OrderByAliasTransformer::new() diff --git a/src/sql/parser/ast.rs b/src/sql/parser/ast.rs index 12b75e9..5b9107b 100644 --- a/src/sql/parser/ast.rs +++ b/src/sql/parser/ast.rs @@ -297,11 +297,24 @@ pub struct OrderByColumn { pub direction: SortDirection, } +/// Where NULLs are placed by an `ORDER BY` item. +/// +/// `Unspecified` records that the user wrote no `NULLS` clause, so the +/// formatters can round-trip the query as typed; the default it stands for +/// lives in `nulls_first()` alone. +#[derive(Debug, Clone, Copy, PartialEq)] +pub enum NullsOrder { + Unspecified, + First, + Last, +} + /// Modern ORDER BY item that supports expressions #[derive(Debug, Clone)] pub struct OrderByItem { pub expr: SqlExpression, pub direction: SortDirection, + pub nulls: NullsOrder, } impl OrderByItem { @@ -314,12 +327,44 @@ impl OrderByItem { table_prefix: None, }), direction, + nulls: NullsOrder::Unspecified, } } /// Create from an expression pub fn from_expression(expr: SqlExpression, direction: SortDirection) -> Self { - Self { expr, direction } + Self { + expr, + direction, + nulls: NullsOrder::Unspecified, + } + } + + /// Should NULLs sort to the head of the result for this item? + /// + /// The default is **NULLS LAST in both directions**, following the + /// reference engine (P17). The standard leaves this implementation-defined + /// and the major engines disagree, so it is a recorded choice rather than a + /// correction: SQLite and MySQL sort NULL as the smallest value, PostgreSQL + /// as the largest, DuckDB pins NULLS LAST either way. This is the one place + /// that choice is written down. + pub fn nulls_first(&self) -> bool { + match self.nulls { + NullsOrder::First => true, + NullsOrder::Last | NullsOrder::Unspecified => false, + } + } + + /// The `NULLS` clause to print, or `None` if the user did not write one. + /// + /// Formatters round-trip the query as typed rather than as resolved, so an + /// unspecified clause stays unspecified even though its meaning is pinned. + pub fn nulls_keyword(&self) -> Option<&'static str> { + match self.nulls { + NullsOrder::Unspecified => None, + NullsOrder::First => Some("NULLS FIRST"), + NullsOrder::Last => Some("NULLS LAST"), + } } } diff --git a/src/sql/parser/ast_formatter.rs b/src/sql/parser/ast_formatter.rs index 0cd591c..29fa5c6 100644 --- a/src/sql/parser/ast_formatter.rs +++ b/src/sql/parser/ast_formatter.rs @@ -196,6 +196,9 @@ impl<'a> AstFormatter<'a> { write!(&mut result, " {}", self.keyword("DESC")).unwrap() } } + if let Some(nulls) = col.nulls_keyword() { + write!(&mut result, " {}", self.keyword(nulls)).unwrap(); + } } } @@ -975,6 +978,10 @@ impl<'a> AstFormatter<'a> { result.push_str(&self.keyword("DESC")); } } + if let Some(nulls) = col.nulls_keyword() { + result.push(' '); + result.push_str(&self.keyword(nulls)); + } } } diff --git a/src/sql/parser/formatter.rs b/src/sql/parser/formatter.rs index a8d7be3..7a5db3c 100644 --- a/src/sql/parser/formatter.rs +++ b/src/sql/parser/formatter.rs @@ -804,7 +804,10 @@ pub fn format_expression(expr: &SqlExpression) -> String { SqlExpression::Column(col_ref) => col_ref.name.clone(), _ => format_expression(&col.expr), }; - format!("{}{}", expr_str, dir) + let nulls = col + .nulls_keyword() + .map_or(String::new(), |k| format!(" {k}")); + format!("{expr_str}{dir}{nulls}") }) .collect(); result.push_str(&order_strs.join(", ")); diff --git a/src/sql/recursive_parser.rs b/src/sql/recursive_parser.rs index cc58c30..ec2418b 100644 --- a/src/sql/recursive_parser.rs +++ b/src/sql/recursive_parser.rs @@ -3,10 +3,10 @@ // Re-exports for backward compatibility - these serve as both imports and re-exports pub use super::parser::ast::{ CTEType, Comment, Condition, DataFormat, FileCTESpec, FrameBound, FrameUnit, HttpMethod, - IntoTable, JoinClause, JoinCondition, JoinOperator, JoinType, LogicalOp, OrderByColumn, - OrderByItem, PivotAggregate, SelectItem, SelectStatement, SetOperation, SingleJoinCondition, - SortDirection, SqlExpression, TableFunction, TableSource, WebCTESpec, WhenBranch, WhereClause, - WindowFrame, WindowSpec, CTE, + IntoTable, JoinClause, JoinCondition, JoinOperator, JoinType, LogicalOp, NullsOrder, + OrderByColumn, OrderByItem, PivotAggregate, SelectItem, SelectStatement, SetOperation, + SingleJoinCondition, SortDirection, SqlExpression, TableFunction, TableSource, WebCTESpec, + WhenBranch, WhereClause, WindowFrame, WindowSpec, CTE, }; pub use super::parser::legacy::{ ColumnInfo, ColumnType, ParseContext, ParseState, Schema, SqlParser, SqlToken, TableInfo, @@ -1557,7 +1557,13 @@ impl Parser { _ => SortDirection::Asc, // Default to ASC if not specified }; - order_items.push(OrderByItem { expr, direction }); + let nulls = self.parse_nulls_order()?; + + order_items.push(OrderByItem { + expr, + direction, + nulls, + }); if matches!(self.current_token, Token::Comma) { self.advance(); @@ -1569,6 +1575,35 @@ impl Parser { Ok(order_items) } + /// Parse an optional `NULLS FIRST` / `NULLS LAST` suffix on an ORDER BY item. + /// + /// `NULLS`, `FIRST` and `LAST` are matched contextually as identifiers + /// rather than promoted to keywords: all three are plausible column names + /// (`first`, `last` especially), and reserving them would break queries that + /// have nothing to do with NULL ordering. Position does the disambiguating - + /// this is only ever consulted directly after an ORDER BY item. + fn parse_nulls_order(&mut self) -> Result { + let is_kw = |tok: &Token, kw: &str| matches!(tok, Token::Identifier(id) if id.eq_ignore_ascii_case(kw)); + + if !is_kw(&self.current_token, "NULLS") { + return Ok(NullsOrder::Unspecified); + } + self.advance(); + + if is_kw(&self.current_token, "FIRST") { + self.advance(); + Ok(NullsOrder::First) + } else if is_kw(&self.current_token, "LAST") { + self.advance(); + Ok(NullsOrder::Last) + } else { + Err(format!( + "Expected FIRST or LAST after NULLS in ORDER BY, found {:?}", + self.current_token + )) + } + } + /// Parse INTO clause for temporary tables /// Syntax: INTO #table_name fn parse_into_clause(&mut self) -> Result { @@ -3247,4 +3282,77 @@ mod tests { msg ); } + + // ===== ORDER BY ... NULLS FIRST / LAST (P13 stage 2) ===== + + fn order_by_nulls(sql: &str) -> Vec { + let mut parser = Parser::new(sql); + let stmt = parser.parse().unwrap_or_else(|e| panic!("{sql}: {e}")); + stmt.order_by + .expect("order_by") + .iter() + .map(|item| item.nulls) + .collect() + } + + #[test] + fn parses_nulls_clause_with_and_without_a_direction() { + assert_eq!( + order_by_nulls("SELECT a FROM t ORDER BY a NULLS FIRST"), + vec![NullsOrder::First] + ); + assert_eq!( + order_by_nulls("SELECT a FROM t ORDER BY a DESC NULLS LAST"), + vec![NullsOrder::Last] + ); + // Case-insensitive, like every other keyword. + assert_eq!( + order_by_nulls("SELECT a FROM t ORDER BY a asc nulls first"), + vec![NullsOrder::First] + ); + } + + #[test] + fn nulls_clause_is_per_item_not_per_statement() { + assert_eq!( + order_by_nulls("SELECT a, b, c FROM t ORDER BY a NULLS FIRST, b DESC, c NULLS LAST"), + vec![NullsOrder::First, NullsOrder::Unspecified, NullsOrder::Last] + ); + } + + #[test] + fn unspecified_is_distinct_from_an_explicit_last() { + // The two mean the same thing to the comparator but not to the + // formatters, which round-trip the query as typed. + assert_eq!( + order_by_nulls("SELECT a FROM t ORDER BY a"), + vec![NullsOrder::Unspecified] + ); + } + + #[test] + fn nulls_is_not_a_reserved_word() { + // `NULLS`, `FIRST` and `LAST` are matched by position, not promoted to + // keywords - all three are plausible column names, and reserving them + // would break queries that have nothing to do with NULL ordering. + for sql in [ + "SELECT nulls FROM t ORDER BY nulls", + "SELECT first, last FROM t ORDER BY first, last DESC", + "SELECT a FROM t WHERE nulls > 1", + ] { + let mut parser = Parser::new(sql); + assert!(parser.parse().is_ok(), "should still parse: {sql}"); + } + } + + #[test] + fn nulls_without_first_or_last_is_a_loud_error() { + // P13's rule: never silently discard what we could not place. + let mut parser = Parser::new("SELECT a FROM t ORDER BY a NULLS SIDEWAYS"); + let err = parser.parse().unwrap_err().to_string(); + assert!( + err.contains("FIRST or LAST"), + "expected a NULLS-specific error, got: {err}" + ); + } } diff --git a/src/sql/window_context.rs b/src/sql/window_context.rs index 061e366..3458f87 100644 --- a/src/sql/window_context.rs +++ b/src/sql/window_context.rs @@ -12,6 +12,7 @@ use tracing::{debug, info}; use crate::data::data_view::DataView; use crate::data::datatable::{DataTable, DataValue}; +use crate::data::datavalue_compare::compare_for_order_by; use crate::sql::parser::ast::{ FrameBound, FrameUnit, OrderByItem, SortDirection, SqlExpression, WindowSpec, }; @@ -66,7 +67,8 @@ pub struct OrderedPartition { impl OrderedPartition { /// Create a new ordered partition from rows already sorted by `sort_cols` - fn new(rows: Vec, table: &DataTable, sort_cols: &[(usize, bool)]) -> Self { + /// (each entry is `(column index, ascending, nulls_first)`) + fn new(rows: Vec, table: &DataTable, sort_cols: &[(usize, bool, bool)]) -> Self { // Build position lookup let row_positions: HashMap = rows .iter() @@ -89,7 +91,7 @@ impl OrderedPartition { fn compute_peer_bounds( rows: &[usize], table: &DataTable, - sort_cols: &[(usize, bool)], + sort_cols: &[(usize, bool, bool)], ) -> Vec<(usize, usize)> { let mut bounds = vec![(0usize, 0usize); rows.len()]; let mut group_start = 0usize; @@ -370,11 +372,11 @@ impl WindowContext { Ok(OrderedPartition::new(rows, view.source(), &sort_cols)) } - /// Resolve ORDER BY items to (column index, ascending) pairs + /// Resolve ORDER BY items to (column index, ascending, `nulls_first`) triples fn resolve_sort_columns( table: &DataTable, order_by: &[OrderByItem], - ) -> Result> { + ) -> Result> { order_by .iter() .map(|col| { @@ -389,7 +391,7 @@ impl WindowContext { .get_column_index(column_name) .ok_or_else(|| anyhow!("Invalid ORDER BY column: {}", column_name))?; let ascending = matches!(col.direction, SortDirection::Asc); - Ok((idx, ascending)) + Ok((idx, ascending, col.nulls_first())) }) .collect() } @@ -403,42 +405,29 @@ impl WindowContext { table: &DataTable, a: usize, b: usize, - sort_cols: &[(usize, bool)], + sort_cols: &[(usize, bool, bool)], ) -> std::cmp::Ordering { - for &(col_idx, ascending) in sort_cols { + for &(col_idx, ascending, nulls_first) 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(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() }; - } - } + // Shared with the top-level ORDER BY (P17). This used to compare + // `DataValue`s through their derived `PartialOrd`, under which + // `Null` is the last variant and therefore sorts as the *maximum* - + // the exact opposite of the top-level comparator, and the reason + // `FIRST_VALUE(x) OVER (ORDER BY x DESC)` returned NULL. The derived + // ordering also compared cross-type values by variant rather than by + // value, so `Integer` never met `Float` numerically. + let ord = compare_for_order_by(val_a, val_b, ascending, nulls_first); + if ord != std::cmp::Ordering::Equal { + return ord; } } std::cmp::Ordering::Equal } /// Sort row indices according to ORDER BY specification - fn sort_rows(rows: &mut [usize], table: &DataTable, sort_cols: &[(usize, bool)]) { + fn sort_rows(rows: &mut [usize], table: &DataTable, sort_cols: &[(usize, bool, bool)]) { let sort_start = Instant::now(); rows.sort_by(|&a, &b| Self::compare_by_sort_cols(table, a, b, sort_cols)); diff --git a/tests/comparison/corpus/08_ordering.toml b/tests/comparison/corpus/08_ordering.toml index 3618cc7..c049b25 100644 --- a/tests/comparison/corpus/08_ordering.toml +++ b/tests/comparison/corpus/08_ordering.toml @@ -89,27 +89,29 @@ sql = "SELECT id FROM null_edges ORDER BY id LIMIT 100" id = "order_by_nulls_last_no_limit" data = "international_sales.csv" sql = "SELECT country, amount FROM international_sales ORDER BY amount DESC NULLS LAST" -expect = "GAP" -# Was AGREE while P13 was live — the NULLS clause was silently ignored and, with -# no LIMIT to lose, the remaining query happened to be the one the user meant. -# Since P13 stage 1 (2026-08-02) we correctly REJECT the clause we do not -# implement. Flips to AGREE when P13 stage 2 implements NULLS FIRST/LAST. +# AGREE since P13 stage 2 (2026-09-05). Its history is the cautionary tale: it +# was AGREE while P13 was live (the clause was silently ignored and, with no +# LIMIT to lose, the remaining query happened to be the one the user meant), +# GAP after stage 1 correctly rejected the unimplemented clause, and AGREE again +# now for the right reason. international_sales.csv has no NULLs, so this pins +# "the clause parses" and nothing about where NULLs land. [[case]] id = "order_by_nulls_last_limit" data = "international_sales.csv" sql = "SELECT country, amount FROM international_sales ORDER BY amount DESC NULLS LAST LIMIT 3" -expect = "GAP" -# Was P13: returned all 20 rows instead of 3, silently, because NULLS LAST took -# the LIMIT with it. Now a parse error — the correct intermediate state, since a -# refusal beats a different-query-that-succeeds. Flips to AGREE with stage 2. +# The original P13 reproducer: returned all 20 rows instead of 3, silently, +# because NULLS LAST took the LIMIT with it. Stage 1 made it a parse error; +# stage 2 (2026-09-05) makes it AGREE. NULL-free data, so it pins the surviving +# LIMIT only. [[case]] -id = "order_by_nulls_first_limit" +id = "order_by_nulls_first_limit_nullfree" data = "international_sales.csv" sql = "SELECT country, amount FROM international_sales ORDER BY amount NULLS FIRST LIMIT 3" -expect = "GAP" -# Same, the FIRST/ASC form. +# Same, the FIRST/ASC form. Renamed 2026-09-05: it had collided with the +# null_edges case of the same name below, so `--check` reported one id twice and +# an id filter would have run both. The suffix marks which one cannot see NULLs. [[case]] id = "trailing_garbage_token" @@ -197,18 +199,19 @@ sql = "SELECT id, score FROM null_edges ORDER BY score DESC, id" id = "order_by_null_default_asc_numeric" data = "null_edges.csv" sql = "SELECT id, score FROM null_edges ORDER BY score, id" -expect = "DIFFER" -# P17. We sort NULL as the smallest value (SQLite/MySQL convention) so NULLs -# come FIRST; DuckDB sorts NULLS LAST regardless of direction. Standard SQL -# leaves this implementation-defined, so this needs a decision, not a reflex fix. +# P17, closed 2026-09-05. We used to sort NULL as the smallest value +# (SQLite/MySQL convention) so NULLs came FIRST; the default is now NULLS LAST +# in both directions, following the reference engine. Standard SQL leaves this +# implementation-defined, so this case pins a recorded *choice*, not a law — +# which is exactly why it is worth keeping now that it AGREEs. [[case]] id = "order_by_null_default_asc_string" data = "null_edges.csv" sql = "SELECT id, label FROM null_edges ORDER BY label, id" -expect = "DIFFER" -# P17 on a string column — pinned separately to prove the rule is type-independent -# and that a fix has to cover both comparators. +# P17 on a string column — pinned separately to prove the rule is type-independent. +# It earned its keep: the fix routes NULL placement ahead of the type-specific +# comparison, so a numeric-only fix would have failed here. # --- P13 stage 2 / P17: explicit NULLS FIRST/LAST, over data that actually has # NULLs. @@ -225,24 +228,23 @@ expect = "DIFFER" # explicit `NULLS LAST` no longer distinguishes "honoured" from "ignored" — it # agrees with the default either way. The load-bearing cases are therefore the # NULLS FIRST ones. `order_by_nulls_last_asc_numeric` is kept anyway because it -# is the case that changes meaning *today* (we currently sort NULL as the -# minimum, so ASC puts them first), and it documents the default converging on -# the explicit form rather than leaving that untested. +# is the case whose meaning the P17 default change moved (we used to sort NULL +# as the minimum, so ASC put them first), and it documents the default +# converging on the explicit form rather than leaving that untested. [[case]] id = "order_by_nulls_first_asc_numeric" data = "null_edges.csv" sql = "SELECT id, score FROM null_edges ORDER BY score NULLS FIRST, id" -expect = "GAP" -# THE case for P13 stage 2: the only shape that disagrees with the post-P17 -# default, so it is the one that fails if the clause is parsed and dropped. +# THE case for P13 stage 2 (closed 2026-09-05): the only shape that disagrees +# with the post-P17 default, so it is the one that fails if the clause is parsed +# and dropped. # Expected: ids 3, 10, 11, 12 (the NULL scores) first, then ascending by score. [[case]] id = "order_by_nulls_last_asc_numeric" data = "null_edges.csv" sql = "SELECT id, score FROM null_edges ORDER BY score NULLS LAST, id" -expect = "GAP" # Agrees with the post-P17 default, so it cannot catch an ignored clause — kept # because it is the explicit form of the behaviour P17 changes, and its flip to # AGREE is what proves the default moved. @@ -251,7 +253,6 @@ expect = "GAP" id = "order_by_nulls_first_desc_numeric" data = "null_edges.csv" sql = "SELECT id, score FROM null_edges ORDER BY score DESC NULLS FIRST, id" -expect = "GAP" # DESC is where the two engines' defaults coincide (see order_by_null_default_desc), # so an explicit NULLS FIRST is the only way to pin the DESC comparator honouring # the clause rather than falling through to a default that happens to match. @@ -260,7 +261,6 @@ expect = "GAP" id = "order_by_nulls_first_string" data = "null_edges.csv" sql = "SELECT id, label FROM null_edges ORDER BY label NULLS FIRST, id" -expect = "GAP" # The string comparator, mirroring order_by_null_default_asc_string: a fix that # reaches only the numeric path is caught here. @@ -268,7 +268,6 @@ expect = "GAP" id = "order_by_nulls_first_limit" data = "null_edges.csv" sql = "SELECT id, score FROM null_edges ORDER BY score NULLS FIRST, id LIMIT 3" -expect = "GAP" # P13's two halves in one case, over real NULLs: the LIMIT must survive the # NULLS clause *and* the clause must reorder. Expected exactly ids 3, 10, 11 — # all NULL-scored, so a dropped clause returns a visibly different set rather diff --git a/tests/comparison/corpus/09_window.toml b/tests/comparison/corpus/09_window.toml index 0819814..655b955 100644 --- a/tests/comparison/corpus/09_window.toml +++ b/tests/comparison/corpus/09_window.toml @@ -146,15 +146,15 @@ sql = "SELECT id, team, FIRST_VALUE(score) OVER (PARTITION BY team ORDER BY scor id = "win_first_value_unfiltered" data = "null_edges.csv" sql = "SELECT id, team, FIRST_VALUE(score) OVER (PARTITION BY team ORDER BY score DESC, id) AS v FROM null_edges ORDER BY id" -expect = "DIFFER" -# P17, in `window_context.rs::sort_rows` rather than the main ORDER BY path. -# Partition 'alpha' is (50, 50, NULL). Sorted DESC the NULL must go LAST, so -# FIRST_VALUE is 50; we sort it FIRST and return NULL. +# P17's second site, closed 2026-09-05: `window_context.rs` rather than the main +# ORDER BY path. Partition 'alpha' is (50, 50, NULL); sorted DESC the NULL must +# go LAST, so FIRST_VALUE is 50 — we used to sort it FIRST and return NULL. # -# Note this is the OPPOSITE of the outer ORDER BY's behaviour, which puts NULLs -# last on DESC (`08_ordering.toml :: order_by_null_default_desc` AGREEs). So the -# two sorts in the engine disagree with each other as well as with the -# reference. The P17 fix has to reach both. +# This was the OPPOSITE of the outer ORDER BY, which already put NULLs last on +# DESC (`08_ordering.toml :: order_by_null_default_desc`), so the two sorts +# disagreed with each other as well as with the reference. They now share one +# comparator (`compare_for_order_by`), which is what actually stops them +# drifting apart again — this case only proves it for one shape. # --- P22: unimplemented window functions return NULL instead of erroring --- diff --git a/tests/test_multi_column_order_by.rs b/tests/test_multi_column_order_by.rs index 0865d1b..e6f07d3 100644 --- a/tests/test_multi_column_order_by.rs +++ b/tests/test_multi_column_order_by.rs @@ -326,8 +326,9 @@ fn test_direct_multi_sort_method() { let mut view = DataView::new(Arc::new(table)); - // Sort by col1 ASC, col2 DESC, col3 ASC - view.apply_multi_sort(&[(0, true), (1, false), (2, true)]) + // Sort by col1 ASC, col2 DESC, col3 ASC (no NULLs in this fixture, so the + // third element - nulls_first - is not exercised here) + view.apply_multi_sort(&[(0, true, false), (1, false, false), (2, true, false)]) .unwrap(); // Verify the sorting diff --git a/tests/test_window_context.rs b/tests/test_window_context.rs index 8db6e5c..17c8fe2 100644 --- a/tests/test_window_context.rs +++ b/tests/test_window_context.rs @@ -1,6 +1,6 @@ use sql_cli::data::data_view::DataView; use sql_cli::data::datatable::{DataColumn, DataRow, DataTable, DataValue}; -use sql_cli::sql::parser::ast::{ColumnRef, OrderByItem, QuoteStyle, SqlExpression}; +use sql_cli::sql::parser::ast::{ColumnRef, NullsOrder, OrderByItem, QuoteStyle, SqlExpression}; use sql_cli::sql::recursive_parser::SortDirection; use sql_cli::sql::window_context::WindowContext; use std::sync::Arc; @@ -35,6 +35,7 @@ fn test_window_context_single_partition() { table_prefix: None, }), direction: SortDirection::Asc, + nulls: NullsOrder::Unspecified, }], ) .unwrap(); @@ -113,6 +114,7 @@ fn test_window_context_with_partitions() { table_prefix: None, }), direction: SortDirection::Asc, + nulls: NullsOrder::Unspecified, }], ) .unwrap(); @@ -185,6 +187,7 @@ fn test_window_context_order_by_desc() { table_prefix: None, }), direction: SortDirection::Desc, + nulls: NullsOrder::Unspecified, }], ) .unwrap(); @@ -251,6 +254,7 @@ fn order_by_score() -> Vec { table_prefix: None, }), direction: SortDirection::Asc, + nulls: NullsOrder::Unspecified, }] }