From a658e30249e3fa6382b1defc086962caad403ade Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Sun, 30 Aug 2026 19:59:39 +0100 Subject: [PATCH] fix(engine): stop ORDER BY splitting quoted column names at the dot (P34) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ORDER BY "name.common"` failed with the self-contradicting error `Column 'name.common' not found. Did you mean 'name.common'?` on any CSV whose header carries dotted names (data/countries.csv). The same column resolved fine in the SELECT list, which made it look like an ORDER BY parsing bug. It is not parsing. The AST is correct: the parser yields ColumnRef { name: "name.common", quote_style: DoubleQuotes, table_prefix: None }, and a genuinely qualified `countries.region` yields name: "region" with table_prefix: Some("countries"). The bug was in resolution — apply_multi_order_by_with_context treated any dot as a table qualifier and looked up only the part after the last dot (`common`), which does not exist. The suggestion was computed from the full name, hence the error naming the column it had just refused to find. Try the literal column name first, and fall back to qualifier-stripping only for an unquoted reference. A double-quoted identifier is one name, dots included — that is what the quotes are for. The fallback stays for unquoted dotted names that older parse paths can still produce. Files P34 (fixed) and R11 (open): resolve_column_index is documented as the canonical resolver "used by all SQL clauses ... to ensure consistent alias resolution" and already handles this case correctly, but ORDER BY never called it and hand-rolled a worse copy. Converging them changes behaviour for unquoted dotted names (qualified-name match vs. suffix strip), so that is left to its own change with a parity run rather than ridden along on a bug fix. Verified on the reported query: all 250 rows come back in exactly the order a 4-key lexicographic sort gives. Also checked DESC, ordering on a quoted dotted column not in the SELECT list, GROUP BY on one, multi-dot names ("translations.ara.common"), and that a genuinely missing dotted column still errors rather than silently sorting wrong. Tests: 1187 passed, 0 failed. cargo fmt and clippy clean. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BfD1B6fURb22qrCwxjhc2q --- docs/ENGINE_REFACTORING.md | 34 ++++++++++++++ docs/SQL_PARITY.md | 40 ++++++++++++++++ src/data/query_engine.rs | 73 +++++++++++++++-------------- tests/test_multi_column_order_by.rs | 69 +++++++++++++++++++++++++++ 4 files changed, 182 insertions(+), 34 deletions(-) diff --git a/docs/ENGINE_REFACTORING.md b/docs/ENGINE_REFACTORING.md index 37dae735..36ba6fae 100644 --- a/docs/ENGINE_REFACTORING.md +++ b/docs/ENGINE_REFACTORING.md @@ -385,6 +385,38 @@ feature work**, and so we can tell the difference between "this is awkward" and them. The compiler's list is exhaustive by definition; the transform was applied from its own line/column output. +### R11 — ORDER BY resolves columns with its own copy of the resolver +- **Status:** 🔴 OPEN — filed 2026-08-30 by [P34](SQL_PARITY.md#p34) +- **Where:** `query_engine::apply_multi_order_by_with_context` vs. + `query_engine::resolve_column_index` (same file, ~3000 lines apart) +- **Observed:** `resolve_column_index` carries a doc comment declaring itself + the canonical resolver, "used by all SQL clauses (WHERE, SELECT, ORDER BY, + GROUP BY) to ensure consistent alias resolution behavior". ORDER BY does not + call it. It has its own inline resolution instead, and that copy differs on + two points: it treats any dot in the name as a table qualifier and takes the + suffix (the canonical one tries the literal name **first**), and it ignores + `table_prefix` entirely (the canonical one resolves the alias and tries the + qualified name). The first divergence was a live wrong-answer bug for + quoted dotted column names — [P34](SQL_PARITY.md#p34). +- **Impact:** one hard error, now fixed narrowly in place. The second + divergence — ORDER BY ignoring `table_prefix` — has no known failing case + because the fallback to the bare name happens to work after projection + unqualifies the columns. That is luck, not design. +- **Same shape as [R9](#r9):** one helper documented as the single + implementation, plus a hand-rolled copy at a site that never adopted it, and + the copy is the one that is wrong. As with R9, the copy looks locally + reasonable — nothing at the ORDER BY site hints that a shared resolver exists. +- **Deliberately deferred at fix time.** Converging on `resolve_column_index` + changes behaviour for *unquoted* dotted names: the canonical path matches a + column's `qualified_name`, the ORDER BY copy strips to the suffix. Those + differ whenever a table's columns were not enriched with qualified names, so + the swap is a behaviour change and wants its own change with parity run, + not a rider on a bug fix. The P34 fix therefore only reordered the ORDER BY + copy's own lookups (literal first) and left the copy in place. +- **When done:** delete the inline resolution, call `resolve_column_index`, and + keep both P34 regression tests green — they pin exactly the two behaviours + the shared resolver has to reproduce. + --- ## Sequencing @@ -407,6 +439,7 @@ R4 fixtures ──────── adopt opportunistically, per transformer to R5 dead code ─────── opportunistic R8 legacy WHERE ──── independent; stage 2 is self-contained, do it in a lull R10 Trilean ──────── DONE; closed P18/P19 (parity 125 → 129) +R11 ORDER BY resolver ─ independent; small, but a behaviour change — wants its own parity run ``` **A note on ordering, from the P18/P19 work being next.** The WHERE evaluator @@ -439,3 +472,4 @@ AGREE count — which makes it safe to land well before the semantics change. | 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 | — | +| 2026-08-30 | P34 fixed: `ORDER BY "col.with.dot"` no longer strips a quoted identifier at the dot. R11 filed — ORDER BY still resolves columns with its own copy of `resolve_column_index` rather than the canonical one | — | diff --git a/docs/SQL_PARITY.md b/docs/SQL_PARITY.md index 83ae8e27..8e4113e2 100644 --- a/docs/SQL_PARITY.md +++ b/docs/SQL_PARITY.md @@ -1116,6 +1116,46 @@ out of date. the corpus case doubles as proof that `NOT LIKE` inherits the NULL semantics rather than growing its own. +### P34 — `ORDER BY "col.with.dot"` fails to resolve the column +- **Status:** 🟢 FIXED 2026-08-30 +- **Observed:** with a CSV whose header carries dotted names (`data/countries.csv` + has `name.common`, `name.official`, `translations.ara.common`, ...), + + ``` + SELECT "name.common", region FROM countries ORDER BY region, "name.common" + ``` + + failed with `Column 'name.common' not found. Did you mean 'name.common'?` — + the error naming the column it had just refused to find. The same query + without the quoted column in the ORDER BY worked, and the SELECT list + resolved `"name.common"` correctly, which is what made it look like an + ORDER BY *parsing* problem. +- **Not parsing.** The AST is right: the parser produces + `ColumnRef { name: "name.common", quote_style: DoubleQuotes, table_prefix: None }`, + and a genuinely qualified `countries.region` parses to + `name: "region", table_prefix: Some("countries")`. The bug was in resolution. + `apply_multi_order_by_with_context` treated *any* dot in the name as a + qualifier and looked up only the part after the last dot (`common`), which + does not exist. The "did you mean" suggestion was computed from the *full* + name, hence the self-contradicting message. +- **Fix:** try the literal column name first, and fall back to + qualifier-stripping only for an **unquoted** reference. A double-quoted + identifier is one name, dots included — that is what the quotes are *for*. + The fallback is kept for unquoted dotted names, which older parse paths can + still produce. +- **Regression tests:** `tests/test_multi_column_order_by.rs` — + `test_order_by_quoted_column_containing_dot` (the bug) and + `test_order_by_table_qualified_column_still_resolves` (the fallback the fix + must not break). +- **Note the shape.** `resolve_column_index` (`query_engine.rs:106`) is + documented as *the* canonical resolver "used by all SQL clauses ... to ensure + consistent alias resolution", and it already gets this case right — literal + name first, dotted name as a qualified lookup second. ORDER BY never called + it and hand-rolled a worse copy. The narrow fix above is deliberate: + converging the ORDER BY path onto the shared resolver changes behaviour for + unquoted dotted names (qualified-name match vs. suffix strip), so it belongs + in [`ENGINE_REFACTORING.md`](ENGINE_REFACTORING.md), not in a bug fix. + --- ## Deferred / won't fix (intentional) diff --git a/src/data/query_engine.rs b/src/data/query_engine.rs index 4327eef3..e5c94038 100644 --- a/src/data/query_engine.rs +++ b/src/data/query_engine.rs @@ -21,6 +21,7 @@ use crate::data::temp_table_registry::TempTableRegistry; use crate::execution_plan::{ExecutionPlan, ExecutionPlanBuilder, StepType}; use crate::sql::aggregates::{contains_aggregate, is_aggregate_compatible}; use crate::sql::parser::ast::ColumnRef; +use crate::sql::parser::ast::QuoteStyle; use crate::sql::parser::ast::SetOperation; use crate::sql::parser::ast::TableSource; use crate::sql::parser::ast::WindowSpec; @@ -3155,8 +3156,11 @@ impl QueryEngine { for order_col in order_by_columns { // Extract column name from expression (currently only supports simple columns) - let column_name = match &order_col.expr { - SqlExpression::Column(col_ref) => col_ref.name.clone(), + let (column_name, is_quoted) = match &order_col.expr { + SqlExpression::Column(col_ref) => ( + col_ref.name.clone(), + !matches!(col_ref.quote_style, QuoteStyle::None), + ), _ => { // TODO: Support expression evaluation in ORDER BY return Err(anyhow!( @@ -3165,46 +3169,47 @@ impl QueryEngine { } }; - // Try to find the column index, handling qualified column names (table.column) - let col_index = if column_name.contains('.') { - // Qualified column name - extract unqualified part - if let Some(dot_pos) = column_name.rfind('.') { + // Always try the literal column name first: a quoted identifier such as + // "name.common" is a single column whose name contains a dot, not a + // table-qualified reference (genuine qualifiers land in table_prefix). + let col_index = view + .source() + .get_column_index(&column_name) + .or_else(|| { + if is_quoted { + return None; + } + // Unquoted name still carrying a qualifier (e.g. "t.col" from an + // older parse path) - after SELECT processing columns are + // unqualified, so fall back to the part after the last dot. + let dot_pos = column_name.rfind('.')?; let col_name = &column_name[dot_pos + 1..]; - - // After SELECT processing, columns are unqualified - // So just use the column name part debug!( "ORDER BY: Extracting unqualified column '{}' from '{}'", col_name, column_name ); view.source().get_column_index(col_name) - } else { - view.source().get_column_index(&column_name) - } - } else { - // Simple column name - view.source().get_column_index(&column_name) - } - .ok_or_else(|| { - // If not found, provide helpful error with suggestions - let suggestion = self.find_similar_column(view.source(), &column_name); - match suggestion { - Some(similar) => anyhow::anyhow!( - "Column '{}' not found. Did you mean '{}'?", - column_name, - similar - ), - None => { - // Also list available columns for debugging - let available_cols = view.source().column_names().join(", "); - anyhow::anyhow!( - "Column '{}' not found. Available columns: {}", + }) + .ok_or_else(|| { + // If not found, provide helpful error with suggestions + let suggestion = self.find_similar_column(view.source(), &column_name); + match suggestion { + Some(similar) => anyhow::anyhow!( + "Column '{}' not found. Did you mean '{}'?", column_name, - available_cols - ) + similar + ), + None => { + // Also list available columns for debugging + let available_cols = view.source().column_names().join(", "); + anyhow::anyhow!( + "Column '{}' not found. Available columns: {}", + column_name, + available_cols + ) + } } - } - })?; + })?; let ascending = matches!(order_col.direction, SortDirection::Asc); sort_columns.push((col_index, ascending)); diff --git a/tests/test_multi_column_order_by.rs b/tests/test_multi_column_order_by.rs index 4b87ff5e..3d1d22ac 100644 --- a/tests/test_multi_column_order_by.rs +++ b/tests/test_multi_column_order_by.rs @@ -353,3 +353,72 @@ fn test_direct_multi_sort_method() { assert_eq!(row3.values[1], DataValue::Integer(1)); assert_eq!(row3.values[2], DataValue::Float(2.0)); } + +/// P34: a double-quoted identifier containing a dot (e.g. `"name.common"`) is a +/// single column name, not a `table.column` qualifier. ORDER BY used to strip +/// everything before the last dot and then fail to find `common`. +#[test] +fn test_order_by_quoted_column_containing_dot() { + let mut table = DataTable::new("countries"); + table.add_column(DataColumn::new("name.common")); + table.add_column(DataColumn::new("region")); + + for (name, region) in [ + ("Peru", "Americas"), + ("Chad", "Africa"), + ("Angola", "Africa"), + ("Brazil", "Americas"), + ] { + table + .add_row(DataRow::new(vec![ + DataValue::String(name.to_string()), + DataValue::String(region.to_string()), + ])) + .unwrap(); + } + + let table_arc = Arc::new(table); + let engine = QueryEngine::new(); + + let view = engine + .execute( + table_arc.clone(), + r#"SELECT "name.common", region FROM countries ORDER BY region, "name.common""#, + ) + .unwrap(); + + assert_eq!(view.row_count(), 4); + let names: Vec = (0..4) + .map(|i| view.get_row(i).unwrap().values[0].to_string()) + .collect(); + assert_eq!(names, vec!["Angola", "Chad", "Brazil", "Peru"]); +} + +/// The qualifier-stripping fallback must still work for an unqualified name that +/// really does carry a table prefix. +#[test] +fn test_order_by_table_qualified_column_still_resolves() { + let mut table = DataTable::new("countries"); + table.add_column(DataColumn::new("region")); + + for region in ["Americas", "Africa", "Asia"] { + table + .add_row(DataRow::new(vec![DataValue::String(region.to_string())])) + .unwrap(); + } + + let table_arc = Arc::new(table); + let engine = QueryEngine::new(); + + let view = engine + .execute( + table_arc.clone(), + "SELECT region FROM countries ORDER BY countries.region", + ) + .unwrap(); + + let regions: Vec = (0..3) + .map(|i| view.get_row(i).unwrap().values[0].to_string()) + .collect(); + assert_eq!(regions, vec!["Africa", "Americas", "Asia"]); +}