-
Notifications
You must be signed in to change notification settings - Fork 49
Allow ARRAY JOIN to be interleaved with other JOIN statements #230
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow ARRAY JOIN to be interleaved with other JOIN statements #230
Conversation
Pull Request Test Coverage Report for Build 21248785455Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
clickhouse-sql-parser/parser/parser_query.go
Lines 203 to 213 in d563f06
| case p.matchKeyword(KeywordLeft), p.matchKeyword(KeywordRight): | |
| modifiers = append(modifiers, p.last().String) | |
| _ = p.lexer.consumeToken() | |
| if p.matchKeyword(KeywordOuter) { | |
| modifiers = append(modifiers, p.last().String) | |
| _ = p.lexer.consumeToken() | |
| } | |
| if p.matchKeyword(KeywordSemi) || p.matchKeyword(KeywordAnti) || | |
| p.matchKeyword(KeywordAny) || p.matchKeyword(KeywordAll) || | |
| p.matchKeyword(KeywordAsof) || p.matchKeyword(KeywordArray) { | |
| modifiers = append(modifiers, p.last().String) |
The new parseJoinOp branch for LEFT/RIGHT now accepts ARRAY as a modifier, which means inputs like RIGHT ARRAY JOIN ... will parse as valid. That widens the grammar compared to the existing array-join clause parser, which only allows LEFT/INNER before ARRAY JOIN (so RIGHT was previously rejected). This makes the parser accept SQL that ClickHouse doesn’t support and produces an AST for invalid queries; it’s especially likely to slip through formatting/analysis without errors. Consider restricting ARRAY to the LEFT/INNER cases only (or explicitly reject RIGHT ARRAY JOIN).
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1df3a0f8d
ℹ️ 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".
|
@hermanschaaf Thanks for your improvement, a few comments inline. |
|
Thanks @git-hulk, please take another look. I'm not sure how you handle breaking changes in this library, but I assume that removing ArrayJoin could be a breaking change, at least to some users. Also there's some error with the linter now, I'm not sure what the issue is, any tips would be appreciated. |
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91b3cdef30
ℹ️ 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".
It should be caused by the Go version being too low(1.18). |
Thanks for pointing that out. We would absolutely like to keep compatibility if possible, but this is a design issue in the old implementation. So, I prefer to use your implementation and just highlight this in our release notes. |
|
btw there's a typo in the |
|
https://github.com/AfterShip/clickhouse-sql-parser/blob/master/.github/workflows/ci.yaml#L18 @hermanschaaf Need to change the go version in ci.yaml. |
|
@hermanschaaf Thank you! |
Since opening #229, I've learned that it is also valid to interleave ARRAY JOIN statements with other JOIN statements. This PR implements a fix for this.
Example
Here is a minimal example that can be tested against both ClickHouse and the parser. Before this change, ClickHouse succeeds but the parser fails, as it assumes that ARRAY JOIN statements always comes first.