From 5dee516b22b1e586623bc714e33287a3f8ca73bf Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Sat, 5 Sep 2026 18:33:54 +0100 Subject: [PATCH] fix(tui): quote column names when expanding SELECT * (T8) Ctrl+X (expand to all schema columns) and Alt+X (expand to visible columns) both did columns.join(", ") on the raw names. On data/countries.csv that produced SELECT name.common, name.official, tld, ..., idd.root, ... FROM countries which the parser reads as method calls on a `name` column. Every dotted name - name.*, idd.* and 60-odd translations.*.*, i.e. most of the file - came out unusable, and the next keystroke was to hand-quote 70 columns or undo. This is T1 in a different hat. Completion had already been taught to quote (quote_if_needed at nine sites in cursor_aware_parser.rs); expansion is the other producer of column text and never learned. Two producers, one of them right, is exactly the drift T1's "the parser owns semantics, the editor owns text" principle is meant to stop - it just had nowhere to put the rule. Introduce src/sql/identifier.rs, the single home for *does this name have to be quoted*. The rule mirrors Lexer::read_identifier, which is what actually decides whether a bare word survives: Unicode alphanumerics plus underscore, not starting with a digit. Keyword status comes from Token::from_keyword rather than a second hand-kept list, so a column called `row` or `end` is quoted for exactly as long as the lexer reserves those words. Three call sites now share it: csv_fixes::needs_quoting (used by all of completion) was a 9-way contains() chain that missed leading digits and keywords - delegates. formatter::needs_quotes had its own 40-word reserved list, hand-kept - delegates, and stops re-quoting text the parser already handed back quoted. Buffer::expand_asterisk{,_visible} did nothing at all - quotes. Left open, deliberately: formatter::needs_quotes is applied to SelectStatement::columns, the deprecated legacy field that can hold expression text, so the formatter still wraps COUNT(*) in quotes. That is a pre-existing bug about *what* it quotes rather than *when*, and it wants fixing where that field is retired. Tests: tests/asterisk_expansion.rs (5) covers both expansion paths, hidden columns, the rest of the query surviving intact, and names that collide with keywords; src/sql/identifier.rs has 8 unit tests for the rule itself. Verified end to end by running the full 76-column expansion of data/countries.csv. Full suite green, fmt clean, no new clippy warnings. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01WjekXbE7Y1Gu4H7Pr6T8is --- docs/TUI_FEATURES.md | 54 ++++++++++++++- src/buffer.rs | 22 +++++-- src/data/csv_fixes.rs | 20 ++---- src/sql/identifier.rs | 127 ++++++++++++++++++++++++++++++++++++ src/sql/mod.rs | 1 + src/sql/parser/formatter.rs | 33 ++-------- tests/asterisk_expansion.rs | 122 ++++++++++++++++++++++++++++++++++ tests/main.rs | 3 + 8 files changed, 337 insertions(+), 45 deletions(-) create mode 100644 src/sql/identifier.rs create mode 100644 tests/asterisk_expansion.rs diff --git a/docs/TUI_FEATURES.md b/docs/TUI_FEATURES.md index 57b353f..aa83ef2 100644 --- a/docs/TUI_FEATURES.md +++ b/docs/TUI_FEATURES.md @@ -59,7 +59,9 @@ columns it is completing. Between them the completer now has both primitives the remaining entries need: a byte span to splice over, and a typed schema. **Recommended order: T3 β†’ T4 β†’ T5**, with **T7** droppable anywhere β€” it is -independent of the others and mostly deletion. T3 is mechanical but wants doing +independent of the others and mostly deletion. **T8** is done: it was T1's bug +in the other producer of column text, and it left behind +`src/sql/identifier.rs` as the one place the quoting rule lives. T3 is mechanical but wants doing *before* T4, not as a retrofit. T4 is the first entry that consumes what T2 captured (`ColumnInfo::cardinality`, `TableInfo::row_count`); those numbers are already flowing and pinned by tests, so the gate can be designed against real @@ -287,3 +289,53 @@ Older, non-living notes that still contain usable thinking: first two rows. - **Not to be confused with T2's leftovers:** the completer's *type* decisions are already schema-driven. This entry is about the surrounding TUI. + +### T8 β€” `SELECT *` expansion emits column names it cannot read back +- **Status:** 🟒 DONE 2026-09-05 +- **Where:** `src/sql/identifier.rs` (new), `src/buffer.rs:1552,1609`, + `src/data/csv_fixes.rs`, `src/sql/parser/formatter.rs` +- **Observed:** Ctrl+X (expand to all schema columns) and Alt+X (expand to + visible columns) both did `columns.join(", ")` on the raw names. On + `data/countries.csv` that produced + + ``` + SELECT name.common, name.official, tld, ..., idd.root, ... FROM countries + ``` + + which the parser reads as method calls on a `name` column. Every dotted name + β€” `name.*`, `idd.*` and 60-odd `translations.*.*`, i.e. most of the file β€” + came out unusable, and the user's next keystroke was to hand-quote 70 columns + or undo. +- **Why it is the same bug as T1 in a different hat:** completion had already + been taught to quote (it calls `quote_if_needed` at nine sites in + `cursor_aware_parser.rs`). Expansion is the *other* producer of column text + and never learned. Two producers, one of them right, is the drift T1's + "the parser owns semantics, the editor owns text" principle is meant to stop + β€” it just did not have anywhere to put the rule. +- **Fixed by:** `src/sql/identifier.rs`, the single home for *does this name + have to be quoted*. The rule mirrors `Lexer::read_identifier`, which is what + actually decides whether a bare word survives: Unicode alphanumerics plus + `_`, not starting with a digit. Keyword status comes from + `Token::from_keyword` rather than a second hand-kept list, so a column called + `row` or `end` is quoted for exactly as long as the lexer reserves those + words. + + Three call sites now share it: + + | Site | Was | Now | + |---|---|---| + | `csv_fixes::needs_quoting` (used by all of completion) | a 9-way `contains()` chain β€” missed leading digits and keywords | delegates | + | `formatter::needs_quotes` | its own 40-word reserved list, hand-kept | delegates (and stops re-quoting text the parser already handed back quoted) | + | `Buffer::expand_asterisk{,_visible}` | nothing at all | quotes | + +- **Tests:** `tests/asterisk_expansion.rs` (5) covers both expansion paths, + hidden columns, the rest of the query surviving intact, and names that + collide with keywords. `src/sql/identifier.rs` has 8 unit tests for the rule + itself. Verified end to end by running the full 76-column expansion of + `data/countries.csv`. +- **Left open:** `formatter::needs_quotes` is applied to + `SelectStatement::columns`, which is the deprecated legacy field and can hold + expression text, so the formatter still wraps `COUNT(*)` in quotes. That is a + pre-existing formatter bug about *what* it quotes, not *when* β€” unchanged + here, and it wants fixing where the field is retired rather than in the + quoting rule. diff --git a/src/buffer.rs b/src/buffer.rs index 95f84d9..7b1c010 100644 --- a/src/buffer.rs +++ b/src/buffer.rs @@ -5,6 +5,7 @@ use crate::data::data_view::DataView; use crate::data::datatable::DataTable; use crate::hybrid_parser::HybridParser; use crate::input_manager::{create_from_input, create_single_line, InputManager}; +use crate::sql::identifier::quote_if_needed; use anyhow::Result; use crossterm::event::KeyEvent; use fuzzy_matcher::skim::SkimMatcherV2; @@ -1573,8 +1574,16 @@ impl Buffer { self.status_message = format!("No columns found for table '{table_name}'"); } else { - // Build the replacement with all columns - let columns_str = columns.join(", "); + // Build the replacement with all columns. Every + // name goes through the shared quoting rule - + // countries.csv's name.official and idd.root are + // unparseable bare, and expansion used to emit + // them that way. + let columns_str = columns + .iter() + .map(|col| quote_if_needed(col)) + .collect::>() + .join(", "); // Replace * with the column list let before_star = &query[..star_abs_pos]; @@ -1614,8 +1623,13 @@ impl Buffer { let visible_columns = dataview.get_display_column_names(); if !visible_columns.is_empty() { - // Build the replacement with visible columns only - let columns_str = visible_columns.join(", "); + // Build the replacement with visible columns only, + // quoted by the same rule as the full expansion. + let columns_str = visible_columns + .iter() + .map(|col| quote_if_needed(col)) + .collect::>() + .join(", "); // Replace * with the column list let before_star = &query[..star_abs_pos]; diff --git a/src/data/csv_fixes.rs b/src/data/csv_fixes.rs index 1bd89ab..be288d4 100644 --- a/src/data/csv_fixes.rs +++ b/src/data/csv_fixes.rs @@ -3,27 +3,19 @@ use std::collections::HashMap; /// Check if a column name needs quoting (contains spaces or special characters) +/// +/// Thin re-export of [`crate::sql::identifier::needs_quoting`] β€” the rule lives +/// there so completion, `SELECT *` expansion and the formatter cannot drift +/// apart on it. #[must_use] pub fn needs_quoting(column_name: &str) -> bool { - column_name.contains(' ') - || column_name.contains('-') - || column_name.contains('.') - || column_name.contains('(') - || column_name.contains(')') - || column_name.contains('[') - || column_name.contains(']') - || column_name.contains('"') - || column_name.contains('\'') + crate::sql::identifier::needs_quoting(column_name) } /// Quote a column name if necessary #[must_use] pub fn quote_if_needed(column_name: &str) -> String { - if needs_quoting(column_name) { - format!("\"{}\"", column_name.replace('"', "\"\"")) - } else { - column_name.to_string() - } + crate::sql::identifier::quote_if_needed(column_name) } /// Build a case-insensitive lookup map for column names diff --git a/src/sql/identifier.rs b/src/sql/identifier.rs new file mode 100644 index 0000000..039bd8c --- /dev/null +++ b/src/sql/identifier.rs @@ -0,0 +1,127 @@ +//! The single source of truth for "does this column name have to be quoted?". +//! +//! Anything that splices a column name back into query text β€” tab completion, +//! `SELECT *` expansion, the pretty-printer β€” has to agree on this, or the +//! user ends up with a query the parser cannot read. `data/countries.csv` is +//! the standing example: `name.official`, `idd.root` and 60-odd +//! `translations.*.common` columns are all unparseable bare. +//! +//! The rules mirror [`Lexer::read_identifier`](crate::sql::parser::lexer), which +//! is what actually decides whether a bare word survives: alphanumeric (Unicode +//! included, so `paΓ­s` is fine) plus `_`, not starting with a digit. Keyword +//! status comes from [`Token::from_keyword`] rather than a second hand-kept +//! list, so a column called `row` or `end` is quoted for as long as the lexer +//! reserves those words and no longer. + +use crate::sql::parser::lexer::Token; + +/// Check if an identifier must be quoted to survive a round trip through the +/// parser. +#[must_use] +pub fn needs_quoting(name: &str) -> bool { + if name.is_empty() { + return true; + } + + // A leading digit makes the lexer read a number, not an identifier. + if name.starts_with(|c: char| c.is_ascii_digit()) { + return true; + } + + // Anything the lexer would stop reading at: '.', ' ', '-', '(', '"', ... + if !name.chars().all(is_identifier_char) { + return true; + } + + // A bare keyword would tokenize as that keyword rather than a column. + Token::from_keyword(name).is_some() +} + +/// Quote an identifier unconditionally, doubling any embedded `"` the way SQL +/// expects (`Has"Quote` -> `"Has""Quote"`). +#[must_use] +pub fn quote_identifier(name: &str) -> String { + format!("\"{}\"", name.replace('"', "\"\"")) +} + +/// Quote an identifier, but only if it would not parse bare. +#[must_use] +pub fn quote_if_needed(name: &str) -> String { + if needs_quoting(name) { + quote_identifier(name) + } else { + name.to_string() + } +} + +/// The characters the lexer will accept inside an unquoted identifier. +fn is_identifier_char(ch: char) -> bool { + ch.is_alphanumeric() || ch == '_' +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn plain_identifiers_are_left_alone() { + for name in ["City", "customer_id", "tld", "cca2", "unMember", "x1"] { + assert!(!needs_quoting(name), "{name} should not need quoting"); + assert_eq!(quote_if_needed(name), name); + } + } + + #[test] + fn dotted_names_need_quoting() { + // The countries.csv case that started this. + assert!(needs_quoting("name.official")); + assert_eq!(quote_if_needed("name.common"), "\"name.common\""); + assert_eq!( + quote_if_needed("translations.ara.official"), + "\"translations.ara.official\"" + ); + } + + #[test] + fn punctuation_and_spaces_need_quoting() { + assert!(needs_quoting("Phone 1")); + assert!(needs_quoting("Customer-ID")); + assert!(needs_quoting("Price ($)")); + assert!(needs_quoting("a/b")); + assert!(needs_quoting("col[0]")); + } + + #[test] + fn leading_digit_needs_quoting() { + assert!(needs_quoting("2024")); + assert!(needs_quoting("1st_place")); + assert!(!needs_quoting("q1")); + } + + #[test] + fn keywords_need_quoting_case_insensitively() { + assert!(needs_quoting("order")); + assert!(needs_quoting("ORDER")); + assert!(needs_quoting("Row")); + assert!(needs_quoting("end")); + // Not reserved by this lexer, so leave it bare. + assert!(!needs_quoting("status")); + assert!(!needs_quoting("independent")); + } + + #[test] + fn unicode_identifiers_do_not_need_quoting() { + assert!(!needs_quoting("paΓ­s")); + assert!(!needs_quoting("naΓ―ve_count")); + } + + #[test] + fn embedded_quotes_are_doubled() { + assert_eq!(quote_if_needed("Has\"Quote"), "\"Has\"\"Quote\""); + } + + #[test] + fn empty_name_is_quoted_rather_than_emitted_bare() { + assert_eq!(quote_if_needed(""), "\"\""); + } +} diff --git a/src/sql/mod.rs b/src/sql/mod.rs index ebbcada..d38d316 100644 --- a/src/sql/mod.rs +++ b/src/sql/mod.rs @@ -11,6 +11,7 @@ pub mod cursor_aware_parser; pub mod functions; pub mod generators; pub mod hybrid_parser; +pub mod identifier; pub mod parser; pub mod query_rewriter; pub mod recursive_parser; diff --git a/src/sql/parser/formatter.rs b/src/sql/parser/formatter.rs index 7a5db3c..462003d 100644 --- a/src/sql/parser/formatter.rs +++ b/src/sql/parser/formatter.rs @@ -896,34 +896,15 @@ fn format_token(token: &Token) -> String { } } -// Check if a column name needs quotes (contains special characters or is a reserved word) +// Check if a column name needs quotes. The rule itself lives in +// crate::sql::identifier so the formatter, tab completion and SELECT * +// expansion cannot drift apart on it; the only thing the formatter adds is +// leaving text the parser already handed back quoted alone. fn needs_quotes(name: &str) -> bool { - // Check for special characters that require quoting - if name.contains('-') || name.contains(' ') || name.contains('.') || name.contains('/') { - return true; + if name.starts_with('"') { + return false; } - - // Check if it starts with a number - if name.chars().next().map_or(false, |c| c.is_ascii_digit()) { - return true; - } - - // Check if it's a SQL reserved word (common ones) - let reserved_words = [ - "SELECT", "FROM", "WHERE", "ORDER", "GROUP", "BY", "HAVING", "INSERT", "UPDATE", "DELETE", - "CREATE", "DROP", "ALTER", "TABLE", "INDEX", "VIEW", "AND", "OR", "NOT", "IN", "EXISTS", - "BETWEEN", "LIKE", "CASE", "WHEN", "THEN", "ELSE", "END", "JOIN", "LEFT", "RIGHT", "INNER", - "OUTER", "ON", "AS", "DISTINCT", "ALL", "TOP", "LIMIT", "OFFSET", "ASC", "DESC", - ]; - - let upper_name = name.to_uppercase(); - if reserved_words.contains(&upper_name.as_str()) { - return true; - } - - // Check if all characters are valid for unquoted identifiers - // Valid: letters, numbers, underscore (but not starting with number) - !name.chars().all(|c| c.is_ascii_alphanumeric() || c == '_') + crate::sql::identifier::needs_quoting(name) } // Format CASE expressions with proper indentation diff --git a/tests/asterisk_expansion.rs b/tests/asterisk_expansion.rs new file mode 100644 index 0000000..357f348 --- /dev/null +++ b/tests/asterisk_expansion.rs @@ -0,0 +1,122 @@ +//! `SELECT *` expansion (Ctrl+X / Alt+X) must emit column names the parser can +//! read back. +//! +//! `data/countries.csv` is the case that exposed this: `name.common`, +//! `idd.root` and 60-odd `translations.*.common` columns all have to be quoted. +//! Expansion used to join the raw names, so pressing Alt+X on that file +//! produced `SELECT name.common, name.official, ...` β€” a query the parser +//! reads as method calls on a `name` column. Tab completion already quoted +//! correctly (T1), which is exactly the drift this pins down: both go through +//! `sql::identifier::quote_if_needed` now. + +use sql_cli::buffer::{Buffer, BufferAPI}; +use sql_cli::data::data_view::DataView; +use sql_cli::data::datatable::{DataColumn, DataRow, DataTable, DataValue}; +use sql_cli::hybrid_parser::HybridParser; +use std::sync::Arc; + +/// The awkward end of countries.csv, plus a couple of names that are fine bare. +const COLUMNS: &[&str] = &[ + "name.common", + "name.official", + "tld", + "cca2", + "idd.root", + "unMember", +]; + +fn table() -> DataTable { + let mut table = DataTable::new("countries"); + for name in COLUMNS { + table.add_column(DataColumn::new(*name)); + } + table + .add_row(DataRow::new( + COLUMNS + .iter() + .map(|c| DataValue::String((*c).to_string())) + .collect(), + )) + .expect("row matches column count"); + table +} + +fn parser() -> HybridParser { + let mut parser = HybridParser::new(); + parser.update_single_table( + "countries".to_string(), + COLUMNS.iter().map(|c| (*c).to_string()).collect(), + ); + parser +} + +fn buffer_with_query(query: &str) -> Buffer { + let mut buffer = Buffer::new(1); + buffer.set_input_text(query.to_string()); + buffer.set_dataview(Some(DataView::new(Arc::new(table())))); + buffer +} + +const EXPANDED: &str = "\"name.common\", \"name.official\", tld, cca2, \"idd.root\", unMember"; + +#[test] +fn schema_expansion_quotes_dotted_names() { + let mut buffer = buffer_with_query("SELECT * FROM countries"); + assert!(buffer.expand_asterisk(&parser())); + assert_eq!( + buffer.get_input_text(), + format!("SELECT {EXPANDED} FROM countries") + ); +} + +#[test] +fn visible_expansion_quotes_dotted_names() { + let mut buffer = buffer_with_query("SELECT * FROM countries"); + assert!(buffer.expand_asterisk_visible()); + assert_eq!( + buffer.get_input_text(), + format!("SELECT {EXPANDED} FROM countries") + ); +} + +#[test] +fn visible_expansion_follows_hidden_columns() { + let mut buffer = buffer_with_query("SELECT * FROM countries"); + let mut view = DataView::new(Arc::new(table())); + view.hide_column_by_name("tld"); + view.hide_column_by_name("cca2"); + buffer.set_dataview(Some(view)); + + assert!(buffer.expand_asterisk_visible()); + assert_eq!( + buffer.get_input_text(), + "SELECT \"name.common\", \"name.official\", \"idd.root\", unMember FROM countries" + ); +} + +#[test] +fn expansion_leaves_the_rest_of_the_query_alone() { + let mut buffer = buffer_with_query("SELECT * FROM countries WHERE cca2 = 'GB' ORDER BY tld"); + assert!(buffer.expand_asterisk_visible()); + assert_eq!( + buffer.get_input_text(), + format!("SELECT {EXPANDED} FROM countries WHERE cca2 = 'GB' ORDER BY tld") + ); +} + +#[test] +fn expansion_quotes_names_that_collide_with_keywords() { + let mut table = DataTable::new("t"); + for name in ["order", "Row", "Price ($)", "1st", "region"] { + table.add_column(DataColumn::new(name)); + } + let mut buffer = Buffer::new(1); + buffer.set_input_text("SELECT * FROM t".to_string()); + buffer.set_dataview(Some(DataView::new(Arc::new(table)))); + + assert!(buffer.expand_asterisk_visible()); + assert_eq!( + buffer.get_input_text(), + "SELECT \"order\", \"Row\", \"Price ($)\", \"1st\", region FROM t" + ); +} diff --git a/tests/main.rs b/tests/main.rs index 954cfd2..0fc1ed6 100644 --- a/tests/main.rs +++ b/tests/main.rs @@ -225,3 +225,6 @@ mod projection_column_width_tests; #[path = "dotted_column_completion.rs"] mod dotted_column_completion; + +#[path = "asterisk_expansion.rs"] +mod asterisk_expansion;