Fix jdbc-v2: discard values list positions that do not address the original SQL - #3018
Open
polyglotAI-bot wants to merge 5 commits into
Open
Fix jdbc-v2: discard values list positions that do not address the original SQL#3018polyglotAI-bot wants to merge 5 commits into
polyglotAI-bot wants to merge 5 commits into
Conversation
…iginal SQL The JavaCC token manager records the INSERT VALUES list positions as offsets into the SQL it rebuilds from the token stream, where semicolons are dropped and JDBC escape sequences are rewritten or dropped, while PreparedStatementImpl slices the SQL the caller passed in. When the two have drifted apart the slice was taken at the wrong offsets, throwing StringIndexOutOfBoundsException out of prepareStatement or producing a truncated values list template that then threw out of addBatch. The positions are now verified against the original SQL and discarded when they do not delimit its values list, so the driver falls back to its generic parameter substitution path. Fixes: #3017
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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
…ues list check closesParenthesizedGroup() scanned the original SQL for the parenthesis closing the values list without skipping comments, so a ( or ) inside a --, //, #, #! or /* */ comment was taken as structural. For a statement whose recorded positions DO address the original SQL correctly, that ended the group early and made the check discard them, sending the statement down the generic substitution path for no reason. Skip comments the same way the token manager that recorded the positions does, including nested /* */ blocks (which ClickHouse supports).
TriageCategory: Summary What this impacts
Concerns
Required reviewer action
|
…s-list-position-coordinates
|
…s-list-position-coordinates # Conflicts: # jdbc-v2/src/test/java/com/clickhouse/jdbc/internal/BaseSqlParserFacadeTest.java
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8ac80e0. Configure here.
| LOG.debug("Values list positions [{}, {}] do not match the original SQL", startPosition, stopPosition); | ||
| stmt.setAssignValuesListStartPosition(-1); | ||
| stmt.setAssignValuesListStopPosition(-1); | ||
| } |
There was a problem hiding this comment.
Discard leaves values groups set
Medium Severity
When values list positions are discarded, assignValuesGroups stays at 1. ConnectionImpl still treats that as a simple single-group insert for the beta RowBinary writer whenever useFunction is false, so statements like VALUES ({d:Int32}, ?) can be routed to WriterStatementImpl instead of the generic substitution path. The ANTLR discard helper already clears the group count for this reason.
Reviewed by Cursor Bugbot for commit 8ac80e0. Configure here.
Collaborator
Author
There was a problem hiding this comment.
Checked this empirically rather than by inspection, and it does not reproduce.
The discard only fires when the recorded positions do not delimit the values list of the original SQL, which requires the rebuilt SQL to differ in length at or before the values list — in practice a JDBC escape sequence inside the values list. I probed every such statement the JavaCC backend parses without errors (the rejected rows of testValuesListPositionsDP), printing what ConnectionImpl routes on:
(toDateTime({ts '2024-01-01 00:00:00'}), ?) groups=1 useFunction=true start=-1 stop=-1
(toDateTime({ts '2024-01-01 00:00:00'}) /* ) */, ?) groups=1 useFunction=true start=-1 stop=-1
(toTime({t '10:20:30'}), ?) groups=1 useFunction=true start=-1 stop=-1
(toInt32({d:Int32}), ?) groups=1 useFunction=true start=-1 stop=-1
(?, toDate({d '2024-01-01'})) groups=1 useFunction=true start=-1 stop=-1
(?, toString({tt 'temp'})) groups=1 useFunction=true start=-1 stop=-1
useFunction is true in every case — the escape has to sit inside a function call for the statement to reach this code at all — so ConnectionImpl never takes the WriterStatementImpl branch, whatever the group count says.
The example in the comment, VALUES ({d:Int32}, ?), never reaches the discard: a bare escape at the start of a value group makes the JavaCC token manager record KEYWORD_VALUES_START without KEYWORD_VALUES_END, and parsePreparedStatement throws NullPointerException on the missing end key before the check runs. That is the separate defect of #3015, fixed in #3014.
The parallel with the ANTLR4 helper does not hold either. There the group count comes from an error-recovered parse tree and is itself untrustworthy, so it is cleared. Here the parse succeeded and the count is correct — exactly one value group — and only the offsets drifted. Setting it to 0 would record something false, and would change the beta RowBinary routing of statements whose positions were never used by that path (WriterStatementImpl does not read the values list positions), with no case that can show the difference. Keeping the change limited to the positions that are actually wrong.
| LOG.debug("Values list positions [{}, {}] do not match the original SQL", startPosition, stopPosition); | ||
| stmt.setAssignValuesListStartPosition(-1); | ||
| stmt.setAssignValuesListStopPosition(-1); | ||
| } |
There was a problem hiding this comment.
Fallback batch clear still breaks
High Severity
Discarding mismatched values list positions forces the generic PreparedStatementImpl batch path, where batchValues is an immutable empty list and clearBatch always calls clear on it. After a successful executeBatch, that throws UnsupportedOperationException, so the statements this PR aims to fix still cannot complete a normal prepare → addBatch → executeBatch flow. The new integration test stops at addBatch and does not catch this.
Reviewed by Cursor Bugbot for commit 8ac80e0. Configure here.
Collaborator
Author
There was a problem hiding this comment.
The UnsupportedOperationException does not happen: Collections.emptyList() returns a Collections.EmptyList, which does not override clear(). The inherited AbstractList.clear() calls removeRange(0, 0), which removes nothing and returns normally — remove(), the operation EmptyList refuses, is never reached.
List<StringBuilder> l = Collections.emptyList();
l.clear(); // returns, size = 0The point about the test stopping at addBatch was fair, so I extended the coverage in ccbc639: PreparedStatementTest#testBatchInsertWithRewrittenValuesList runs the whole prepare → addBatch × 2 → executeBatch → clearBatch flow on INSERT INTO t (s, n) VALUES (toDateTime({ts '2024-01-01 00:00:00'}), ?) — a statement whose positions this PR discards — and reads the two inserted rows back. It passes (PreparedStatementTest 82/82 green against a live server), so the generic substitution path completes the flow.
The rows of insertWithRewrittenValuesListDP stay at addBatch, on purpose: {d:Int32} is a ClickHouse query parameter that is never bound, so the server rejects those statements for reasons that have nothing to do with the client-side offsets under test.
Leaving the thread open for a reviewer to close.
The test of a values list the token manager rewrites stopped at addBatch, so it did not show that the generic substitution path the statement now falls back to also executes and clears the batch. Run the whole prepare, addBatch, executeBatch, clearBatch flow and read the inserted rows back. Also restore the two batch reuse assertions this branch had dropped: an executeBatch of invalid batch data throws, and it throws again while the batch data is not cleared.
1 task
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 #3017.
The JavaCC token manager builds a second copy of the statement from the token stream (
builderinClickHouseSqlParser.jj) and records keyword positions into it (addCustomKeywordPosition→builder.lastIndexOf(...)). That rebuilt SQL is not always character-identical to the input: semicolons are dropped and JDBC escape sequences ({d '...'},{ts '...'},{t '...'},{tt '...'}) are rewritten to ClickHouse expressions of a different length — or dropped entirely when the lexer treats their content as invalid, which also swallows ClickHouse query parameters whose name starts withd/t(e.g.{d:Int32}).PreparedStatementImpl, however, slices the original SQL with those offsets (originalSql.substring(start, stop + 1)) and maps parameter offsets — scanned from the original SQL — into that slice. When the rebuilt SQL has drifted, the slice is taken at the wrong offsets: if the rewrite grew the statement,Connection#prepareStatementthrowsStringIndexOutOfBoundsException; if it shrank it, the values list template is silently truncated andPreparedStatement#addBatchthrowsStringIndexOutOfBoundsExceptionbecause the parameter offset falls outside the template. Both are unchecked exceptions escaping the JDBC API.The
ANTLR4andANTLR4_PARAMS_PARSERbackends use parse-tree indices into the original SQL and are not affected.The values list positions are a coordinate contract between the parser adapter and the driver, so the fix is applied where the two coordinate systems meet:
JavaCCParser#parsePreparedStatementnow verifies the recorded positions against the original SQL and discards them when they do not delimit its values list. With the positions unset,PreparedStatementImpluses its generic per-statement substitution path (buildSQL()), which is original-SQL based and correct, so such statements are prepared without error. (Applying the escape sequence itself is a separate concern — the statement is still sent to the server as written.)Changes
jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/SqlParserFacade.java: addeddiscardValuesListPositionsNotMatchingOriginalSql(...), called at the end ofJavaCCParser#parsePreparedStatement. The positions are kept only when they address the original SQL: the start is an opening parenthesis, the matching closing parenthesis (quoted text skipped, so a)inside a string literal does not count) falls exactly on the recorded stop, and every parameter position lies inside the range. Otherwise both are reset to-1, the "unset" value every consumer already checks for, and the reason is logged atDEBUG.CHANGELOG.md: entry under0.11.0-rc1→ Bug Fixes.No public API or configuration change. No behavior change for a statement whose values list positions already addressed the original SQL — the overwhelming majority, including statements with comments, newlines, quoted identifiers, a trailing semicolon, and length-neutral escape sequences.
Test
BaseSqlParserFacadeTest#testValuesListPositions(new,@DataProvider, runs for all three parser backends): whenever the reported values list satisfies the preconditionPreparedStatementImplslices on (one values group, both positions set), the positions must address the original SQL — start on(, stop on)within the statement, and all parameters inside. For the JavaCC backend it additionally pins which statements keep their positions and which have them discarded. The rows cover a plain insert, a trailing semicolon, a)inside a string literal, a length-neutral escape ({d '2024-01-01'}→date'2024-01-01', which must keep working), a growing escape ({ts '...'}), a shrinking escape ({tt '...'}), a whitespace-padded escape that shrinks by a single character (which lands the stop on the inner function parenthesis and is only caught by matching the parenthesis, not by acharAt(stop) == ')'check), and a swallowed ClickHouse query parameter ({d:Int32}).Without the fix, 5 of these rows fail on the JavaCC backend; the ANTLR4 backends are unaffected in both directions.
PreparedStatementTest#testInsertWithRewrittenValuesList(new, integration,@DataProvider): prepares such anINSERT ... VALUESthrough a realConnection, checks the parameter count, sets the parameter and callsaddBatch(). Without the fix all three rows fail withStringIndexOutOfBoundsException— two fromprepareStatement, one fromaddBatch— which is the real entry point users hit.Verified:
mvn -pl jdbc-v2 test→ 1325 tests, 0 failures (1316 before, 1321 on the pre-existing code plus the new rows).mvn -pl jdbc-v2 -DskipUTs=true -Dit.test=PreparedStatementTest verify→ 74 tests, 0 failures.docs/changes_checklist.mdLOG.debug(...)on a path that was previously silent; no new logger, no level change, no user data logged (positions only).privateon the package-privateJavaCCParser.docs/features.mdunchanged — no feature added, removed, or intentionally changed.Pre-PR validation gate
main, pass on this branch)AGENTS.mdanddocs/changes_checklist.mdJAVACCis the defaultjdbc_sql_parser; pinned throughConnection#prepareStatement)