fix: Accept reserved keyword as expression operand - #305
Merged
git-hulk merged 3 commits intoAug 5, 2026
Conversation
A reserved keyword followed by a binary operator, `::`, `)` or `]` can only be an operand, so read it as a column name in expression position: `WHERE limit > 0`, `toFloat64(limit)` and `SELECT limit > 0` all parse in ClickHouse but were rejected here.
Extend the disambiguator to AND/OR/NOT/IN/LIKE/ILIKE/BETWEEN/IS/THEN/ ELSE/END and share it with the select-item check, so `WHERE limit IN (1, 2)`, `THEN limit ELSE` and `SELECT limit + 1, offset + 1 FROM t` parse. Add coverage for every accepted form, formatter round-trips, and the clause-parsing cases that must keep failing.
Match the fixture style of AfterShip#269: six query fixtures with parse, format and beautify goldens, keeping only the parse-error guardrails inline.
erezrokah
marked this pull request as ready for review
August 5, 2026 12:19
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
git-hulk
self-requested a review
August 5, 2026 12:26
Contributor
|
Thanks @erezrokah for this PR! I was just about to raise one myself for this :) |
pull Bot
pushed a commit
to edisplay/signoz
that referenced
this pull request
Aug 7, 2026
Bumps `clickhouse-sql-parser` to v0.5.5, fixes the false rejection that was left over once it landed, and closes three holes in the same validator that the first two changes brought to light. ## The bump **Reserved keywords as expression operands** ([#305](AfterShip/clickhouse-sql-parser#305)). `interval` was fixed in v0.5.4, but the same defect affected 36 other keywords once the column appeared as an operand rather than bare. Sweeping 94 candidates against ClickHouse 26.8.1.337, only `on` still rejects — and ClickHouse runs that too. This one was live: `sum(limit)` on a metric label. **Panic on an unparseable `DEFAULT` expression** ([#306](AfterShip/clickhouse-sql-parser#306)). Both known cases return a parse error now instead of dereferencing nil. The `recover` in `ErrIfStatementIsNotValid` stays — it guards the next one of these, not these two. [#307](AfterShip/clickhouse-sql-parser#307) also allows `CAST` in a table function's argument list. ## Table functions are only table functions in a table position The parser types a call inside a table function's argument list as a `TableFunctionExpr` as well, so the generator allow list only ever cleared a generator whose argument was a literal. Every real dashboard computes its row count — `numbers(greatest(1, intDiv(end_ns - start_ns, step_ns) + 1))` — and every one was refused, on `intDiv` rather than on `numbers`. `TableExpr.Expr` is the only table position a SELECT can reach, so the allow list asks that instead. Of the four places the parser builds a `TableFunctionExpr`, two are `CREATE TABLE` paths rejected as not-a-SELECT before the walk starts, one is `parseTableArgPrimaryExpr`, and one is the `FROM`/`JOIN` path that wraps into a `TableExpr`. ## Three holes that were already open Skipping argument position is only safe if nothing there can read, and that turned out not to be true — not because of this change, but independently of it. **Reading functions.** `file` is both a table function and a scalar function, and the validator never inspected scalar calls at all. On `main` today, `SELECT file('/etc/passwd')` is accepted and returns the file. A numeric wrapper passes ClickHouse's type check, so the row count alone is an oracle: `numbers(length(file(x)))` yields one row per byte. The same applies to the 42 dictionary accessors, which can be backed by HTTP, ODBC or another database, to `catboostEvaluate`, and to the introspection functions. All are now refused by name wherever they appear, under `clickhouse_sql_reading_function`. **`x IN db.table`.** ClickHouse reads this as `x IN (SELECT * FROM db.table)`, and a qualified name on the right of `IN` parses as a `Path`, not a `TableIdentifier` — so `SELECT * FROM t WHERE a IN system.users` bypassed the internal-database rule entirely. Now checked, including the `GLOBAL IN` and `NOT IN` forms. **Quoted generator names.** The allow list matched on the formatted name, which carries the quoting, so ``SELECT * FROM `numbers`(31)`` was refused. It now reads the identifier the way the internal-database branch already did. ## Effect Replaying 72 distinct shapes of production `clickhouse_sql` that the validator currently rejects: **64 pass, up from 59 on v0.5.4**. Two came from the bump, three from the table-position change, and those three are 379 of the 1390 sampled occurrences. The three new rules add no false positives to the corpus. Of the eight left, four are correct rejections (`system` reads, `SHOW TABLES`), one is a dashboard variable rendering as the literal `<no value>`, one is SQL ClickHouse also rejects, and two are an open upstream gap. ## Tests `TestErrIfStatementIsNotValid_ShouldPassButFails` is back, holding what remains: three forms of a parenthesised left operand of a set operator, and `on` as a column name. It also stopped panicking — `errors.Asc` dereferences the error it is given, so a case starting to pass took the suite out with a SIGSEGV instead of reporting. Both refusal tables now share one harness, bounded by the same timeout the passing table uses. Known gap: no input is currently known to panic the parser, so the `recover` has no test exercising it.
IlyaGulya
added a commit
to IlyaGulya/clickhouse-sql-parser
that referenced
this pull request
Aug 19, 2026
`CASE -1 WHEN 1 THEN 5 ELSE 7 END` and the other prefix operator forms did not parse from v0.5.5. ClickHouse accepts all of them. The keyword disambiguator of AfterShip#305 reads a keyword as a column name when an operator follows it, because `SELECT CASE - 1` is a valid subtraction of a column with the name CASE. CASE is different from the other keywords of that group: an operator can also start the operand of the simple form. Only the WHEN that follows an operand of any length tells the two apart, and no fixed lookahead reaches it. Try the CASE reading first and fall back to the column name, as the INTERVAL case does. The lexer state is the only parse state, thus the restore is complete.
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
3 participants
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.
Reads a reserved keyword as a column name when the following token can only continue an expression - a binary operator,
::,),], or one of AND/OR/NOT/IN/LIKE/ILIKE/BETWEEN/IS/THEN/ELSE/END - so thatWHERE limit > 0,toFloat64(limit),SELECT limit + 1, offset + 1 FROM tandWHERE limit IN (1, 2)parse as ClickHouse already accepts them (every added case was verified against ClickHouse 26.2).