Fix jdbc-v2: nest /* */ block comments in the ANTLR4 lexer - #3022
Open
polyglotAI-bot wants to merge 3 commits into
Open
Fix jdbc-v2: nest /* */ block comments in the ANTLR4 lexer#3022polyglotAI-bot wants to merge 3 commits into
polyglotAI-bot wants to merge 3 commits into
Conversation
The lexer rule ended a block comment at the first */, so the rest of a nested comment was lexed as SQL: statements the server accepts were reported as syntax errors and ANTLR4_PARAMS_PARSER counted a ? inside the nested part as a bind parameter. The comment is now lexed through a dedicated lexer mode whose mode stack holds the nesting level, mirroring the JavaCC grammar's nesting depth counter and the server. Fixes: #3021
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
7 tasks
TriageCategory: Summary What this impacts
Concerns
Required reviewer action
|
|
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
2 participants
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.
Description
Fixes #3021.
ClickHouse nests
/* */block comments: an inner/*raises the nesting level and only the matching*/ends the comment. Thejdbc-v2JavaCC grammar nests (commentNestingDepth,ClickHouseSqlParser.jj) andClickHouseUtils.skipMultiLineCommentnests, but the ANTLR4 lexer rule was'/*' .*? '*/', whose non-greedy body stops at the FIRST*/. Everything after that closer was lexed as SQL, so with theANTLR4/ANTLR4_PARAMS_PARSERbackends a statement the server accepts (e.g.SELECT 1 /* ) /* ) */ ) */, ?) produced error nodes — whichSqlParserFacadeturns intohasErrorsand a forcedhasResultSet— andANTLR4_PARAMS_PARSER, which takes placeholder positions from the parse tree, counted a?inside the nested part of the comment as a bind parameter and reported wrong positions. Those positions are load-bearing:PreparedStatementImplslices the original SQL by them.The comment is now lexed through a dedicated lexer mode; the mode stack holds the nesting level, which is the ANTLR equivalent of the JavaCC grammar's depth counter and of the server's lexer.
Notes on behaviour outside the nesting case:
/* x */,/**/,/*/ x */,/*inside a string literal,a / bdivision).SELECT 1 /* oops) is now skipped to the end of the statement instead of being lexed as stray/*tokens, so theANTLR4_PARAMS_PARSERbackend no longer reportshasErrorsfor it (the server still rejects the statement). TheANTLR4andJAVACCbackends are unaffected — their placeholder scan (ClickHouseUtils.skipMultiLineComment) already throws for an unclosed comment.Changes
jdbc-v2/src/main/antlr4/.../ClickHouseLexer.g4:MULTI_LINE_COMMENTnow pushes a newIN_MULTI_LINE_COMMENTlexer mode; inside that mode/*pushes another level,*/pops one, and any other character is skipped.jdbc-v2/src/test/java/.../BaseSqlParserFacadeTest.java: newtestBlockCommentNesting@DataProviderregression test (runs on all three backends via the existingJavaCCParserTest/Antlr4ParserTest/Antlr4ParamsParserTestsubclasses).CHANGELOG.md: Bug Fixes entry.Test
testBlockCommentNestingasserts, for each statement, that it parses without errors, the argument count, the exact?parameter positions in the original SQL, and the insert/result-set classification. Expected values were verified against a live ClickHouse 26.5 server (e.g.SELECT 1 /* ) /* ) */ ) */, 2→1 2,SELECT /* a /* b */ c */ 1→1,SELECT /* ? /* ? */ ? */ 5→5).Rows cover: nested comments containing parens /
?/;, a multiline nested comment, three-level and 64-level nesting, a comment before the statement plus a trailing comment, a comment in aWHEREclause between two placeholders, nested comments inside aVALUESlist and before the column list of anINSERT; plus contrast rows that must keep their current behaviour — a plain comment,/**/,/*/ 2 */, a/* ... */inside a string literal, and a plain comment in aVALUESlist.Without the fix, 15 of the 48 cases fail (both ANTLR4 backends); the
JAVACCrows pass before and after, which is what pins the expectations to the already-correct backend. With the fix the wholejdbc-v2unit suite is green:mvn -pl jdbc-v2 test→ 1346 tests, 0 failures.Pre-PR validation gate
main, 0 with the fix)jdbc-v2unit suite greenAGENTS.md/docs/changes_checklist.md("String, SQL, or serialized output changed" + "Conditional logic or guard changed": comment/quoting rules checked against the server, focused regression tests added, TestNG@DataProviderused instead of near-identical methods, no issue references or narrative inside the test code) anddocs/features.md(the documented rule that?inside comments is not a bind parameter is now honoured by the ANTLR4 backends — no doc change needed)Out of scope (separate follow-up)
While confirming this, the ANTLR4
SINGLE_LINE_COMMENTrule turned out to accept--,#and#!but not//, which both the server and the JavaCC grammar accept. That is a distinct defect in a distinct rule, so it is deliberately not bundled here.