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