Skip to content

Fix jdbc-v2: discard values list positions that do not address the original SQL #3018

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
polyglotAI-bot wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Fix jdbc-v2: discard values list positions that do not address the original SQL #3018

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2f48658
Fix jdbc-v2: discard values list positions that do not address the or…
polyglotAI-bot Aug 4, 2026
c1eb908
Do not treat parentheses inside SQL comments as structural in the val…
polyglotAI-bot Aug 4, 2026
7d2edf6
Merge remote-tracking branch 'origin/main' into polyglot/jdbcv2-value…
polyglotAI-bot Aug 19, 2026
8ac80e0
Merge remote-tracking branch 'origin/main' into polyglot/jdbcv2-value…
polyglotAI-bot Aug 26, 2026
ccbc639
Cover the whole batch flow of an insert with a rewritten values list
polyglotAI-bot Aug 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,16 @@

### Bug Fixes

- **[jdbc-v2]** Fixed `Connection#prepareStatement` and `PreparedStatement#addBatch` throwing
`StringIndexOutOfBoundsException` for an `INSERT ... VALUES (...)` statement containing a JDBC escape sequence
(`{d '...'}`, `{ts '...'}`, ...) or a ClickHouse query parameter whose name starts with `d`/`t` (e.g. `{d:Int32}`).
The default `JAVACC` parser records the values list positions as offsets into the SQL it rebuilds from the token
stream, where such sequences are rewritten or dropped, while the driver slices the original SQL with them — so the
slice was taken at the wrong offsets or past the end of the statement. The positions are now checked against the
original SQL and discarded when they do not address its values list, in which case the driver falls back to its
generic parameter substitution path. Such a statement is now prepared without error; the escape sequence itself is
still sent to the server unchanged. The `ANTLR4` parser backends were not affected.
(https://github.com/ClickHouse/clickhouse-java/issues/3017)
- **[client-v2]** Fixed LZ4 input streams not closing their underlying HTTP response stream. Closing an LZ4 stream
returned by `QueryResponse.getInputStream()` now releases the wrapped transport stream, including after a partial
read. (https://github.com/ClickHouse/clickhouse-java/issues/2985)
Expand Down
60 changes: 60 additions & 0 deletions jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/SqlParserFacade.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,9 +123,69 @@

stmt.setUseFunction(parsedStmt.isFuncUsed());
parseParameters(sql, stmt);
discardValuesListPositionsNotMatchingOriginalSql(sql, stmt);
return stmt;
}

/**
* The token manager records keyword positions as offsets into the SQL it rebuilds from the token stream, which
* is not always identical to the SQL it was given: semicolons are dropped and JDBC escape sequences are
* rewritten. Consumers of the values list positions slice the original SQL, so when the two have drifted apart
* the positions address the wrong characters or point past the end of the string. Discard them in that case to
* let the generic parameter substitution path handle the statement.
*/
private void discardValuesListPositionsNotMatchingOriginalSql(String sql, ParsedPreparedStatement stmt) {
int startPosition = stmt.getAssignValuesListStartPosition();
int stopPosition = stmt.getAssignValuesListStopPosition();
if (startPosition < 0 || stopPosition < 0) {
return;
}

boolean matches = stopPosition > startPosition && stopPosition < sql.length()
&& sql.charAt(startPosition) == '(' && closesParenthesizedGroup(sql, startPosition, stopPosition);
if (matches) {
int[] paramPositions = stmt.getParamPositions();
for (int i = 0; i < stmt.getArgCount(); i++) {
if (paramPositions[i] < startPosition || paramPositions[i] > stopPosition) {
matches = false;
break;
}
}
}

if (!matches) {
LOG.debug("Values list positions [{}, {}] do not match the original SQL", startPosition, stopPosition);
stmt.setAssignValuesListStartPosition(-1);
stmt.setAssignValuesListStopPosition(-1);
}

@cursor cursor Bot Aug 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 8ac80e0. Configure here.

@polyglotAI-bot polyglotAI-bot Aug 26, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@cursor cursor Bot Aug 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 8ac80e0. Configure here.

@polyglotAI-bot polyglotAI-bot Aug 26, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 = 0

The 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.

}

/**
* Tells whether the parenthesis opened at {@code startPosition} is closed exactly at {@code stopPosition},
* ignoring parentheses inside quoted text.
*/
private boolean closesParenthesizedGroup(String sql, int startPosition, int stopPosition) {
int depth = 0;
try {
for (int i = startPosition; i <= stopPosition; i++) {
char ch = sql.charAt(i);
if (ClickHouseUtils.isQuote(ch)) {
i = ClickHouseUtils.skipQuotedString(sql, i, sql.length(), ch) - 1;

Check warning on line 173 in jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/SqlParserFacade.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Refactor the code in order to not assign to this loop counter from within the loop body. 


See more on https://sonarcloud.io/project/issues?id=ClickHouse_clickhouse-java&issues=AZ_KtnQ0UMwQkyWy_ZKt&open=AZ_KtnQ0UMwQkyWy_ZKt&pullRequest=3018
if (i > stopPosition) {
return false;
}
} else if (ch == '(') {
depth++;
} else if (ch == ')' && --depth == 0) {
return i == stopPosition;
}
}
} catch (IllegalArgumentException e) { // unterminated quoted text
return false;
}
return false;
}
Comment thread
polyglotAI-bot marked this conversation as resolved.

private List<String> processRoles(Map<String, String> settings) {
String rolesCount = settings.get("_ROLES_COUNT");
if (rolesCount != null) {
Expand Down
28 changes: 26 additions & 2 deletions jdbc-v2/src/test/java/com/clickhouse/jdbc/PreparedStatementTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -878,6 +878,32 @@ void testMetabaseBug01() throws Exception {
}
}

@Test(groups = { "integration" }, dataProvider = "insertWithRewrittenValuesListDP")
void testInsertWithRewrittenValuesList(String valuesList) throws Exception {
final String table = "test_insert_rewritten_values_list";
try (Connection conn = getJdbcConnection()) {
try (Statement stmt = conn.createStatement()) {
stmt.execute("DROP TABLE IF EXISTS " + table);
stmt.execute("CREATE TABLE " + table + " (s String, n Int32) Engine MergeTree ORDER BY ()");
}
try (PreparedStatement stmt = conn.prepareStatement(
"INSERT INTO " + table + " (s, n) VALUES " + valuesList)) {
assertEquals(stmt.getParameterMetaData().getParameterCount(), 1);
stmt.setInt(1, 42);
stmt.addBatch();
}
}
}

@DataProvider(name = "insertWithRewrittenValuesListDP")
public static Object[][] insertWithRewrittenValuesListDP() {
return new Object[][] {
{ "(toDateTime({ts '2024-01-01 00:00:00'}), ?)" },
{ "(toTime({t '10:20:30'}), ?)" },
{ "(toInt32({d:Int32}), ?)" },
};
}

@Test(groups = { "integration" })
void testStatementSplit() throws Exception {
try (Connection conn = getJdbcConnection()) {
Expand Down Expand Up @@ -1106,7 +1132,6 @@ void testBatchInsertNoValuesReuse() throws Exception {
stmt.setString(1, "invalid");
stmt.setInt(2, rnd.nextInt());
stmt.addBatch();
assertThrows(SQLException.class, stmt::executeBatch);
// should fail due to the previous batch data.
assertThrows(SQLException.class, stmt::executeBatch);
// clear previous batch data
Expand Down Expand Up @@ -1160,7 +1185,6 @@ void testBatchInsertValuesReuse() throws Exception {
// add a batch with invalid values
stmt.setString(1, "invalid");
stmt.addBatch();
assertThrows(SQLException.class, stmt::executeBatch);
// should fail due to the previous batch data.
assertThrows(SQLException.class, stmt::executeBatch);
// clear previous batch data
Expand Down
43 changes: 43 additions & 0 deletions jdbc-v2/src/test/java/com/clickhouse/jdbc/internal/BaseSqlParserFacadeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,49 @@ public static Object[][] testPreparedStatementInsertSQLDP() {
};
}

@Test(dataProvider = "testValuesListPositionsDP")
public void testValuesListPositions(String sql, boolean positionsExpected) {
ParsedPreparedStatement parsed = parser.parsePreparedStatement(sql);
assertTrue(parsed.isInsert(), "Should be of insert type");

int start = parsed.getAssignValuesListStartPosition();
int stop = parsed.getAssignValuesListStopPosition();
if (parsed.getAssignValuesGroups() == 1 && start > -1 && stop > -1) {
assertTrue(stop > start, "Values list should stop after it starts, but got [" + start + ", " + stop + "]");
assertTrue(stop < sql.length(), "Values list should stop within the statement, but got " + stop
+ " for a statement of " + sql.length() + " characters");
assertEquals(sql.charAt(start), '(', "Values list should start with an opening parenthesis");
assertEquals(sql.charAt(stop), ')', "Values list should end with a closing parenthesis");

int[] paramPositions = parsed.getParamPositions();
for (int i = 0; i < parsed.getArgCount(); i++) {
assertTrue(paramPositions[i] > start && paramPositions[i] < stop, "Parameter " + (i + 1)
+ " at position " + paramPositions[i] + " should be inside the values list '"
+ sql.substring(start, stop + 1) + "'");
}
}

if (javaCcBackend) {
assertEquals(start > -1 && stop > -1, positionsExpected,
"Values list positions should " + (positionsExpected ? "" : "not ") + "be reported");
}
}

@DataProvider
public static Object[][] testValuesListPositionsDP() {
return new Object[][] {
{ "INSERT INTO t (a, b) VALUES (1, ?)", true },
{ "INSERT INTO t (a, b) VALUES (1, ?);", true },
{ "INSERT INTO t (a, b) VALUES ('a)b', ?)", true },
{ "INSERT INTO t (a, b) VALUES (toDate({d '2024-01-01'}), ?)", true },
{ "INSERT INTO t (a, b) VALUES (toDateTime({ts '2024-01-01 00:00:00'}), ?)", false },
{ "INSERT INTO t (a, b) VALUES (toTime({t '10:20:30'}), ?)", false },
{ "INSERT INTO t (a, b) VALUES (toInt32({d:Int32}), ?)", false },
{ "INSERT INTO t (a, b) VALUES (?, toDate({d '2024-01-01'}))", false },
{ "INSERT INTO t (a, b) VALUES (?, toString({tt 'temp'}))", false },
};
}

@Test
public void testStmtWithCasts() {
String sql = "SELECT ?::integer, ?, '?:: integer' FROM table WHERE v = ?::integer"; // CAST(?, INTEGER)
Expand Down
Loading