From 70642b2539b3d80695680ee667ad198553d3717d Mon Sep 17 00:00:00 2001 From: Polyglot AI <293096396+polyglotAI-bot@users.noreply.github.com> Date: Tue, 4 Aug 2026 03:19:46 +0000 Subject: [PATCH] Fix jdbc-v2: discard the values list of a recovered ANTLR4 parse tree 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: https://github.com/ClickHouse/clickhouse-java/issues/3019 --- CHANGELOG.md | 11 ++++ .../jdbc/internal/SqlParserFacade.java | 16 +++++ .../jdbc/PreparedStatementTest.java | 43 ++++++++++++ .../internal/BaseSqlParserFacadeTest.java | 66 +++++++++++++++++++ 4 files changed, 136 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 52c92a57a..1ef94ea7c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,17 @@ ### Bug Fixes +- **[jdbc-v2]** Fixed `PreparedStatement#executeBatch` sending a syntactically broken `INSERT` when an `ANTLR4` parser + backend is selected (`jdbc_sql_parser=ANTLR4` / `ANTLR4_PARAMS_PARSER`) and the values list contains a value + expression the bundled grammar cannot parse - a JDBC escape sequence (`{d '...'}`), or valid ClickHouse syntax the + grammar does not cover such as a hex string literal (`hex(x'AB')`). Such a statement is still given a parse tree, + completed by error recovery, and the values list positions and the value group count were read from it: the values + list was reported to stop at the closing parenthesis of a nested function call, so the batch template lost its own + closing parenthesis, and a two-group values list could be reported as a single group. Both are now discarded when the + statement could not be parsed without errors, so the driver uses its generic parameter substitution path instead - and, + with the beta `RowBinary` writer enabled, such a statement is no longer routed to it. The default `JAVACC` backend is + not affected by this. + (https://github.com/ClickHouse/clickhouse-java/issues/3019) - **[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) diff --git a/jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/SqlParserFacade.java b/jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/SqlParserFacade.java index 178c9a070..f3a912076 100644 --- a/jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/SqlParserFacade.java +++ b/jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/SqlParserFacade.java @@ -182,6 +182,7 @@ public ParsedPreparedStatement parsePreparedStatement(String sql) { parseSQL(sql, new ParsedPreparedStatementListener(stmt, processUseRolesExpr)); if (stmt.isHasErrors()) { stmt.setHasResultSet(true); + discardValuesListOfRecoveredParseTree(stmt); } // Combine database and table like JavaCC does String tableName = stmt.getTable(); @@ -194,6 +195,20 @@ public ParsedPreparedStatement parsePreparedStatement(String sql) { return stmt; } + /** + * A statement the grammar cannot match is still given a parse tree, completed by error recovery: a rule context + * then ends at the token the parser recovered on rather than at the token the rule requires. The values list + * positions and the value group count are read from such contexts, so they may address only a part of the + * values list or report a wrong number of groups. Discard them, so that consumers slicing the statement with + * them use the generic parameter substitution path instead. + */ + static void discardValuesListOfRecoveredParseTree(ParsedPreparedStatement stmt) { + LOG.debug("Discarding values list of a statement that could not be parsed without errors"); + stmt.setAssignValuesListStartPosition(-1); + stmt.setAssignValuesListStopPosition(-1); + stmt.setAssignValuesGroups(0); + } + protected ClickHouseParser parseSQL(String sql, ClickHouseParserBaseListener listener) { CharStream charStream = CharStreams.fromString(sql); ClickHouseLexer lexer = new ClickHouseLexer(charStream); @@ -415,6 +430,7 @@ public ParsedPreparedStatement parsePreparedStatement(String sql) { parseSQL(sql, new ParseStatementAndParamsListener(stmt, processUseRolesExpr)); if (stmt.isHasErrors()) { stmt.setHasResultSet(true); + discardValuesListOfRecoveredParseTree(stmt); } // Combine database and table like JavaCC does String tableName = stmt.getTable(); diff --git a/jdbc-v2/src/test/java/com/clickhouse/jdbc/PreparedStatementTest.java b/jdbc-v2/src/test/java/com/clickhouse/jdbc/PreparedStatementTest.java index 14c19f7a9..4fc1aae9d 100644 --- a/jdbc-v2/src/test/java/com/clickhouse/jdbc/PreparedStatementTest.java +++ b/jdbc-v2/src/test/java/com/clickhouse/jdbc/PreparedStatementTest.java @@ -7,6 +7,7 @@ import com.clickhouse.data.ClickHouseVersion; import com.clickhouse.data.Tuple; import com.clickhouse.jdbc.internal.JdbcUtils; +import com.clickhouse.jdbc.internal.SqlParserFacade; import org.apache.commons.lang3.RandomStringUtils; import org.testng.Assert; import org.testng.annotations.DataProvider; @@ -1194,6 +1195,48 @@ void testBatchInsertValuesReuse() throws Exception { } } + @Test(groups = {"integration"}, dataProvider = "sqlParserDP") + void testBatchInsertWithValueOfUnsupportedSyntax(String parserName) throws Exception { + String table = "test_pstmt_batch_unsupported_syntax"; + Properties properties = new Properties(); + properties.setProperty(DriverProperties.SQL_PARSER.getKey(), parserName); + properties.setProperty(ASYNC_INSERT_SETTING_KEY, ServerSettings.OFF); + try (Connection conn = getJdbcConnection(properties)) { + try (Statement stmt = conn.createStatement()) { + stmt.execute("DROP TABLE IF EXISTS " + table); + stmt.execute("CREATE TABLE " + table + " (v1 Int32, v2 String) Engine MergeTree ORDER BY ()"); + } + + try (PreparedStatement stmt = conn.prepareStatement( + "INSERT INTO " + table + " (v1, v2) VALUES (?, hex(x'AB'))")) { + for (int i = 1; i <= 2; i++) { + stmt.setInt(1, i); + stmt.addBatch(); + } + assertEquals(stmt.executeBatch().length, 2); + } + + try (Statement stmt = conn.createStatement(); + ResultSet rs = stmt.executeQuery("SELECT v1, v2 FROM " + table + " ORDER BY v1")) { + for (int i = 1; i <= 2; i++) { + assertTrue(rs.next()); + assertEquals(rs.getInt(1), i); + assertEquals(rs.getString(2), "AB"); + } + assertFalse(rs.next()); + } + } + } + + @DataProvider(name = "sqlParserDP") + public static Object[][] sqlParserDP() { + return new Object[][] { + { SqlParserFacade.SQLParser.JAVACC.name() }, + { SqlParserFacade.SQLParser.ANTLR4.name() }, + { SqlParserFacade.SQLParser.ANTLR4_PARAMS_PARSER.name() }, + }; + } + @Test(groups = {"integration"}) void testWriteUUID() throws Exception { String sql = "insert into `test_issue_2327` (`id`, `uuid`) values (?, ?)"; diff --git a/jdbc-v2/src/test/java/com/clickhouse/jdbc/internal/BaseSqlParserFacadeTest.java b/jdbc-v2/src/test/java/com/clickhouse/jdbc/internal/BaseSqlParserFacadeTest.java index 945701ad0..850a975f5 100644 --- a/jdbc-v2/src/test/java/com/clickhouse/jdbc/internal/BaseSqlParserFacadeTest.java +++ b/jdbc-v2/src/test/java/com/clickhouse/jdbc/internal/BaseSqlParserFacadeTest.java @@ -1,6 +1,7 @@ package com.clickhouse.jdbc.internal; +import com.clickhouse.data.ClickHouseUtils; import com.clickhouse.jdbc.internal.parser.javacc.ClickHouseSqlUtils; import org.testng.Assert; import org.testng.annotations.DataProvider; @@ -17,6 +18,7 @@ import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertFalse; +import static org.testng.Assert.assertNotEquals; import static org.testng.Assert.assertTrue; public abstract class BaseSqlParserFacadeTest { @@ -151,6 +153,70 @@ public static Object[][] testPreparedStatementInsertSQLDP() { }; } + @Test(dataProvider = "testValuesListOfUnsupportedSyntaxDP") + public void testValuesListOfUnsupportedSyntax(String sql, boolean parseable, int valueGroups, int args) { + ParsedPreparedStatement parsed = parser.parsePreparedStatement(sql); + assertTrue(parsed.isInsert(), "Should be of insert type"); + + int reportedValueGroups = parsed.getAssignValuesGroups(); + int start = parsed.getAssignValuesListStartPosition(); + int stop = parsed.getAssignValuesListStopPosition(); + if (parseable) { + assertEquals(reportedValueGroups, valueGroups, "Value groups do not match"); + assertEquals(parsed.getArgCount(), args, "Args do not match"); + if (valueGroups == 1) { + assertTrue(start > -1 && stop > -1, + "Values list positions should be reported, but got [" + start + ", " + stop + "]"); + } + } else if (valueGroups > 1) { + assertNotEquals(reportedValueGroups, 1, "Should not report a single value group"); + } + if (reportedValueGroups != 1 || start < 0 || stop < 0) { + return; // the positions are only used to slice the statement of a single value group + } + + assertTrue(stop > start && stop < sql.length(), "Values list should stop after it starts and within the " + + "statement of " + sql.length() + " characters, but got [" + start + ", " + stop + "]"); + assertEquals(sql.charAt(start), '(', "Values list should start with an opening parenthesis"); + assertEquals(stop, indexOfClosingParenthesis(sql, start), + "Values list should stop where the value group opened at " + start + " is closed"); + + 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) + "'"); + } + } + + @DataProvider + public static Object[][] testValuesListOfUnsupportedSyntaxDP() { + return new Object[][] { + { "INSERT INTO t (a, b) VALUES (?, hex(x'AB'))", false, 1, 1 }, + { "INSERT INTO t (a, b) VALUES (?, toDate({d '2024-01-01'}))", false, 1, 1 }, + { "INSERT INTO t (a, b) VALUES (?, hex(x'AB')), (?, hex(x'CD'))", false, 2, 2 }, + { "INSERT INTO t (a, b) VALUES (?, hex('AB'))", true, 1, 1 }, + { "INSERT INTO t (a, b) VALUES (?, 'a)b')", true, 1, 1 }, + { "INSERT INTO t (a, b) VALUES (?, hex('AB')) ;", true, 1, 1 }, + { "INSERT INTO t (a, b) VALUES (?, ?), (?, ?)", true, 2, 4 }, + }; + } + + private static int indexOfClosingParenthesis(String sql, int openingPosition) { + int depth = 0; + for (int i = openingPosition; i < sql.length(); i++) { + char ch = sql.charAt(i); + if (ClickHouseUtils.isQuote(ch)) { + i = ClickHouseUtils.skipQuotedString(sql, i, sql.length(), ch) - 1; + } else if (ch == '(') { + depth++; + } else if (ch == ')' && --depth == 0) { + return i; + } + } + return -1; + } + @Test public void testStmtWithCasts() { String sql = "SELECT ?::integer, ?, '?:: integer' FROM table WHERE v = ?::integer"; // CAST(?, INTEGER)