From 70a047015b8a9aaa4279c14594cebc390562db3d Mon Sep 17 00:00:00 2001 From: Polyglot AI <293096396+polyglotAI-bot@users.noreply.github.com> Date: Thu, 1 Oct 2026 05:35:17 +0000 Subject: [PATCH] Fix jdbc-v2: use the RowBinary writer only for a values list of placeholders 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: https://github.com/ClickHouse/clickhouse-java/issues/3083 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 7 +++ .../com/clickhouse/jdbc/ConnectionImpl.java | 1 + .../jdbc/internal/SqlParserFacade.java | 8 ++- .../jdbc/WriterStatementImplTest.java | 58 +++++++++++++++++++ .../internal/BaseSqlParserFacadeTest.java | 12 ++++ 5 files changed, 85 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ecea0c4f7..1b2dcf09b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -180,6 +180,13 @@ now required to work with server. (https://github.com/ClickHouse/clickhouse-java ### Bug Fixes +- **[jdbc-v2]** Fixed an `INSERT ... VALUES` statement whose values list holds a literal, an expression (e.g. `? + 1`) + or, with the `JAVACC` parser, a JDBC escape sequence or a query parameter, being written with the RowBinary writer + when `beta.row_binary_for_simple_insert` is enabled. The writer takes one bound value per column, so the bound values + were shifted to other columns: the statement failed with a misleading error, or stored wrong data with no error. The + parsers did not report such values: the `JAVACC` parser discarded the result of its values-list check, and the + `ANTLR4` parsers reported only function calls. Now the writer is used only for a values list of `?` placeholders, + and other statements use the standard `PreparedStatement` path. (https://github.com/ClickHouse/clickhouse-java/issues/3083) - **[client-v2]** Fixed writing a `String` value into a `UUID` column (also as an `Array`/`Tuple` element or a `Map` key) failing with `ClassCastException`. The string is now parsed with `UUID.fromString`; a string it cannot parse is rejected on the client with an `IllegalArgumentException`. (https://github.com/ClickHouse/clickhouse-java/issues/3132) diff --git a/jdbc-v2/src/main/java/com/clickhouse/jdbc/ConnectionImpl.java b/jdbc-v2/src/main/java/com/clickhouse/jdbc/ConnectionImpl.java index f947f7b70..ff7e1a8fe 100644 --- a/jdbc-v2/src/main/java/com/clickhouse/jdbc/ConnectionImpl.java +++ b/jdbc-v2/src/main/java/com/clickhouse/jdbc/ConnectionImpl.java @@ -473,6 +473,7 @@ public PreparedStatement prepareStatement(String sql, int resultSetType, int res * - number of arguments matches schema or column list * RowBinary cannot be used when * - INSERT INTO t VALUES (now(), ?, ?) !# there is a function in the values + * - INSERT INTO t VALUES (1, ?, ? + 1) !# there is a literal or an expression in the values * - INSERT INTO t VALUES (now(), ?, 1), (now(), ?, 2) !# multiple values list * - INSERT INTO t SELECT ?, ?, ? !# insert from select * - INSERT INTO [TABLE] FUNCTION f(...) VALUES (?) !# the target is a table function 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 c19871ebb..9b5169604 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 @@ -122,7 +122,7 @@ public ParsedPreparedStatement parsePreparedStatement(String sql) { } } - stmt.setUseFunction(parsedStmt.isFuncUsed()); + stmt.setUseFunction(stmt.isUseFunction() || parsedStmt.isFuncUsed()); parseParameters(sql, stmt); discardValuesListPositionsNotMatchingOriginalSql(sql, stmt); return stmt; @@ -435,6 +435,12 @@ public void enterInsertParameterFuncExpr(ClickHouseParser.InsertParameterFuncExp public void enterAssignmentValuesList(ClickHouseParser.AssignmentValuesListContext ctx) { parsedStatement.setAssignValuesListStartPosition(ctx.getStart().getStartIndex()); parsedStatement.setAssignValuesListStopPosition(ctx.getStop().getStopIndex()); + for (ClickHouseParser.AssignmentValueContext value : ctx.assignmentValue()) { + if (!(value instanceof ClickHouseParser.InsertParameterContext)) { + parsedStatement.setUseFunction(true); + break; + } + } } @Override diff --git a/jdbc-v2/src/test/java/com/clickhouse/jdbc/WriterStatementImplTest.java b/jdbc-v2/src/test/java/com/clickhouse/jdbc/WriterStatementImplTest.java index 4959a3b6f..8980653b1 100644 --- a/jdbc-v2/src/test/java/com/clickhouse/jdbc/WriterStatementImplTest.java +++ b/jdbc-v2/src/test/java/com/clickhouse/jdbc/WriterStatementImplTest.java @@ -369,6 +369,64 @@ public void testInsertWithUnparseableFunctionNotWrittenWithRowBinary(String pars } } + @DataProvider(name = "insertValuesListForms") + Object[][] insertValuesListForms() { + Object[][] statements = { + {"INSERT INTO %s (a, b, c) VALUES (?, ?, ?)", new int[]{7, 20, 30}, true}, + {"INSERT INTO %s (a, b, c) VALUES (7, ?, ?)", new int[]{20, 30}, false}, + {"INSERT INTO %s (a, b, c) VALUES (?, 20, ?)", new int[]{7, 30}, false}, + {"INSERT INTO %s (a, b, c) VALUES (?, ? + 1, ?)", new int[]{7, 19, 30}, false}, + {"INSERT INTO %s (a, b, c) VALUES ({fn ABS(-7)}, ?, ?)", new int[]{20, 30}, false}, + }; + List rows = new ArrayList<>(); + for (SqlParserFacade.SQLParser parser : SqlParserFacade.SQLParser.values()) { + for (Object[] statement : statements) { + rows.add(new Object[]{parser.name(), statement[0], statement[1], statement[2]}); + } + } + return rows.toArray(new Object[0][]); + } + + @Test(groups = {"integration"}, dataProvider = "insertValuesListForms") + public void testInsertUsesWriterOnlyForValuesListOfPlaceholders(String parser, String sqlTemplate, int[] params, + boolean expectWriter) throws SQLException { + String table = "bt_writer_values_list_" + UUID.randomUUID().toString().replace('-', '_'); + Properties properties = new Properties(); + properties.setProperty(DriverProperties.BETA_ROW_BINARY_WRITER.getKey(), "true"); + properties.setProperty(DriverProperties.SQL_PARSER.getKey(), parser); + properties.setProperty(ASYNC_INSERT_SETTING_KEY, ServerSettings.OFF); + try (Connection connection = getJdbcConnection(properties)) { + try (Statement stmt = connection.createStatement()) { + stmt.execute("CREATE TABLE " + table + " (a Int32, b Int32, c Nullable(Int32)) Engine MergeTree ORDER BY ()"); + } + + try { + String sql = String.format(sqlTemplate, table); + try (PreparedStatement ps = connection.prepareStatement(sql)) { + Assert.assertEquals(ps instanceof WriterStatementImpl, expectWriter, + "Only a values list of placeholders must use the RowBinary writer: " + sql); + for (int i = 0; i < params.length; i++) { + ps.setInt(i + 1, params[i]); + } + Assert.assertEquals(ps.executeUpdate(), 1); + } + + try (Statement stmt = connection.createStatement(); + ResultSet rs = stmt.executeQuery("SELECT a, b, c FROM " + table)) { + Assert.assertTrue(rs.next()); + Assert.assertEquals(rs.getInt("a"), 7); + Assert.assertEquals(rs.getInt("b"), 20); + Assert.assertEquals(rs.getObject("c"), 30); + Assert.assertFalse(rs.next()); + } + } finally { + try (Statement stmt = connection.createStatement()) { + stmt.execute("DROP TABLE IF EXISTS " + table); + } + } + } + } + private static boolean hasInjectedCause(Throwable t) { for (Throwable c = t; c != null; c = c.getCause()) { if (c instanceof IOException && "injected buffer close failure".equals(c.getMessage())) { 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 2096cc62d..1ce748db0 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 @@ -1610,6 +1610,7 @@ public static Object[][] testInsertUseFunctionDP() { /* values list of placeholders only */ {"INSERT INTO t (v1, v2) VALUES (?, ?)", false, true}, {"INSERT INTO t VALUES (?, ?)", false, true}, + {"INSERT INTO t (v1, v2) VALUES (\n ?,\t? )", false, true}, /* function call the grammar matches */ {"INSERT INTO t (v1, v2) VALUES (?, now())", true, true}, {"INSERT INTO t (v1, v2) VALUES (toString(?), ?)", true, true}, @@ -1617,6 +1618,17 @@ public static Object[][] testInsertUseFunctionDP() { {"INSERT INTO t (v1, v2) VALUES (?, hex(x'AB'))", true, false}, {"INSERT INTO t (v1, v2) VALUES (hex(x'AB'), ?)", true, false}, {"INSERT INTO t (v1, v2) VALUES (?, ?), (?, hex(x'AB'))", true, false}, + /* values list holding a value that is not a placeholder nor a function call */ + {"INSERT INTO t (v1, v2, v3) VALUES (7, ?, ?)", true, true}, + {"INSERT INTO t (v1, v2, v3) VALUES (?, ?, 7)", true, true}, + {"INSERT INTO t (v1, v2) VALUES ('2020-01-01 00:00:00', ?)", true, true}, + {"INSERT INTO t (v1, v2) VALUES (NULL, ?)", true, true}, + {"INSERT INTO t (v1, v2) VALUES (-7, ?)", true, true}, + {"INSERT INTO t (v1, v2) VALUES (? + 1, ?)", true, true}, + {"INSERT INTO t (v1, v2) VALUES (?::Int32, ?)", true, true}, + {"INSERT INTO t (v1, v2) VALUES ([?, ?], ?)", true, true}, + {"INSERT INTO t (v1, v2) VALUES ({fn now()}, ?)", true, false}, + {"INSERT INTO t (v1, v2) VALUES ({p1:DateTime}, ?)", true, false}, }; }