-
Notifications
You must be signed in to change notification settings - Fork 614
[jdbc-v2] Fix parser issues #2473
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
Conversation
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
|
I ran into an unexpected issue while reviewing this PR. Please try again later. |
|
/windsurf-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.
💡 To request another review, post a new comment with "/windsurf-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.
Pull Request Overview
Fixes several SQL parser issues by enhancing grammar rules, updating parsing logic, and extending test coverage for INSERT and other statements.
- Added support for unquoting table identifiers in both generic and INSERT contexts.
- Introduced new keyword tokens (
NAME,USER) and rearranged test assertions. - Expanded test cases for INSERT/DELETE statements and updated client buffer defaults and CI flags.
Reviewed Changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| jdbc-v2/src/test/java/com/clickhouse/jdbc/internal/SqlParserTest.java | Reordered an assertion and added new test cases for INSERT, DELETE, and COUNT expressions. |
| jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/ParsedPreparedStatement.java | Added enterTableExprIdentifier hook and updated enterInsertStmt to use unquoteIdentifier. |
| jdbc-v2/src/main/antlr4/com/clickhouse/jdbc/internal/ClickHouseParser.g4 | Added NAME and USER to the keyword rule. |
| client-v2/src/test/java/com/clickhouse/client/ClientTests.java | Changed socket buffer sizes from 8196 to 804800 in tests. |
| .github/workflows/analysis.yml | Added --no-transfer-progress to Maven commands for CI. |
Comments suppressed due to low confidence (4)
jdbc-v2/src/test/java/com/clickhouse/jdbc/internal/SqlParserTest.java:298
- Tests include unquoted and quoted table names but lack schema-qualified identifiers. Consider adding cases like
"insert into db.events (s) values ('a')"to validate parsing of multi-part names.
{"insert into `events` (s) values ('a')", 0},
jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/ParsedPreparedStatement.java:215
- [nitpick] The generic override
enterTableExprIdentifiersetsthis.tablefor any table expression, which can unintentionally override the target table in INSERT or other statements. Consider scoping this assignment to specific contexts or resetting the field before reuse.
public void enterTableExprIdentifier(ClickHouseParser.TableExprIdentifierContext ctx) {
jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/ParsedPreparedStatement.java:225
- Using
tableId.getText()may include schema qualifiers or quoting syntax thatunquoteIdentifierdoes not fully handle. Add tests and update parsing logic to correctly extract both schema and table names (e.g.,db.table).
this.table = SqlParser.unquoteIdentifier(tableId.getText());
client-v2/src/test/java/com/clickhouse/client/ClientTests.java:299
- [nitpick] The buffer size
804800is a magic number; extracting it into a named constant or adding a comment explaining its origin will improve readability and maintainability.
.setSocketRcvbuf(804800)
|
|
Hello, this issue very critical for me, can you please tell me when the fix will be released? |
|
Good day, @OksiBlack! |




Summary
Closes #2450
Closes #2451
Closes #2461
Checklist
Delete items not relevant to your PR: