Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 40 additions & 13 deletions docs/SQL_PARITY.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 <expr>` 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,
Expand Down Expand Up @@ -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`.
Expand All @@ -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`
Expand Down Expand Up @@ -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
Expand Down
15 changes: 9 additions & 6 deletions src/data/arithmetic_evaluator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
165 changes: 165 additions & 0 deletions src/data/column_resolution_error.rs
Original file line number Diff line number Diff line change
@@ -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<String> = (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}");
}
}
1 change: 1 addition & 0 deletions src/data/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
39 changes: 22 additions & 17 deletions src/data/query_engine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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
Expand Down
Loading