From da6c96a6c1db3b6e5c25e8872b512207e277b548 Mon Sep 17 00:00:00 2001 From: Diederik Siderius Date: Fri, 11 Sep 2026 15:47:16 +0200 Subject: [PATCH] fix: by-name column access derives real names for joins and compounds (#709) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit output_column_names_joined (src/codegen/select/order_by.rs) generalizes the existing single-table "alias, else bare column name, else columnN" rule to a joined FROM: a bare column reference already names itself independent of its table qualifier (SELECT a.x carries name: "x" regardless of the "a." prefix), so the only real gap was */table.* expansion, which now draws from each joined table's own schema in FROM-clause order. A compound SELECT already had the right machinery internally (compile_select_compound resolves its own trailing ORDER BY against the leftmost arm's names) — it just wasn't reachable from header derivation, which is now unconditional instead of requiring select.compound.is_empty(). derive_headers (src/bin/sqlite-rs/repl.rs) wires both cases in; the CLI test that asserted the old column1/column2 fallback for a join (tests/unit/repl_dot_commands.rs::select_with_join_falls_back_to_positional_headers) is inverted to assert the real names instead. Known pre-existing gap, not introduced or widened here: an unaliased *computed* expression (`a + 1`) still falls back to columnN rather than the oracle's own expression-text rendering, in both the single-table and joined paths alike — this crate has no expression-to-SQL-text printer yet. Not covered by this ticket's acceptance criteria. Not done (api.rs is PR #705, unmerged, out of scope here): - Row::get_by_name/result_column_names in the embedding API itself — once #705 lands, its result_column_names should delegate to output_column_names/output_column_names_joined rather than reimplementing the rule, and tests/unit/api_statement_test.rs::joins_and_compounds_report_positional_column_names (named in this ticket's acceptance criteria, but not present on this branch) should be added/inverted the same way repl_dot_commands.rs's test was here. - Spec 013 Requirement 3's "scoped to single-table selects" sentence: left in place, since spec 013's text lives on the unmerged branch too and touching it here isn't safe — flagged for removal in the same PR that merges #705. spend: roughly matched the medium estimate. --- CHANGELOG.md | 9 + src/bin/sqlite-rs/repl.rs | 56 +++-- src/codegen.rs | 4 +- src/codegen/select.rs | 2 +- src/codegen/select/order_by.rs | 54 +++++ tests/corpus/main.rs | 1 + tests/corpus/result_column_names_test.rs | 253 +++++++++++++++++++++++ tests/unit/repl_dot_commands.rs | 11 +- 8 files changed, 370 insertions(+), 20 deletions(-) create mode 100644 tests/corpus/result_column_names_test.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index bfc46db9..f402570e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,15 @@ All notable changes to sqlite-rs. Format follows [Keep a Changelog](https://keep gives them, with no new opcode or synthesized rows (ADR-0046, supersedes ADR-0029's problem statement for this one table) (#707). +- `.headers on`'s column labels for a join or a compound `SELECT` + fell back to positional `column1`/`column2` placeholders instead of + deriving real names. A joined `FROM` now expands `*`/`table.*` + against each source table in order (`output_column_names_joined`), + and a compound takes its names from the leftmost arm — matching + what `.headers on` prints in stock `sqlite3` for a two/three-table + join, `UNION`/`UNION ALL`, a subquery in `FROM`, an aliased or + table-qualified reference, and duplicate names across a join (#709). + ## [0.18.10] - 2026-08-31 ### Fixed diff --git a/src/bin/sqlite-rs/repl.rs b/src/bin/sqlite-rs/repl.rs index b6fdcd7d..f3f2cd9c 100644 --- a/src/bin/sqlite-rs/repl.rs +++ b/src/bin/sqlite-rs/repl.rs @@ -47,7 +47,8 @@ use std::rc::Rc; use sqlite_rs::btree::TableCursor; use sqlite_rs::codegen::{ - compile_statement, leading_keywords, output_column_names, resolve_from_table_schema, + compile_statement, leading_keywords, output_column_names, output_column_names_joined, + resolve_from_table_schema, }; use sqlite_rs::dump; use sqlite_rs::parser::{ends_with_semicolon, parse_select, split_statements, ParseOutcome}; @@ -398,30 +399,55 @@ fn run_one_statement( } } -/// `.headers on`'s column labels for `select`'s result set: for a -/// single-table, non-compound `SELECT` this is +/// `.headers on`'s column labels for `select`'s result set: /// [`output_column_names`]'s "alias, else bare column name, else -/// `columnN`" rule against the resolved `FROM` table; anything the -/// codegen pipeline resolves less directly (no `FROM`, a join, or a -/// compound) falls back to positional `column1..columnN` labels — a -/// scope-cut noted in the issue's write-up rather than plumbing this -/// REPL's header derivation through the full join/compound resolver. +/// `columnN`" rule, applied against whichever table(s) `select`'s own +/// `FROM` clause resolves to — a single table, or (#709) +/// [`output_column_names_joined`] against every joined table in order +/// for `*`/`table.*` expansion. A compound `SELECT` takes its names +/// from the leftmost arm (`select` itself, never `select.compound`), +/// matching the oracle and the same rule `compile_select_compound` +/// already uses internally to resolve a compound's own trailing +/// `ORDER BY`. A `FROM`-less `select` (or one whose `FROM` doesn't +/// resolve — dead code today, `run_query`/the REPL would already have +/// rejected an unresolvable table before headers are ever derived) +/// falls back to positional `column1..columnN` labels. fn derive_headers( select: &sqlite_rs::parser::ast::Select, schemas: &[sqlite_rs::schema::TableSchema], ) -> Vec { - let single_table = select.compound.is_empty() - && select - .from - .as_ref() - .is_some_and(|from| from.joins.is_empty()); - if single_table { - if let Some(from) = &select.from { + if let Some(from) = &select.from { + if from.joins.is_empty() { if let Ok(schema) = resolve_from_table_schema(&from.first, schemas) { return output_column_names(select, &schema); } + } else if let Ok(tables) = joined_tables(from, schemas) { + return output_column_names_joined(select, &tables); } } let count = select.columns.len().max(1); (1..=count).map(|i| format!("column{i}")).collect() } + +/// Resolves every table in `from` (leftmost plus each `JOIN`) to its +/// `(alias, schema)` pair, in `FROM`-clause order — the shape +/// [`output_column_names_joined`] needs for `table.*` qualifier +/// matching. +fn joined_tables( + from: &sqlite_rs::parser::ast::FromClause, + schemas: &[sqlite_rs::schema::TableSchema], +) -> Result, sqlite_rs::schema::TableSchema)>, sqlite_rs::codegen::CodegenError> +{ + let mut out = Vec::with_capacity(from.joins.len().saturating_add(1)); + out.push(( + from.first.alias.clone(), + resolve_from_table_schema(&from.first, schemas)?, + )); + for join in &from.joins { + out.push(( + join.table.alias.clone(), + resolve_from_table_schema(&join.table, schemas)?, + )); + } + Ok(out) +} diff --git a/src/codegen.rs b/src/codegen.rs index e4807e48..97f4c2cb 100644 --- a/src/codegen.rs +++ b/src/codegen.rs @@ -26,8 +26,8 @@ pub use dispatch::{compile_statement, leading_keywords, DispatchError}; pub use pragma::compile_pragma; pub use select::{ compile_select, compile_select_compound, compile_select_joined, compile_select_with_catalog, - compile_select_with_catalog_and_stats, explain_query_plan, output_column_names, CodegenError, - EqpRow, + compile_select_with_catalog_and_stats, explain_query_plan, output_column_names, + output_column_names_joined, CodegenError, EqpRow, }; pub use stmt::delete::{compile_delete, compile_delete_with_catalog}; pub use stmt::insert::compile_insert; diff --git a/src/codegen/select.rs b/src/codegen/select.rs index ebb38b04..a512de2b 100644 --- a/src/codegen/select.rs +++ b/src/codegen/select.rs @@ -198,7 +198,7 @@ pub use entry::{ }; pub use eqp::{explain_query_plan, EqpRow}; pub use joins::compile_select_joined; -pub use order_by::output_column_names; +pub use order_by::{output_column_names, output_column_names_joined}; pub(crate) use aggregate::{ compile_grouped_scan, select_has_aggregate, try_compile_index_only_count, diff --git a/src/codegen/select/order_by.rs b/src/codegen/select/order_by.rs index 4807a734..f03ad562 100644 --- a/src/codegen/select/order_by.rs +++ b/src/codegen/select/order_by.rs @@ -109,6 +109,60 @@ pub fn output_column_names(select: &Select, schema: &TableSchema) -> Vec .collect() } +/// #709: like [`output_column_names`], but for a `FROM` clause with +/// joins, where a `*`/`table.*` expansion must draw columns from the +/// right per-table schema instead of a single one. `tables` is +/// `(alias, schema)` pairs in the same order the joined tables appear +/// in the `FROM` clause (leftmost first); `table.*`/`table.col` +/// qualifier matching uses alias-or-name, mirroring +/// `TableBinding::matches_qualifier`. +/// +/// A bare column reference's name doesn't depend on which table it +/// came from (`ExprKind::Column`'s `name` is already +/// qualifier-independent — `SELECT a.x` carries `name: "x"` same as an +/// unqualified `x`), so an ordinary `ResultColumn::Expr` needs no +/// per-table lookup at all; only `Star`/`TableStar` expansion does. +/// Same "alias, else bare column name, else `columnN`" fallback order +/// as the single-table rule, and the same known gap: an unaliased +/// *computed* expression (`a + 1`) falls back to positional `columnN` +/// here rather than the oracle's own expression-text rendering — no +/// expression-to-SQL-text printer exists in this crate yet, so that +/// part of SQLite's rule is unimplemented for both the single-table +/// and joined paths alike, not a regression introduced here. +pub fn output_column_names_joined( + select: &Select, + tables: &[(Option, TableSchema)], +) -> Vec { + let mut out = Vec::new(); + for col in &select.columns { + match col { + ResultColumn::Star => { + for (_, schema) in tables { + out.extend(schema.columns.iter().cloned()); + } + } + ResultColumn::TableStar { table } => { + if let Some((_, schema)) = tables.iter().find(|(alias, schema)| { + alias + .as_deref() + .map(|a| a.eq_ignore_ascii_case(table)) + .unwrap_or_else(|| schema.name.eq_ignore_ascii_case(table)) + }) { + out.extend(schema.columns.iter().cloned()); + } + } + ResultColumn::Expr { expr, alias } => { + let name = alias.clone().unwrap_or_else(|| match &expr.kind { + ExprKind::Column { name, .. } => name.clone(), + _ => format!("column{}", out.len().saturating_add(1)), + }); + out.push(name); + } + } + } + out +} + pub(super) fn order_by_entries(select: &Select, schema: &TableSchema) -> Vec { let mut out = Vec::new(); for col in &select.columns { diff --git a/tests/corpus/main.rs b/tests/corpus/main.rs index 9cb663e0..3191dc66 100644 --- a/tests/corpus/main.rs +++ b/tests/corpus/main.rs @@ -50,6 +50,7 @@ mod partial_sort_test; mod plan_parity_test; mod regen_test; mod repl_test; +mod result_column_names_test; mod schema_test; mod skip_scan_test; mod sql_corpus_test; diff --git a/tests/corpus/result_column_names_test.rs b/tests/corpus/result_column_names_test.rs new file mode 100644 index 00000000..4d480596 --- /dev/null +++ b/tests/corpus/result_column_names_test.rs @@ -0,0 +1,253 @@ +// Copyright 2026 Schuberg Philis +// SPDX-License-Identifier: Apache-2.0 +//! #709: `.headers on` column labels for a join or a compound +//! `SELECT` used to fall back to positional `column1`/`column2` +//! placeholders — `derive_headers` (`src/bin/sqlite-rs/repl.rs`) had +//! no join-aware naming, and never even tried a compound (despite +//! `output_column_names` already implementing the "leftmost arm" rule +//! internally, for a compound's own trailing `ORDER BY` resolution). +//! `output_column_names_joined` (`src/codegen/select/order_by.rs`) +//! closes the join gap; routing a compound through the existing +//! single-arm `output_column_names` closes the other. Oracle-diffed +//! against the pinned 3.53.4 `sqlite3 -header`. + +use crate::oracle::pinned_oracle; +use std::io::Write; +use std::path::{Path, PathBuf}; +use std::process::{Command, Stdio}; +use std::sync::atomic::{AtomicU64, Ordering}; + +const CLI: &str = env!("CARGO_BIN_EXE_sqlite-rs"); + +fn scratch_db(label: &str, oracle: &Path, setup_sql: &str) -> PathBuf { + static COUNTER: AtomicU64 = AtomicU64::new(0); + let n = COUNTER.fetch_add(1, Ordering::Relaxed); + let path = std::env::temp_dir().join(format!( + "sqlite_rs_result_column_names_test_{}_{n}_{label}.db", + std::process::id() + )); + std::fs::remove_file(&path).ok(); + let status = Command::new(oracle) + .arg(&path) + .arg(setup_sql) + .status() + .expect("creating scratch fixture db"); + assert!(status.success()); + path +} + +/// The oracle's own header row for `sql`, via one-shot `-header`. +fn oracle_header(oracle: &Path, db: &Path, sql: &str) -> String { + let output = Command::new(oracle) + .arg("-readonly") + .arg("-header") + .arg("-separator") + .arg("|") + .arg(db) + .arg(sql) + .output() + .expect("invoking sqlite3 oracle"); + assert!( + output.status.success(), + "oracle failed on {sql:?}: {}", + String::from_utf8_lossy(&output.stderr) + ); + String::from_utf8_lossy(&output.stdout) + .lines() + .next() + .unwrap_or_default() + .to_string() +} + +/// Our own header row for `sql`, via the REPL's `.headers on`/list +/// mode. The REPL echoes a bare `sqlite> ` prompt per stdin line read +/// before any of that line's own output, with no intervening newline — +/// stripped out here so the remaining first non-blank line is the +/// header row itself. +fn our_header(db: &Path, sql: &str) -> String { + let script = format!(".headers on\n.mode list\n{sql};\n.quit\n"); + let mut child = Command::new(CLI) + .arg("repl") + .arg(db) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .expect("spawning sqlite-rs repl"); + child + .stdin + .take() + .unwrap() + .write_all(script.as_bytes()) + .unwrap(); + let output = child.wait_with_output().expect("waiting on repl"); + assert!( + output.status.success(), + "repl failed on {sql:?}: {}", + String::from_utf8_lossy(&output.stderr) + ); + let stdout = String::from_utf8_lossy(&output.stdout).replace("sqlite> ", ""); + stdout + .lines() + .find(|line| !line.trim().is_empty()) + .unwrap_or_default() + .to_string() +} + +fn assert_header_matches_oracle(oracle: &Path, db: &Path, sql: &str) { + assert_eq!( + our_header(db, sql), + oracle_header(oracle, db, sql), + "{sql:?}" + ); +} + +/// The core defect: a two-table join used to report +/// `column1|column2`. +#[test] +fn two_table_join_reports_real_column_names() { + let Some(oracle) = pinned_oracle() else { + eprintln!("skipping: no pinned 3.53.4 sqlite3 oracle on this machine"); + return; + }; + let db = scratch_db( + "join2", + &oracle, + "CREATE TABLE a(x INTEGER, y TEXT); CREATE TABLE b(x INTEGER, z TEXT); \ + INSERT INTO a VALUES (1, 'p'); INSERT INTO b VALUES (1, 'q');", + ); + assert_header_matches_oracle(&oracle, &db, "SELECT a.x, b.z FROM a JOIN b ON a.x = b.x"); +} + +/// A three-table join. +#[test] +fn three_table_join_reports_real_column_names() { + let Some(oracle) = pinned_oracle() else { + eprintln!("skipping: no pinned 3.53.4 sqlite3 oracle on this machine"); + return; + }; + let db = scratch_db( + "join3", + &oracle, + "CREATE TABLE a(x INTEGER); CREATE TABLE b(x INTEGER, y TEXT); \ + CREATE TABLE c(x INTEGER, z TEXT); INSERT INTO a VALUES (1); \ + INSERT INTO b VALUES (1, 'p'); INSERT INTO c VALUES (1, 'q');", + ); + assert_header_matches_oracle( + &oracle, + &db, + "SELECT a.x, b.y, c.z FROM a JOIN b ON a.x = b.x JOIN c ON a.x = c.x", + ); +} + +/// `UNION` takes its names from the leftmost arm. +#[test] +fn union_reports_leftmost_arm_names() { + let Some(oracle) = pinned_oracle() else { + eprintln!("skipping: no pinned 3.53.4 sqlite3 oracle on this machine"); + return; + }; + let db = scratch_db( + "union", + &oracle, + "CREATE TABLE a(x INTEGER); CREATE TABLE b(y INTEGER); \ + INSERT INTO a VALUES (1); INSERT INTO b VALUES (2);", + ); + assert_header_matches_oracle(&oracle, &db, "SELECT x FROM a UNION SELECT y FROM b"); +} + +/// `UNION ALL` likewise. +#[test] +fn union_all_reports_leftmost_arm_names() { + let Some(oracle) = pinned_oracle() else { + eprintln!("skipping: no pinned 3.53.4 sqlite3 oracle on this machine"); + return; + }; + let db = scratch_db( + "union_all", + &oracle, + "CREATE TABLE a(x INTEGER); CREATE TABLE b(y INTEGER); \ + INSERT INTO a VALUES (1); INSERT INTO b VALUES (2);", + ); + assert_header_matches_oracle(&oracle, &db, "SELECT x FROM a UNION ALL SELECT y FROM b"); +} + +/// A subquery in `FROM`. +#[test] +fn subquery_in_from_reports_real_column_names() { + let Some(oracle) = pinned_oracle() else { + eprintln!("skipping: no pinned 3.53.4 sqlite3 oracle on this machine"); + return; + }; + let db = scratch_db( + "subquery", + &oracle, + "CREATE TABLE a(x INTEGER, y TEXT); INSERT INTO a VALUES (1, 'p');", + ); + assert_header_matches_oracle(&oracle, &db, "SELECT s.x FROM (SELECT x, y FROM a) s"); +} + +/// An alias wins over a derived name. +#[test] +fn alias_wins_over_derived_name() { + let Some(oracle) = pinned_oracle() else { + eprintln!("skipping: no pinned 3.53.4 sqlite3 oracle on this machine"); + return; + }; + let db = scratch_db( + "alias", + &oracle, + "CREATE TABLE a(x INTEGER); INSERT INTO a VALUES (1);", + ); + assert_header_matches_oracle(&oracle, &db, "SELECT a.x AS renamed FROM a"); +} + +/// `SELECT a.x` reports `x`, not `a.x`. +#[test] +fn qualified_reference_reports_bare_column_name() { + let Some(oracle) = pinned_oracle() else { + eprintln!("skipping: no pinned 3.53.4 sqlite3 oracle on this machine"); + return; + }; + let db = scratch_db( + "qualified", + &oracle, + "CREATE TABLE a(x INTEGER); INSERT INTO a VALUES (1);", + ); + assert_header_matches_oracle(&oracle, &db, "SELECT a.x FROM a"); +} + +/// `*` expansion across a join draws each table's own columns. +#[test] +fn star_expansion_across_join_matches_oracle() { + let Some(oracle) = pinned_oracle() else { + eprintln!("skipping: no pinned 3.53.4 sqlite3 oracle on this machine"); + return; + }; + let db = scratch_db( + "star_join", + &oracle, + "CREATE TABLE a(x INTEGER, y TEXT); CREATE TABLE b(x INTEGER, z TEXT); \ + INSERT INTO a VALUES (1, 'p'); INSERT INTO b VALUES (1, 'q');", + ); + assert_header_matches_oracle(&oracle, &db, "SELECT * FROM a JOIN b ON a.x = b.x"); +} + +/// Duplicate names are legal: `SELECT a.x, b.x` yields two columns +/// both named `x`. +#[test] +fn duplicate_names_across_a_join_are_legal() { + let Some(oracle) = pinned_oracle() else { + eprintln!("skipping: no pinned 3.53.4 sqlite3 oracle on this machine"); + return; + }; + let db = scratch_db( + "dup", + &oracle, + "CREATE TABLE a(x INTEGER); CREATE TABLE b(x INTEGER); \ + INSERT INTO a VALUES (1); INSERT INTO b VALUES (1);", + ); + let header = our_header(&db, "SELECT a.x, b.x FROM a JOIN b ON a.x = b.x"); + assert_eq!(header, "x|x"); + assert_header_matches_oracle(&oracle, &db, "SELECT a.x, b.x FROM a JOIN b ON a.x = b.x"); +} diff --git a/tests/unit/repl_dot_commands.rs b/tests/unit/repl_dot_commands.rs index 630cc0b8..fe7ca950 100644 --- a/tests/unit/repl_dot_commands.rs +++ b/tests/unit/repl_dot_commands.rs @@ -348,8 +348,14 @@ fn crlf_line_endings_are_trimmed_like_bare_newlines() { assert!(out.contains('1'), "{out}"); } +/// #709: a join used to fall back to positional `column1|column2` +/// headers — `derive_headers` had no join-aware naming at all. +/// `output_column_names_joined` now derives real names the same way a +/// single-table `SELECT` always could: a bare column reference names +/// itself regardless of which joined table it came from (`t.a` names +/// `a`, matching the oracle). #[test] -fn select_with_join_falls_back_to_positional_headers() { +fn select_with_join_reports_real_column_names() { let db = scratch_db("join-headers"); seed(&db, "CREATE TABLE t(a)"); seed(&db, "CREATE TABLE u(b)"); @@ -360,5 +366,6 @@ fn select_with_join_falls_back_to_positional_headers() { &db, ".headers on\nSELECT t.a, u.b FROM t JOIN u ON 1;\n.quit\n", ); - assert!(out.contains("column1|column2"), "{out}"); + assert!(out.contains("a|b"), "{out}"); + assert!(!out.contains("column1|column2"), "{out}"); }