[JDBC-V2] Fix Parser Issues #2579
Merged
Conversation
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
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
3 tasks
![windsurf-bot[bot]](https://avatars.githubusercontent.com/in/1066231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Other comments (3)
- jdbc-v2/src/main/antlr4/com/clickhouse/jdbc/internal/parser/ClickHouseParser.g4 (490-490) There's a commented-out alternative in the cteUnboundCol rule (`// | LPAREN cteStmt? selectStmt RPAREN AS identifier # CteUnboundNestedSelect`). This suggests there might be an unresolved issue with handling nested selects in CTEs. Either implement this properly or remove the comment to avoid confusion.
- client-v2/src/test/java/com/clickhouse/client/datatypes/DataTypeTests.java (543-545) Consider using RoundingMode.HALF_UP or RoundingMode.HALF_EVEN instead of FLOOR for decimal comparisons. FLOOR always rounds down which might not be appropriate for all test cases, especially when dealing with financial data.
- jdbc-v2/src/test/java/com/clickhouse/jdbc/internal/SqlParserTest.java (240-240) The error message doesn't match the assertion. Consider changing to `Assert.assertFalse(stmt.isHasErrors(), "Statement has parsing errors")` for clarity.
💡 To request another review, post a new comment with "/windsurf-review".
jdbc-v2/src/test/java/com/clickhouse/jdbc/internal/SqlParserTest.java
Outdated
Show resolved
Hide resolved
jdbc-v2/src/main/antlr4/com/clickhouse/jdbc/internal/parser/antlr4/ClickHouseParser.g4
Show resolved
Hide resolved
jdbc-v2/src/main/antlr4/com/clickhouse/jdbc/internal/parser/antlr4/ClickHouseParser.g4
Show resolved
Hide resolved
AVMusorin
reviewed
Oct 10, 2025
jdbc-v2/src/test/java/com/clickhouse/jdbc/internal/BaseSqlParserFacadeTest.java
Show resolved
Hide resolved
EastLord
reviewed
Oct 13, 2025
jdbc-v2/src/main/antlr4/com/clickhouse/jdbc/internal/parser/ClickHouseParser.g4
Outdated
Show resolved
Hide resolved
|
EastLord
reviewed
Oct 15, 2025
| | SYSTEM DROP UNCOMPRESSED CACHE | ||
| | SYSTEM DROP COMPILED EXPRESSION CACHE | ||
| | SYSTEM DROP QUERY CONDITION CACHE | ||
| | SYSTEM DROP QUERY CACHE (TAG literal)? |
EastLord
reviewed
Oct 15, 2025
There was a problem hiding this comment.
Hello,
In ClickHouseLexer.g4, two distinct tokens are defined:
QUERY : '?';
QUERY_SQL : Q U E R Y;
However, in ClickHouseParser.g4, the rule for SYSTEM DROP ... CACHE uses the QUERY token:
SYSTEM DROP QUERY CACHE
The parser rule should be updated to use the QUERY_SQL token, which correctly represents the literal keyword.
The line should be changed from:
SYSTEM DROP QUERY CACHE
To:
SYSTEM DROP QUERY_SQL CACHE
This will ensure the grammar correctly parses the valid ClickHouse SQL syntax.
There was a problem hiding this comment.
Yes, you are right.
Thank you!
Seems I need to rename query to JDBC_PARAM
…RAM_PLACEHOLDER to avoid mistakes
EastLord
approved these changes
Oct 16, 2025
|
Hello. To help identify and address any remaining gaps, I would like to propose leveraging the extensive suite of test queries directly from the official ClickHouse repository. These queries are used to test the native ClickHouse parser and represent a massive collection of valid (and invalid) SQL statements. The official test suite can be found here: https://github.com/ClickHouse/ClickHouse/tree/master/tests/queries |
|
Good day, @EastLord ! Many thanks to your help and suggestions! yes, we have already run tests from this suite. However it requires porting tests to Java, there are no JDBC parameter tests what adds another dimension for tests. |
mzitnik
approved these changes
Oct 29, 2025
| * </ul> | ||
| */ | ||
| // SQL_PARSER("jdbc_sql_parser", "JAVACC", List.of("ANTLR4", "ANTLR4_PARAMS_PARSER", "JAVACC")), | ||
| SQL_PARSER("jdbc_sql_parser", "ANTLR4", List.of("ANTLR4", "ANTLR4_PARAMS_PARSER", "JAVACC")), |
There was a problem hiding this comment.
Setting to ANTLR4 means that we are using ANTLR4 as the default one?
There was a problem hiding this comment.
Thank you for catching it!
Yes, we need to use JAVACC by default.
mzitnik
reviewed
Oct 29, 2025
|
@mzitnik |
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
This PR introduces option to select old JavaCC parser that is proven to work and has more relaxed rules. Parameter parsing code is ported from older version as most stable logic. And this PR will make old parser as choice by default so it will work almost like JDBC V1.
And:
Closes #2574
Closes #2568
Closes #2537
Closes #2595
Closes #2617
Closes #2569
Closes #2570
Closes #2571
Closes #2572
Closes #2573
Closes #2609
Checklist
Delete items not relevant to your PR: