Fix jdbc-v2: accept // line comments in the ANTLR4 lexer - #3024
Open
polyglotAI-bot wants to merge 3 commits into
Open
Fix jdbc-v2: accept // line comments in the ANTLR4 lexer#3024polyglotAI-bot wants to merge 3 commits into
polyglotAI-bot wants to merge 3 commits into
Conversation
The lexer's SINGLE_LINE_COMMENT rule accepted '--', '#' and '#!' but not '//', which the ClickHouse server and the driver's own JavaCC grammar both accept. Since SLASH : '/' exists, '// x' lexed as two operator tokens, so both ANTLR4 backends reported syntax errors on valid SQL and misclassified an INSERT preceded by such a comment as a statement with a result set. Fixes: #3023
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
…e-slash-line-comments
TriageCategory: Summary What this impacts
Concerns
Required reviewer action
|
…e-slash-line-comments
|
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 #3023.
The
jdbc-v2ANTLR4 lexer rule for line comments accepted--,#and#!but not//:The ClickHouse server accepts
//as a line comment (verified on 26.5.1:SELECT 1 // c\n, 2returns1 2), and the driver's own JavaCC grammar already accepts it (ClickHouseSqlParser.jj:("--" | "//" | "#" | "#!")). Because the lexer also definesSLASH : '/',// commentwas lexed as twoSLASHtokens, so both ANTLR4-based backends (ANTLR4,ANTLR4_PARAMS_PARSER) raised syntax errors on SQL the server accepts; since a statement with errors is then flagged as having a result set, anINSERTpreceded by a//comment was additionally misclassified. WithANTLR4_PARAMS_PARSERa?inside a//comment was also counted as a bind parameter.The fix adds
//to the rule's alternatives, so it is skipped exactly like the other line-comment prefixes.MULTI_LINE_COMMENT('/*' .*? '*/') cannot match//, andSLASH(division,SAMPLE k/n) needs a single/, so no previously-valid input changes meaning.//inside a string literal or a quoted identifier is still literal text, since those tokens start with their quote character.Changes
jdbc-v2/src/main/antlr4/.../ClickHouseLexer.g4: added'//'toSINGLE_LINE_COMMENT.jdbc-v2/src/test/java/.../BaseSqlParserFacadeTest.java: newtestDoubleSlashLineComments@DataProvidertest (runs on all three parser backends) plus three contrast rows intestMiscStmtDp.CHANGELOG.md: bug-fix entry.Test
testDoubleSlashLineCommentscovers, through bothparsePreparedStatementandparsedStatement: a leading//comment (with and without a space), a trailing one terminated by EOF, one in the middle of aSELECTlist,\r\nline endings,//*(which must lex as a line comment, not a block comment), a//comment before anINSERT, and one inside anINSERT ... VALUESlist. It asserts no parse errors, the statement type (isInsert), and the result-set flag; the argument count is asserted for theANTLR4_PARAMS_PARSERbackend, which is the one that derives placeholders from the grammar.Contrast rows added to
testMiscStmtDp(correct before and after this change):10 / 2division,'//?0.1'inside a string literal, and`a//b?`as a quoted identifier — each keeps its existing parse and argument count.Unpatched, the 9 new rows fail on both ANTLR4 backends (18 failures); with the fix the whole
jdbc-v2module suite is green:mvn -pl jdbc-v2 test→ 1334 tests, 0 failures. TheJAVACCrows pass before and after.Two notes for the reviewer:
SqlParserFacade.parseParameters), used by theJAVACCandANTLR4backends, also does not skip//comments; that is a separate defect fixed in Fix jdbc-v2: skip // comments and heredocs when scanning for ? placeholders #3010 (issue [jdbc-v2] PreparedStatement placeholder scan misses // comments and $tag$ heredocs, so a ? inside them is counted as a parameter #3009), which is why the new test asserts the argument count only forANTLR4_PARAMS_PARSER.MULTI_LINE_COMMENTline in the same grammar file, so whichever lands second will need a trivial rebase.Pre-PR validation gate
AGENTS.md(targeted Maven run,@DataProviderparametrization, no issue numbers in test code, CHANGELOG updated)docs/features.mdunaffected (SQL comment syntax is not a listed feature)