Fix jdbc-v2: reparse the parameters of a recovered ANTLR4 parse tree - #3026
Open
polyglotAI-bot wants to merge 2 commits into
Open
Fix jdbc-v2: reparse the parameters of a recovered ANTLR4 parse tree#3026polyglotAI-bot wants to merge 2 commits into
polyglotAI-bot wants to merge 2 commits into
Conversation
ANTLR4_PARAMS_PARSER collects the ? placeholders from the parse tree only. A statement the grammar cannot match is still given a parse tree, completed by error recovery, but the tokens the parser recovered on are not part of it, so a placeholder inside such an expression never reaches the listener and is lost. Re-derive the placeholders from the original SQL in that case, as the other two parser backends always do. Fixes: #3025
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
…4params-parameter-undercount # Conflicts: # jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/SqlParserFacade.java # jdbc-v2/src/test/java/com/clickhouse/jdbc/internal/BaseSqlParserFacadeTest.java
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 #3025.
The
ANTLR4_PARAMS_PARSERbackend derivesargCount/paramPositionsonly from listener callbacks on the ANTLR4 parse tree (SqlParserFacade.ANTLR4AndParamsParser/ParseStatementAndParamsListener); the other two backends (JAVACC, plainANTLR4) re-derive them from the original SQL with the tree-independentparseParameters(...). A statement the bundled grammar cannot match is still given a parse tree, completed by error recovery, but the tokens the parser recovered on are not part of it - so a?inside an expression the grammar could not match never fires its callback and is silently dropped. The trigger does not have to be invalid SQL: valid ClickHouse syntax the grammar does not cover is enough (hex(x'AB')), as is a JDBC escape sequence ({d '...'}). The consequences are user-visible:getParameterMetaData().getParameterCount()is too low,setXxxfor a dropped placeholder throws (PreparedStatementImplsizes its value array fromargCount), andbuildSQL()substitutes the remaining values at the wrong offsets. This PR drops what the listener collected and re-derives the placeholders from the original SQL whenever the statement could not be parsed without errors, which is exactly what the other two backends always do.Measured on
main(1a11756), same SQL through all three backends -ANTLR4_PARAMS_PARSERis the only one affected, and after this change all three agree:Changes
SqlParserFacade.ANTLR4AndParamsParser#parsePreparedStatement: onisHasErrors(), call the newreparseParametersOfRecoveredParseTree(sql, stmt), which resets the collected parameters and runsparseParameters(sql, stmt). Placed next to the existingisHasErrors()handling, mirroring how the othertree-derived fields are treated when the parse tree is a product of error recovery.
ParsedPreparedStatement#resetParameters(): package-private, setsargCountback to0so the placeholders can bere-collected (there was no way to reset it). Every consumer of
getParamPositions()reads only the firstargCountentries, so the entries beyond it are never read.
JAVACCor plainANTLR4backends, and no change for a statement that parses without errors.Test
BaseSqlParserFacadeTest#testParametersInUnparsableExpressions(new@DataProvider) - runs for all threebackends through the existing
JavaCCParserTest/Antlr4ParserTest/Antlr4ParamsParserTestsubclasses, andasserts both the parameter count and the exact position of each placeholder. Rows cover a placeholder after an
unmatched expression, an unmatched expression in each of two value groups, a
SELECT(the loss is notinsert-specific), plus two contrast rows: a statement that hits error recovery but has no placeholders (must stay
0) and one that parses cleanly (must stay2). Before the fix, exactly the fourANTLR4_PARAMS_PARSERrows failed(
0/1instead of1/2); the other two backends passed unchanged.PreparedStatementTest#testInsertWithUnparsableValueExpression(new, integration, parametrized over the threebackends) - goes through the real
prepareStatemententry point: assertsgetParameterCount(), sets the parameter,executeUpdate()s and reads the row back (v1 = 'AB'fromhex(x'AB'),v2= the bound value), and does the samefor a
SELECT. Before the fix theANTLR4_PARAMS_PARSERrun failed (getParameterCount()returned0).jdbc-v2unit suite (1316 tests) and thePreparedStatementTest+StatementTestintegration suites (146tests) pass against ClickHouse 26.5; no existing test was modified.
I also checked the two ways this fallback could plausibly have made something worse, and neither happens: a
?in aheredoc (
$$a?b$$) does not sethasErrors, so the fallback never runs for it (that scan gap is#3009), and for a ternary in a statement that does hit
error recovery (
SELECT n > 0 ? ? : ? FROM t WHERE h = hex(x'AB')) this backend returned no parameters at allbefore the change, so it now behaves like the
ANTLR4andJAVACCbackends rather than worse than them.Pre-PR validation gate
AGENTS.md/docs/changes_checklist.mdCHANGELOG.mdupdatedprepareStatement)Compatibility
No public API change;
resetParameters()is package-private. Behavior changes only for a statement that the ANTLR4grammar could not parse without errors while
jdbc_sql_parser=ANTLR4_PARAMS_PARSERis selected - such statementspreviously lost placeholders.
docs/features.mdneeds no update (no feature added, removed, or intentionally changed).Related
parseParametersscan itself (comments and heredocs), which this fix now also relies on for the error path.