Fix jdbc-v2: discard the values list of a recovered ANTLR4 parse tree - #3020
Open
polyglotAI-bot wants to merge 2 commits into
Open
Fix jdbc-v2: discard the values list of a recovered ANTLR4 parse tree#3020polyglotAI-bot wants to merge 2 commits into
polyglotAI-bot wants to merge 2 commits into
Conversation
The ANTLR4 parser backends read the INSERT values list positions and the value group count from parse tree contexts. A statement the grammar cannot match still gets a tree, completed by error recovery, where a context ends at the token the parser recovered on: the values list was then reported to stop at the closing parenthesis of a nested function call, and a two-group values list could be counted as a single group. PreparedStatementImpl slices the original SQL with those positions for batch inserts, so the template lost its closing parenthesis and the server rejected the statement. Fixes: #3019
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
8 tasks
…4-values-list-of-recovered-tree # Conflicts: # jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/SqlParserFacade.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 #3019.
With
jdbc_sql_parser=ANTLR4/ANTLR4_PARAMS_PARSER, the values list positions (enterAssignmentValuesList) and the value group count (enterDataClauseValues) are read from ANTLR parse tree contexts. A statement the bundled grammar cannot match is still given a parse tree, completed by error recovery, where a rule context ends at the token the parser recovered on rather than at the token the rule requires. ForINSERT INTO t (v1, v2) VALUES (?, hex(x'AB'))the values list is then reported to stop at the closing parenthesis of the nestedhex(...)call - one character short - while still reporting a single value group, which is exactly the preconditionPreparedStatementImpluses to slice the values-list template out of the original SQL.executeBatch()therefore sends... VALUES (1, hex(x'AB'),(2, hex(x'AB')and the server answersCode: 62 ... SYNTAX_ERROR. The same recovery also counts a genuinely two-group values list as one ((?, hex(x'AB')), (?, hex(x'CD'))), which additionally gates the betaRowBinarywriter inConnectionImpl#prepareStatement.Triggers are not limited to JDBC escape sequences (
{d '...'}, the ANTLR-side counterpart of #3017):x'AB'is valid ClickHouse SQL (SELECT hex(x'AB')->AB) that this grammar has no rule for, so a plain, server-valid statement reproduces it.The fix discards both properties when the parse produced errors - already tracked through
visitErrorNode- so the driver falls back to its generic per-statement parameter substitution path. Statements that parse cleanly are untouched, and the defaultJAVACCbackend is not affected by this (its own variant is #3017 / #3018).Changes
jdbc-v2 SqlParserFacade:ANTLR4Parser#parsePreparedStatementandANTLR4AndParamsParser#parsePreparedStatementnow calldiscardValuesListOfRecoveredParseTree(stmt)whenstmt.isHasErrors(), resetting the values list positions to-1and the value group count to0.CHANGELOG.md: bug-fix entry.Test
BaseSqlParserFacadeTest#testValuesListOfUnsupportedSyntax(runs for all three backends): asserts the invariant that a reported single-group values list starts at(and stops exactly where that group's parenthesis closes (quote-aware), with every parameter position inside it, and that a two-group list is never reported as one. Rows coverhex(x'AB'),toDate({d '2024-01-01'})and a two-group list, plus contrast rows that must keep reporting their positions, group count and argument count unchanged (hex('AB'), a)inside a string literal, a trailing;, and a clean two-group list). Without the fix: 6 failures (3 rows x 2 ANTLR4 backends);JAVACCpasses before and after.PreparedStatementTest#testBatchInsertWithValueOfUnsupportedSyntax(integration, parametrized over the three backends): preparesINSERT INTO t (v1, v2) VALUES (?, hex(x'AB')), adds two batches, executes and reads the rows back. Without the fix both ANTLR4 backends fail with the server'sSYNTAX_ERRORon the truncated template;JAVACCpasses before and after.mvn -pl jdbc-v2 test: 1319 tests, all green.mvn -pl jdbc-v2 -DskipUTs=true -Dit.test=PreparedStatementTest,StatementTest,ConnectionTest verify: 205 tests, 2 pre-existing failures unrelated to this change (ConnectionTest#testSecureConnection,#testSSLModeVerifyCa- the local environment has no TLS endpoint).changes_checklist.md
SqlParserFacadeand only resets values already carried by the internalParsedPreparedStatement.DEBUGthrough the existingLOGof the class, with no SQL in the message.docs/features.mdchange (nojdbc-v2feature added, removed, or intentionally changed - a statement that previously produced a broken batch statement now goes through the existing generic substitution path).CHANGELOG.mdupdated, with the issue link.RowBinarywriter route (noted in the CHANGELOG); results stay correct.Pre-PR validation gate
PreparedStatementTest#testBatchInsertWithValueOfUnsupportedSyntaxfails onmainfor both ANTLR4 backends with the serverSYNTAX_ERROR)AGENTS.md(targeted Maven runs, TestNG@DataProviderinstead of near-identical methods, no issue numbers or narrative in test code)prepareStatement/addBatch/executeBatch, not only at parser level)Notes for reviewers
While verifying this I measured a separate defect that is deliberately not addressed here:
ANTLR4_PARAMS_PARSERderivesargCount/paramPositionsonly from listener callbacks, so error recovery can also swallow a?(e.g.VALUES (toDate({d '2024-01-01'}), ?)reports 0 arguments there, 1 with the plainANTLR4backend). That is a different property with a different consumer and will be raised on its own.