Fix should allow trailing comma in SELECT list before clause keyword#268
Merged
Merged
Conversation
ClickHouse accepts `SELECT a, FROM t` (e.g. when a column is commented out with `--`). Break the SELECT-item loop after consuming a comma if the next token is a clause-starting keyword (FROM, WHERE, GROUP, etc.). Fixes incorrect AST for existing CREATE MATERIALIZED VIEW fixtures where FROM was previously consumed as a bare select expression.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
There was a problem hiding this comment.
Pull request overview
This PR updates the ClickHouse SQL parser to accept a trailing comma at the end of a SELECT item list when the next token begins the next clause (notably FROM), matching ClickHouse behavior and fixing mis-parses where FROM <table> was previously consumed as a select expression plus alias.
Changes:
- Allow
SELECT <expr>, FROM <table>by stoppingparseSelectItems()after a comma when the next token is a clause-start keyword. - Add a query fixture and expected AST/format/beautify outputs covering the trailing-comma-before-
FROMcase. - Update two
CREATE MATERIALIZED VIEW ... AS SELECT ...fixtures whose expected outputs changed now thatFROMis correctly parsed as theFROMclause rather than a select expression.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| parser/parser_column.go | Stops parsing additional select items after a comma if the next token is a select-list terminator keyword (enables trailing comma before clause keywords). |
| parser/testdata/query/select_trailing_comma_before_from.sql | Adds a regression query fixture demonstrating SELECT count(x), --comment \n FROM .... |
| parser/testdata/query/output/select_trailing_comma_before_from.sql.golden.json | Adds expected AST output for the new query fixture. |
| parser/testdata/query/format/select_trailing_comma_before_from.sql | Adds expected formatted output (trailing comma removed). |
| parser/testdata/query/format/beautify/select_trailing_comma_before_from.sql | Adds expected beautified output for the new query. |
| parser/testdata/ddl/output/create_materialized_view_with_refresh.sql.golden.json | Updates expected AST so FROM table_v5 is parsed as a From clause (not a select expression). |
| parser/testdata/ddl/output/create_materialized_view_with_definer.sql.golden.json | Same as above for the definer MV fixture. |
| parser/testdata/ddl/format/create_materialized_view_with_refresh.sql | Updates expected formatted SQL for the MV fixture to remove the erroneous , FROM AS .... |
| parser/testdata/ddl/format/create_materialized_view_with_definer.sql | Same as above for the definer MV fixture. |
| parser/testdata/ddl/format/beautify/create_materialized_view_with_refresh.sql | Updates expected beautified SQL to reflect correct FROM clause formatting. |
| parser/testdata/ddl/format/beautify/create_materialized_view_with_definer.sql | Same as above for the definer MV fixture. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
git-hulk
approved these changes
May 11, 2026
git-hulk
changed the title
git-hulk
changed the title
3 tasks
erezrokah
pushed a commit
to erezrokah/clickhouse-sql-parser
that referenced
this pull request
May 11, 2026
…comma-before-keyword-table handling Address review feedback: previous lookahead applied to all clause-starter keywords, which regressed parsing of `SELECT a, FROM `limit`` (trailing comma before FROM where the next ident is itself a clause keyword). Restrict the special-case to the four keywords that legitimately appear as bare column names. FROM/WHERE/GROUP/ORDER/etc. stay unconditional terminators so PR AfterShip#268 trailing-comma handling is preserved. Adds regression fixture select_trailing_comma_before_from_keyword_table.sql.
erezrokah
pushed a commit
to erezrokah/clickhouse-sql-parser
that referenced
this pull request
May 11, 2026
Empirically validated against ClickHouse server 25.10.7.6: CH accepts every reserved keyword as a bare column reference in SELECT projections except NOT (a unary operator). Tested 256 keywords in both mid-list (`SELECT a, KW, b FROM t`) and last-item (`SELECT a, KW FROM t`) positions. Changes: - isSelectItemTerminatorKeyword: bypass terminator handling for ANY keyword whose next token is `,`, `AS`, or another clause-starter keyword. Backticked identifiers tokenize as TokenKindIdent (not TokenKindKeyword), so the trailing-comma-before-keyword-table case from AfterShip#268 (`SELECT count(*), FROM `limit``) is still handled. - parseColumnExpr: reuse the same lookahead so CASE/CAST/EXTRACT/INTERVAL parse as identifiers when used as bare column names (covers the last-select-item case where the line-420 `,`/AS fallback didn't fire). - Fixtures: replace the single select_limit_as_column.sql with four fixtures covering the four classes — column-like tail keyword (LIMIT), clause-starter keyword (FROM/TO/WHERE/GROUP/ORDER), last-item form, and expression-starter keyword (CASE/CAST/EXTRACT/ INTERVAL). Existing trailing-comma fixture (select_trailing_comma_before_from_keyword_table.sql) still passes.
orian
pushed a commit
to orian/clickhouse-sql-parser
that referenced
this pull request
May 13, 2026
…fterShip#268) ClickHouse accepts `SELECT a, FROM t` (e.g. when a column is commented out with `--`). Break the SELECT-item loop after consuming a comma if the next token is a clause-starting keyword (FROM, WHERE, GROUP, etc.). Fixes incorrect AST for existing CREATE MATERIALIZED VIEW fixtures where FROM was previously consumed as a bare select expression.
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.
Permits
SELECT a, FROM t(e.g. when a column is commented out with--), matching ClickHouse behavior; also corrects two CREATE MATERIALIZED VIEW fixtures whereFROMwas previously consumed as a bare select expression.