From c9d31d8bd53dd0605211ab02d7608245684ea9b9 Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Sun, 6 Sep 2026 19:11:23 +0100 Subject: [PATCH] fix(errors): say which failure a qualified column reference hit (P40 piece 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "Column 'a.path' not found. Table 'a' may not support qualified column names" was the fallback for every unresolvable qualified reference, written out at three call sites. Its central claim is essentially never true — qualified names work — so it sends the reader after a feature limitation instead of at their query. It has now misdirected two separate investigations. It opened P40 ("maybe table doesn't support qualified columns"), and on 2026-09-06 it cost a second afternoon on a query with no generator in it at all: WITH a AS (SELECT 'hello_world' AS path) SELECT SPLIT_PART(a.path, '_', 1) -- no FROM a That is an out-of-scope reference and nothing more; DuckDB rejects it with "Referenced table \"a\" not found!". Adding FROM a returns 'hello'. Two genuinely different failures were sharing one message, so split them: prefix not in scope -> Unknown table or alias 'a' in 'a.path'. The query selects from 'DUAL'. A CTE has to be named in a FROM clause before its columns can be referenced. column missing -> Column 'nope' not found in 'a'. Available columns: path, id The worst of the three sites was the SELECT-list one, which branched on whether any column carried a qualified_name and, if none did, blamed qualification. For a single table or a CTE no column is qualified, so the heuristic fired on the common case and was wrong every time. The in-scope test is deliberately generous — table name, resolved alias, or any column's qualified-name prefix. A false "in scope" costs only a less pointed message; a false "unknown table" would reintroduce exactly the confident wrong explanation this removes. Message logic now lives in one module rather than three copies, the R9/R11 lesson applied before it becomes a finding. Five unit tests, one asserting the old wording cannot come back. Parity contract holds at 156 AGREE / 181 cases; full suite green. Pieces 2 (resolve generator args against the real source table) and 3 (row-wise explode) of P40 are untouched. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JBEUeckCwmWXoWTpQUTDqP --- docs/SQL_PARITY.md | 53 ++++++--- src/data/arithmetic_evaluator.rs | 15 ++- src/data/column_resolution_error.rs | 165 ++++++++++++++++++++++++++++ src/data/mod.rs | 1 + src/data/query_engine.rs | 39 ++++--- 5 files changed, 237 insertions(+), 36 deletions(-) create mode 100644 src/data/column_resolution_error.rs diff --git a/docs/SQL_PARITY.md b/docs/SQL_PARITY.md index db819ca..0b479ac 100644 --- a/docs/SQL_PARITY.md +++ b/docs/SQL_PARITY.md @@ -96,7 +96,7 @@ Suggested fix order, by silent blast radius: | 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) | | 11 | [P35](#p35), [P36](#p36), [P43](#p43) | Not parity obligations — a DuckDB extension, a naming difference, and a `RANGE` endpoint convention. Decide *whether*, not just when. P43 is the one with a migration cost attached, so it wants deciding before it accumulates more callers | -| 12 | [P40](#p40) a generator's args can't reference columns | Hard error and loudly signposted, so last by blast radius — but it splits: the misleading *"may not support qualified column names"* message is a few lines and is the part that wastes the next person's afternoon. Take that alone; the explode feature can wait on the `UNNEST` decision | +| 12 | [P40](#p40) a generator's args can't reference columns | Hard error and loudly signposted, so last by blast radius. **Piece 1 (the message) shipped 2026-09-06** after it misdirected a second investigation; pieces 2 (resolve args against the real table) and 3 (row-wise explode) remain, and the explode feature still waits on the `UNNEST` decision | | — | [P27](#p27) `OR` in `JOIN ... ON` | **Reclassified 2026-08-08, re-scoped 2026-09-04 — possibly smaller than it was filed as.** The AST is still the blocker (`JoinCondition` is a `Vec` of AND-ed conditions with nowhere to put an `OR`), but the executor already evaluates expressions per row pair and already has a merged-row `cross_join`, so `INNER JOIN ON ` may lower to cross-join + the R10 WHERE evaluator. Do the timeboxed scoping pass in the entry before sequencing this | P27–P30 jump the queue because all four were found *by* fixing something else, @@ -1683,7 +1683,8 @@ out of date. --- ### P40 — A table generator's arguments cannot reference columns, so there is no way to explode one row into many -- **Status:** 🔴 OPEN — hard error, but the error text misdirects (see below) +- **Status:** 🔴 OPEN — hard error. **Piece 1 of 3 (the misleading message) is + done, 2026-09-06**; pieces 2 and 3 remain - **Corpus:** none yet — see *Pinning it*. - **Observed:** `SPLIT` is a **table generator** (`sql/generators/string_generators.rs:12`), in the same family as `READ_JSON` / `RANGE`, so it can only appear in `FROM`. @@ -1697,15 +1698,38 @@ out of date. | `WITH a AS (…) SELECT * FROM split(a.path,'/')` | 🚫 *Column 'a.path' not found. Table 'a' may not support qualified column names* | | `SELECT * FROM split((SELECT p FROM …),'/')` | 🚫 *Unsupported expression type for arithmetic evaluation: ScalarSubquery* | -- **The error message is actively misleading, and it misdirected the report - that opened this entry.** "Table 'a' may not support qualified column names" - is the generic fallback at `query_engine.rs:132`. Read literally it points at - CTE scoping and qualified-name resolution — and that is exactly the conclusion - it produced ("maybe table doesn't support qualified columns"). Neither is - involved. The unqualified - form fails identically, and so does a plain base-table column with no CTE in - sight. Whatever else is decided here, **this message must stop blaming - qualification** — a generator argument that is not a constant should say so. +- **The error message was actively misleading, and it misdirected the report + that opened this entry — 🟢 FIXED 2026-09-06.** "Table 'a' may not support + qualified column names" was the generic fallback, written out at *three* call + sites. Read literally it points at CTE scoping and qualified-name resolution — + and that is exactly the conclusion it produced ("maybe table doesn't support + qualified columns"). Neither is involved. + + **It then cost a second afternoon, 2026-09-06**, on a query that had nothing to + do with generators: `WITH a AS (…) SELECT SPLIT_PART(a.path,'_',1)` with no + `FROM a`. That is simply an out-of-scope reference — DuckDB says *Referenced + table "a" not found!* — but the message sent the reader looking at qualified-name + support again. Two people-hours to one sentence of wrong explanation is the + argument for taking messages seriously as a class. + + The worst of the three sites was the SELECT-list one: it branched on whether + *any* column carried a `qualified_name` and, if none did, blamed qualification. + For a single table or a CTE no column is qualified, so the heuristic fired on + the common case and was wrong every time. + + Now one implementation, `data::column_resolution_error`, which separates the + two failures that were sharing a message: + + | Situation | Message | + |---|---| + | prefix names nothing in scope | *Unknown table or alias 'a' in 'a.path'. The query selects from 'DUAL'. A CTE has to be named in a FROM clause before its columns can be referenced.* | + | prefix in scope, no such column | *Column 'nope' not found in 'a'. Available columns: path, id* | + + The in-scope test is deliberately generous (table name, resolved alias, or any + column's qualified-name prefix): a false "in scope" costs only a less pointed + message, whereas a false "unknown table" would reintroduce the confident wrong + explanation this replaced. Five unit tests, one of which asserts the old + wording cannot return. - **Root cause — two defects stacked, and only the second is a feature request:** 1. **The args are evaluated against DUAL.** `statement_executor.rs:124-148` @@ -1748,8 +1772,11 @@ out of date. argument fails. - **Decision: fix, in three independently shippable pieces**, smallest first: - 1. **The message.** No semantics, no risk, and it is the part that wastes - other people's time. Do this even if the rest is deferred. + 1. ~~**The message.**~~ ✅ **Done 2026-09-06.** No semantics, no risk, and it + was the part that wasted other people's time — twice, as it turned out. + Note what it does *not* do: a generator argument that is not a constant + still reports "unknown table or alias", which is accurate for the reported + shape but will read oddly once piece 2 lands. Revisit the wording then. 2. **Resolve generator arguments against the real source table and CTE context** instead of DUAL. Makes single-row and constant-expression arguments honest, and makes the remaining failure an accurate "this needs a diff --git a/src/data/arithmetic_evaluator.rs b/src/data/arithmetic_evaluator.rs index 18ffabf..4f5de84 100644 --- a/src/data/arithmetic_evaluator.rs +++ b/src/data/arithmetic_evaluator.rs @@ -269,12 +269,15 @@ impl<'a> ArithmeticEvaluator<'a> { .map(|v| v.clone()); } - // If not found, return error - Err(anyhow!( - "Column '{}' not found. Table '{}' may not support qualified column names", - qualified_name, - actual_table - )) + // If not found, say which of the two failures this is. + Err( + crate::data::column_resolution_error::qualified_column_not_found( + self.table, + table_prefix, + actual_table, + &column_ref.name, + ), + ) } else { // Simple column name lookup self.evaluate_column(&column_ref.name, row_index) diff --git a/src/data/column_resolution_error.rs b/src/data/column_resolution_error.rs new file mode 100644 index 0000000..70f4b15 --- /dev/null +++ b/src/data/column_resolution_error.rs @@ -0,0 +1,165 @@ +//! The error message produced when a qualified column reference (`a.path`) +//! cannot be resolved. +//! +//! It lives in one place because it was previously written out at three call +//! sites, all saying the same misleading thing: *"Table 'a' may not support +//! qualified column names"*. That explanation is essentially never true — +//! qualified names work — and it sends the reader looking for a feature +//! limitation instead of at their query. It cost real debugging time twice +//! from field use. See T9 in `docs/TUI_FEATURES.md` and P40 in +//! `docs/SQL_PARITY.md`. +//! +//! There are two genuinely different failures behind one message, and the +//! reader needs to be told which one they hit: +//! +//! 1. the prefix names nothing in scope — usually a CTE that was defined but +//! never put in a `FROM` clause; +//! 2. the prefix is in scope but has no such column — usually a typo. + +use crate::data::datatable::DataTable; +use anyhow::anyhow; + +/// How many column names to list before truncating. +const MAX_LISTED_COLUMNS: usize = 12; + +/// Build the error for an unresolvable `prefix.column` reference. +/// +/// `prefix` is what the user wrote; `resolved` is that prefix after alias +/// resolution (pass the same value when the caller has no alias map). +#[must_use] +pub fn qualified_column_not_found( + table: &DataTable, + prefix: &str, + resolved: &str, + column: &str, +) -> anyhow::Error { + if prefix_is_in_scope(table, prefix, resolved) { + anyhow!( + "Column '{}' not found in '{}'. {}", + column, + prefix, + available_columns(table) + ) + } else { + anyhow!( + "Unknown table or alias '{}' in '{}.{}'. {} \ + A CTE has to be named in a FROM clause before its columns can be referenced.", + prefix, + prefix, + column, + tables_in_scope(table) + ) + } +} + +/// Whether `prefix` plausibly names the table being queried. +/// +/// Deliberately generous — a false "in scope" only costs a slightly less +/// pointed message, whereas a false "unknown table" would reintroduce exactly +/// the kind of confident, wrong explanation this module exists to remove. +fn prefix_is_in_scope(table: &DataTable, prefix: &str, resolved: &str) -> bool { + if table.name.eq_ignore_ascii_case(prefix) || table.name.eq_ignore_ascii_case(resolved) { + return true; + } + + // A JOIN or CTE result carries qualified names like `orders.id`; if any + // column is qualified with this prefix, the prefix is certainly in scope. + table.columns.iter().any(|c| { + c.qualified_name.as_deref().is_some_and(|q| { + q.split_once('.').is_some_and(|(t, _)| { + t.eq_ignore_ascii_case(prefix) || t.eq_ignore_ascii_case(resolved) + }) + }) + }) +} + +fn tables_in_scope(table: &DataTable) -> String { + if table.name.is_empty() { + "The query selects from no named table.".to_string() + } else { + format!("The query selects from '{}'.", table.name) + } +} + +fn available_columns(table: &DataTable) -> String { + let names: Vec<&str> = table.columns.iter().map(|c| c.name.as_str()).collect(); + if names.is_empty() { + return "It has no columns.".to_string(); + } + + let shown = names.len().min(MAX_LISTED_COLUMNS); + let mut msg = format!("Available columns: {}", names[..shown].join(", ")); + if names.len() > shown { + msg.push_str(&format!(" (+{} more)", names.len() - shown)); + } + msg +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::data::datatable::{DataColumn, DataTable}; + + fn table_named(name: &str, columns: &[&str]) -> DataTable { + let mut t = DataTable::new(name); + for c in columns { + t.add_column(DataColumn::new(*c)); + } + t + } + + #[test] + fn an_out_of_scope_prefix_names_the_prefix_not_a_missing_feature() { + // The reported case: `WITH a AS (...) SELECT a.path` with no FROM. + let t = table_named("", &[]); + let msg = qualified_column_not_found(&t, "a", "a", "path").to_string(); + + assert!(msg.contains("Unknown table or alias 'a'"), "{msg}"); + assert!(msg.contains("FROM"), "{msg}"); + // The old message's central claim must not come back. + assert!(!msg.contains("qualified column names"), "{msg}"); + } + + #[test] + fn a_prefix_matching_the_table_reports_the_column_and_lists_alternatives() { + let t = table_named("a", &["path", "id"]); + let msg = qualified_column_not_found(&t, "a", "a", "nope").to_string(); + + assert!(msg.contains("Column 'nope' not found in 'a'"), "{msg}"); + assert!(msg.contains("path"), "{msg}"); + assert!(msg.contains("id"), "{msg}"); + assert!(!msg.contains("Unknown table"), "{msg}"); + } + + #[test] + fn an_alias_resolving_to_the_table_counts_as_in_scope() { + // `FROM orders o` — the user wrote `o`, which resolves to `orders`. + let t = table_named("orders", &["id"]); + let msg = qualified_column_not_found(&t, "o", "orders", "nope").to_string(); + + assert!(msg.contains("Column 'nope' not found in 'o'"), "{msg}"); + assert!(!msg.contains("Unknown table"), "{msg}"); + } + + #[test] + fn a_qualified_column_name_puts_its_prefix_in_scope() { + // JOIN results carry `orders.id`, and the table itself is named + // something else entirely. + let mut t = table_named("join_result", &["id"]); + t.columns[0].qualified_name = Some("orders.id".to_string()); + let msg = qualified_column_not_found(&t, "orders", "orders", "nope").to_string(); + + assert!(msg.contains("Column 'nope' not found in 'orders'"), "{msg}"); + assert!(!msg.contains("Unknown table"), "{msg}"); + } + + #[test] + fn a_long_column_list_is_truncated() { + let names: Vec = (0..30).map(|i| format!("c{i}")).collect(); + let refs: Vec<&str> = names.iter().map(String::as_str).collect(); + let t = table_named("wide", &refs); + let msg = qualified_column_not_found(&t, "wide", "wide", "nope").to_string(); + + assert!(msg.contains("(+18 more)"), "{msg}"); + } +} diff --git a/src/data/mod.rs b/src/data/mod.rs index dd7afca..d180601 100644 --- a/src/data/mod.rs +++ b/src/data/mod.rs @@ -35,6 +35,7 @@ pub mod stream_loader; // Query execution pub mod arithmetic_evaluator; pub mod batch_window_evaluator; // Batch evaluation for window functions +pub mod column_resolution_error; // One home for the 'a.path not found' message pub mod evaluation_context; pub mod group_by_expressions; pub mod hash_join; diff --git a/src/data/query_engine.rs b/src/data/query_engine.rs index 63aca2b..5f8f2f9 100644 --- a/src/data/query_engine.rs +++ b/src/data/query_engine.rs @@ -128,11 +128,14 @@ impl ExecutionContext { } // Not found with either qualified or unqualified name - Err(anyhow!( - "Column '{}' not found. Table '{}' may not support qualified column names", - qualified_name, - actual_table - )) + Err( + crate::data::column_resolution_error::qualified_column_not_found( + table, + table_prefix, + &actual_table, + &column_ref.name, + ), + ) } else { // Unqualified column reference if let Some(idx) = table.get_column_index(&column_ref.name) { @@ -3010,24 +3013,26 @@ impl QueryEngine { // only lookup would otherwise fail. See // `ExecutionContext::resolve_column_index` for the same logic. let qualified_name = format!("{}.{}", table_prefix, col_ref.name); - table.find_column_by_qualified_name(&qualified_name) + table + .find_column_by_qualified_name(&qualified_name) .or_else(|| { table_columns .iter() .position(|c| c.eq_ignore_ascii_case(&col_ref.name)) }) .ok_or_else(|| { - // Check if any columns have qualified names for better error message - let has_qualified = table.columns.iter() - .any(|c| c.qualified_name.is_some()); - if !has_qualified { - anyhow::anyhow!( - "Column '{}' not found. Note: Table '{}' may not support qualified column names", - qualified_name, table_prefix - ) - } else { - anyhow::anyhow!("Column '{}' not found", qualified_name) - } + // The old message here branched on whether ANY + // column carried a qualified name and, if not, + // blamed qualified-name support. For a single + // table or a CTE no column is qualified, so + // that fired on the common case and was wrong + // every time. + crate::data::column_resolution_error::qualified_column_not_found( + table, + table_prefix, + table_prefix, + &col_ref.name, + ) })? } else { // Simple column name lookup