fix(tui): quote column names when expanding SELECT * (T8)

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.

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::<Vec<_>>()
+                                .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::<Vec<_>>()
+                            .join(", ");
 
                         // Replace * with the column list
                         let before_star = &query[..star_abs_pos];

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

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(""), "\"\"");
+    }
+}

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;

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