Fix jdbc-v2: use the RowBinary writer only for a values list of placeholders

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,jdbc-v2]** - Replaced slow HTTP LZ4 compression with compressing stream from 
 `lz4-java` library. Client uses Apache Compress to handle HTTP compression (because it has 
 convenient factory for many compressions methods. However, Apache Compress uses slow LZ4 implementation what causes very slow inserts. Now `lz4-java` used for insert. Query still slow but will be fix in future releases (need refactoring and custom implementation to solve it). Using `ZSTD` for queries should solve the issue.(https://github.com/ClickHouse/clickhouse-java/issues/2273). 

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

jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/SqlParserFacade.java

@@ -82,7 +82,7 @@
         }
 
         @Override
         public ParsedPreparedStatement parsePreparedStatement(String sql) {
             ParsedPreparedStatement stmt = new ParsedPreparedStatement();
             ClickHouseSqlStatement parsedStmt = parse(sql);
             if (parsedStmt.getStatementType() == StatementType.USE) {
@@ -122,7 +122,7 @@
                 }
             }
 
-            stmt.setUseFunction(parsedStmt.isFuncUsed());
+            stmt.setUseFunction(stmt.isUseFunction() || parsedStmt.isFuncUsed());
             parseParameters(sql, stmt);
             discardValuesListPositionsNotMatchingOriginalSql(sql, stmt);
             return stmt;
@@ -435,6 +435,12 @@
             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

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<Object[]> 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())) {

jdbc-v2/src/test/java/com/clickhouse/jdbc/internal/BaseSqlParserFacadeTest.java

@@ -1610,13 +1610,25 @@ 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},
                 /* function call with an argument the grammar cannot match */
                 {"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},
         };
     }