-
Notifications
You must be signed in to change notification settings - Fork 615
[jdbc-v2] Support multi-dot notation for database names #2663
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
base: main
Are you sure you want to change the base?
[jdbc-v2] Support multi-dot notation for database names #2663
Conversation
Fixes ClickHouse#2650 Changes: - Updated ANTLR grammar databaseIdentifier rule to support multiple dot-separated identifiers: identifier (DOT identifier)* - Added extractTableName() helper method in SqlParserFacade to properly handle multi-part table identifiers by unquoting each part separately - Added testMultiDotNotation() test with 3 test cases covering SELECT, INSERT, and quoted identifiers - All 367 existing tests continue to pass This allows database names like 'a.b' in table references such as 'a.b.c' or '`db.part1`.`table`' to be parsed correctly.
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".
jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/SqlParserFacade.java
Outdated
Show resolved
Hide resolved
Addresses PR feedback: Only append dot after database identifier if table identifier is not null to avoid trailing dots.
|
/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.
Looks good to me 🤙
💡 To request another review, post a new comment with "/windsurf-review".
jdbc-v2/src/main/antlr4/com/clickhouse/jdbc/internal/parser/antlr4/ClickHouseParser.g4
Show resolved
Hide resolved
jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/SqlParserFacade.java
Outdated
Show resolved
Hide resolved
|
Good day, @devdavidkarlsson ! I've left comments. |
Address PR feedback: Handle multi-dot notation directly in the parser grammar rather than in post-processing Java code. Changes: - ANTLR: Simplified to use getText() directly from parser, removed extractTableName() method that was walking the parse tree - JavaCC: Modified tableIdentifier rule to parse all dot-separated identifiers and split database/table within the grammar action - Both parsers now handle the logic in the grammar/parser itself
|
@chernser new attempt: I added the JavaCC (.jj) and got rid of extractTableName, tests should be green now. |
|
/windsurf-review |
1 similar comment
|
/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".
jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/SqlParserFacade.java
Outdated
Show resolved
Hide resolved
Address bot feedback about quoted identifiers containing dots. Changes: - ANTLR: Use ClickHouseSqlUtils.unescape() instead of SQLUtils.unquoteIdentifier() to properly handle escaped backticks - JavaCC: Already working correctly (no changes needed) - Added comprehensive test suite with 15 test cases covering all ClickHouse-relevant scenarios for quoted identifiers with dots Test coverage: Case SQL Pattern Database Table ---- ------------------------------------ ---------------- --------------- 1 db.table db table 2 `db`.`table` db table 3 db.`table.name` db table.name 4 `db.part1`.`table` db.part1 table 5 db.`table.name` db table.name 6 `db.part1`.table db.part1 table 7 db.`tab``le` db tab`le 8 `my db`.`table name!@#` my db table name!@# 9 `db.part1`.`table.name` AS t db.part1 table.name 10 db.`a.b.c.d` db a.b.c.d 11 `db.part1.part2`.`table` db.part1.part2 table 12 db.part1.table2 db.part1 table2 13 `db.part1`.`part2`.`table` db.part1.part2 table 14 db.part1.`table.name` db.part1 table.name 15 `db.part1`.part2.table3 db.part1.part2 table3 All 1104 tests passing (367 base tests × 3 parsers + 1 new test × 3).
ba2f40a to
823b35d
Compare
|
/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".
jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/SqlParserFacade.java
Outdated
Show resolved
Hide resolved
|
Good day, @devdavidkarlsson ! |
| super.enterColumnExprPrecedence3(ctx); | ||
| } | ||
|
|
||
| private String unquoteTableIdentifier(String rawTableId) { |
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.
strange - I thought I've left the comment that it should be actually handle by parser.
Because one of the points to have parser is to avoid writing this kind of methods.
| { | ||
| if (record && t != null && token_source.table == null) { | ||
| token_source.table = ClickHouseSqlUtils.unescape(t.image); | ||
| if (record && token_source.table == null && parts.size() > 0) { |
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.
why parser cannot do it for us?
chernser
left a comment
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.
Please try to get clean table and database names using parser only.
…the parsing, instead of in the java code
Description
Fixes #2650
This PR adds support for multi-dot notation in database names without requiring quotes, enabling table references like
a.b.cwherea.bis the database name andcis the table name.Changes
databaseIdentifierrule to support multiple dot-separated identifiers:identifier (DOT identifier)*extractTableName()helper method inSqlParserFacadeto properly unquote each identifier part separately and join them correctlytestMultiDotNotation()test method with 3 test cases:SELECT * FROM a.b.c WHERE id = ?SELECT * FROM `db.part1`.`table` WHERE id = ?INSERT INTO a.b.c (col1, col2) VALUES (?, ?)Testing
Example Usage
The JDBC driver now correctly parses these statements and extracts the full table name
test.db.usersfor metadata operations.