fix(tui): quote column names when expanding SELECT * (T8) - #66
Merged
Conversation
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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WjekXbE7Y1Gu4H7Pr6T8is
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
namecolumn. 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
roworendis 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.
Claude-Session: https://claude.ai/code/session_01WjekXbE7Y1Gu4H7Pr6T8is