diff --git a/docs/ENGINE_REFACTORING.md b/docs/ENGINE_REFACTORING.md index 37dae73..36ba6fa 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 83ae8e2..8e4113e 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 4327eef..e5c9403 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 4b87ff5..3d1d22a 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"]); +}