Repository navigation
Fix jdbc-v2: use the RowBinary writer only for a values list of placeholders - #3173
Merged
Merged
Conversation
…holders With beta.row_binary_for_simple_insert enabled, ConnectionImpl#prepareStatement routes a single-group INSERT ... VALUES to WriterStatementImpl unless the parser reports a function in the values list. The writer takes one bound value per column, so a values list holding a literal, an expression, a JDBC escape or a query parameter was written with its bound values shifted to other columns. - JAVACC: the values-list check for anything that is not a placeholder was overwritten by the token manager's function flag. Combine the two. - ANTLR4 / ANTLR4_PARAMS_PARSER: only function calls were reported. Report any value in the values list that is not a bare placeholder. Fixes: #3083 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
…s-list-literal-not-rowbinary
TriageCategory: Summary What this impacts
Concerns
Required reviewer action
|
|
chernser
approved these changes
Oct 1, 2026
Contributor
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 7470ea0. Configure here.
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.
Summary
Closes #3083.
With
beta.row_binary_for_simple_insert=true,ConnectionImpl#prepareStatementsends a single-groupINSERT ... VALUEStoWriterStatementImplunlessParsedPreparedStatement.useFunctionis set. The writer takes one bound value per column. So when the values list holds a value that is not a?placeholder, the bound values move to other columns. Examples of such values: a literal (7,NULL,'str'), an expression (? + 1,?::Int32,[?, ?]), a JDBC escape ({fn ABS(-7)}) or a query parameter ({p1:DateTime}). ForINSERT INTO t (a, b, c) VALUES (7, ?, ?)with a nullablec, the result isa=20 b=30 c=NULL, with no error.The parsers did not report such values:
JAVACC(the default):parsePreparedStatementalready scans the values list for anything other than?,,and whitespace. Then it overwrites that result withparsedStmt.isFuncUsed(), which is true only for a function call. Now the two signals are combined.ANTLR4/ANTLR4_PARAMS_PARSER: only theInsertParameterFuncExpralternative set the flag. TheInsertRawValue(literal) andInserParameterExpr(expression) alternatives did not. NowenterAssignmentValuesListsets the flag when any value is not a bare placeholder (InsertParameterContext). This checks the contract directly ("a values list of parameter placeholders only") and does not list the alternatives that break it.The issue names the
JAVACCbackend. The twoANTLR4backends have the same defect for literals and expressions, and the same entry point reaches it, so this PR fixes all three.Changes
jdbc-v2/.../internal/SqlParserFacade.javaJavaCCParser#parsePreparedStatement:setUseFunction(stmt.isUseFunction() || parsedStmt.isFuncUsed()).ParsedPreparedStatementListener#enterAssignmentValuesList: setuseFunctionwhen a value is not a bare?.jdbc-v2/.../ConnectionImpl.java: one comment line added to the list of "RowBinary cannot be used when" cases.CHANGELOG.md: bug-fix entry.Compatibility
docs/features.mdhas no entry for this beta property.isUseFunction()is the routing decision inConnectionImpl. A values list of placeholders only still uses the writer on all three backends; the contrast rows below pin this.JAVACC, a comment inside the values list (VALUES (?, /* c */ ?)) is now reported by the scan. That statement uses the standard path, which is always correct; only the beta writer optimization is not used.Test
BaseSqlParserFacadeTest#testInsertUseFunction(runs for all three backends). New rows:useFunction=true):(7, ?, ?),(?, ?, 7), a string literal,NULL,-7,? + 1,?::Int32,[?, ?],{fn now()},{p1:DateTime}.useFunction=false): placeholders separated by newline and tab.main, 26 rows fail; with the fix, none.WriterStatementImplTest#testInsertUsesWriterOnlyForValuesListOfPlaceholders(integration, all three backends). Each statement goes throughprepareStatementwith the beta writer enabled, into(a Int32, b Int32, c Nullable(Int32)), and the test asserts the whole row (7, 20, 30).{fn ABS(-7)}must not useWriterStatementImpl.(?, ?, ?)must still useWriterStatementImpl.main, 10 rows fail (routed to the writer); with the fix, none.mvn -pl jdbc-v2 test: 1963 tests, 0 failures.mvn -pl jdbc-v2 -DskipUTs=true verifyagainst ClickHouse 26.9: 576 tests, 4 failures, none caused by this change.ConnectionTestSSL × 2: the local environment has no client certificates.StatementTest× 2: "database already exists", caused by side databases left from an earlier local run.PreparedStatementTestandWriterStatementImplTestall pass on a fresh database.Not in this PR
The issue also suggests an optional, complementary guard: select the writer only when the placeholder count equals the column count. This PR does not add it, because it would change how an insert with fewer placeholders than columns (with column defaults) behaves today. The parser fix alone covers
(?, ? + 1, ?), where the counts are equal but the values list is not placeholders only.Checklist
main(parser andprepareStatement), root cause above, fix on the live routing path, new tests fail without the fix and pass with it, no existing test changed🤖 Generated with Claude Code