Support SQL-standard keyword arguments for trim, substring, overlay and overlayUTF8 - #290
Open
therealpandey wants to merge 7 commits into
Open
Support SQL-standard keyword arguments for trim, substring, overlay and overlayUTF8#290therealpandey wants to merge 7 commits into
therealpandey wants to merge 7 commits into
Conversation
AfterShip#289) ClickHouse accepts `trim(BOTH ' ' FROM s)`, `substring(s FROM 2 FOR 3)` and `overlay(s PLACING r FROM 2 FOR 3)`, but the parser only split arguments on commas, so it rejected all of them. Recognise the keyword form from a per-function table listing the leading modifiers, the separator keywords in order, and how many are mandatory. The form is decided after the first argument, as parseColumnExtractExpr already does for EXTRACT, so comma arguments and `both`/`placing`/`overlay` as column names keep working. Each separator becomes a BinaryOperation and the trim modifier a UnaryExpr, so no new AST node is needed and traversal and formatting are unchanged. Grammar verified against ClickHouse 26.8.1.337 in both directions: the forms it rejects — a missing trim modifier, a missing FROM, out-of-order separators, and substringUTF8, which has no keyword form unlike overlayUTF8 — stay rejected.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b745bcbbd1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
therealpandey
marked this pull request as draft
July 29, 2026 13:33
A comma fills the same argument slot a separator keyword would, so the slot index has to run across the argument list rather than reset on every item. Without that, `substring(s, 2 FOR 3)` looked for FROM in the second item and rejected FOR, though ClickHouse accepts it. Two per-function limits keep the shared slot index from admitting more than ClickHouse does: - overlay and overlayUTF8 cannot mix commas with their keyword form in either direction, so `overlay(s PLACING r FROM 1, 2)` and `overlay(s, r FROM 1)` stay syntax errors. - substring's keyword form has a fixed arity, so `substring(s, 2 FOR 3, 4)` and `substring(s FROM 2, 3, 4)` stay syntax errors however the earlier separators were spelled. Comma-only calls keep the arity they parse with today. trim needs neither: a trailing comma argument after its keyword form is an arity error in ClickHouse rather than a syntax error. All 34 mixed and unmixed forms were diffed against ClickHouse 26.8.1.337 in both directions and now agree.
therealpandey
force-pushed
the
fix/keyword-arg-functions
branch
from
July 29, 2026 15:20
189f513 to
b812071
Compare
The keyword-argument parser parsed each argument with parseExpr, losing three
things parseColumnExprListWithLParen does on the path it replaces. Because every
call to these four functions is routed through it, plain comma calls lost them
too:
- an optional `AS <alias>` per argument, so `trim('x' AS y)` and
`substring('h' AS s, 2)` stopped parsing although ClickHouse accepts them
- a leading DISTINCT
- a trailing parametric argument list
Also drop the fixture and assertion for `trim(both)` and `trim(both, x)`:
ClickHouse rejects both, because its trim grammar always reads BOTH as the
modifier, so they were the wrong thing to pin. BOTH as an ordinary name is still
covered through max(both) and an alias.
Reuse parseOneStmt from position_test.go rather than a second copy of it.
The keyword forms build UnaryExpr and BinaryOperation instead of a node of their own, so nothing in walk.go or the position tests covered them. Assert that Walk reaches every operand and that the spans match what the same nodes report elsewhere, so a future change to either cannot regress them silently. Also collapse the two comma checks, which returned the same error, into one.
No other parse function in the file carries a godoc, and the remaining notes either repeated the condition they sat on or described the AST literal below them. What stays is the reason FROM cannot go in getNextPrecedence, the two field semantics the names do not convey, and the slot index running across the argument list.
trimBoth, trimLeft, trimRight and the ltrim/rtrim aliases take comma arguments only. Assert they still round-trip and that the keyword form is rejected for them, since ClickHouse offers it on trim alone.
therealpandey
changed the title
position and locate take their argument with IN, CAST with AS, and substringUTF8, substr, mid and byteSlice take commas only. One case each, so the fixture pins the whole family rather than only the functions this change touches.
therealpandey
marked this pull request as ready for review
July 29, 2026 16:41
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be44ad6e6d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Comment on lines
+494
to
+498
| KeywordOverlay, | ||
| KeywordOverlayUTF8, | ||
| KeywordPartition, | ||
| KeywordPipeline, | ||
| KeywordPlacing, |
There was a problem hiding this comment.
Adding these names to the lexer keyword set makes overlay, overlayUTF8, and placing produce TokenKindKeyword, but the bare table-alias path still only consumes aliases when currentTokenKind() != TokenKindKeyword (parser/parser_query.go:414). As a result, queries that previously parsed, such as SELECT * FROM t placing or SELECT * FROM t overlay, now fail unless users add AS or quote the alias, even though these words are not added to reservedKeywords and the rest of the parser treats non-reserved keywords as identifiers.
Useful? React with 👍 / 👎.
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
1 participant
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.
Fixes #289.
ClickHouse accepts keyword-separated arguments for these four; the parser only split on commas:
A per-function table drives it, and the form is decided after the first argument, as
EXTRACTalready does. Commas and separator keywords share one slot index, so mixing matches ClickHouse per function:Separators reuse
BinaryOperationand the modifierUnaryExpr, so no new AST node and no visitor, walk or formatter change. No existing golden moved.Verified in both directions against ClickHouse 26.8.1.337.