Fix jdbc-v2: assume a function in the values list of a recovered ANTLR4 parse tree

CHANGELOG.md

@@ -44,6 +44,17 @@
 
 ### Bug Fixes 
 
+- **[jdbc-v2]** Fixed an `INSERT` whose values list holds a function call the bundled `ANTLR4` grammar cannot match -
+  such as `hex(x'AB')`, valid ClickHouse the grammar has no hex string literal for - being reported to hold no function
+  call when an `ANTLR4` parser backend is selected (`jdbc_sql_parser=ANTLR4` / `ANTLR4_PARAMS_PARSER`). Function calls in
+  a values list are reported by a callback on the parse tree, and such a statement is still given a parse tree, completed
+  by error recovery, which skips the tokens the parser recovered on - the function call among them. With the beta
+  `RowBinary` writer enabled (`beta.row_binary_for_simple_insert=true`) the statement was then routed to it, where a
+  literal function-call column cannot be written; it now takes the generic parameter substitution path, as it already did
+  for a function call the grammar matches. Since such a parse tree cannot tell, any insert that could not be parsed
+  without errors is now assumed to hold a function call in its values list, so none of them is written with the
+  `RowBinary` writer. The default `JAVACC` backend is not affected by this.
+  (https://github.com/ClickHouse/clickhouse-java/issues/3027)
 - **[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)

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

@@ -182,18 +182,35 @@ public ParsedPreparedStatement parsePreparedStatement(String sql) {
             parseSQL(sql, new ParsedPreparedStatementListener(stmt, processUseRolesExpr));
             if (stmt.isHasErrors()) {
                 stmt.setHasResultSet(true);
+                assumeFunctionInValuesListOfRecoveredParseTree(stmt);
             }
             // Combine database and table like JavaCC does
             String tableName = stmt.getTable();
             if (stmt.getDatabase() != null && stmt.getTable() != null) {
                 tableName = String.format("%s.%s", stmt.getDatabase(), stmt.getTable());
             }
             stmt.setTable(tableName);
-            
+
             parseParameters(sql, stmt);
             return stmt;
         }
 
+        /**
+         * A function call in an insert values list is reported by a listener callback on the parse tree. A statement
+         * the grammar cannot match is still given a parse tree, completed by error recovery, which skips the tokens
+         * the parser recovered on - a function call among them is never reported, so the values list is reported to
+         * hold no function call while it does. Since such a tree cannot tell, assume a function call is present, so
+         * that a consumer requiring a values list of parameter placeholders only - the RowBinary insert path - does
+         * not take the statement.
+         */
+        static void assumeFunctionInValuesListOfRecoveredParseTree(ParsedPreparedStatement stmt) {
+            if (stmt.isInsert()) {
+                LOG.debug("Assuming a function in the values list of an insert into {} that could not be parsed without errors",
+                        stmt.getTable());
+                stmt.setUseFunction(true);
+            }
+        }
+
         protected ClickHouseParser parseSQL(String sql, ClickHouseParserBaseListener listener) {
             CharStream charStream = CharStreams.fromString(sql);
             ClickHouseLexer lexer = new ClickHouseLexer(charStream);
@@ -415,6 +432,7 @@ public ParsedPreparedStatement parsePreparedStatement(String sql) {
             parseSQL(sql, new ParseStatementAndParamsListener(stmt, processUseRolesExpr));
             if (stmt.isHasErrors()) {
                 stmt.setHasResultSet(true);
+                assumeFunctionInValuesListOfRecoveredParseTree(stmt);
             }
             // Combine database and table like JavaCC does
             String tableName = stmt.getTable();

jdbc-v2/src/test/java/com/clickhouse/jdbc/WriterStatementImplTest.java

@@ -1,6 +1,7 @@
 package com.clickhouse.jdbc;
 
 import com.clickhouse.client.api.internal.ServerSettings;
+import com.clickhouse.jdbc.internal.SqlParserFacade;
 import org.testng.Assert;
 import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
@@ -138,6 +139,60 @@ public void close() throws IOException {
         }
     }
 
+    @DataProvider(name = "antlr4ParserBackends")
+    Object[][] antlr4ParserBackends() {
+        return new Object[][]{
+                {SqlParserFacade.SQLParser.ANTLR4.name()},
+                {SqlParserFacade.SQLParser.ANTLR4_PARAMS_PARSER.name()},
+        };
+    }
+
+    @Test(groups = {"integration"}, dataProvider = "antlr4ParserBackends")
+    public void testInsertWithUnparseableFunctionNotWrittenWithRowBinary(String parserName) throws SQLException {
+        String table = "bt_writer_unparseable_function";
+        Properties properties = new Properties();
+        properties.setProperty(DriverProperties.BETA_ROW_BINARY_WRITER.getKey(), "true");
+        properties.setProperty(DriverProperties.SQL_PARSER.getKey(), parserName);
+        properties.setProperty(ASYNC_INSERT_SETTING_KEY, ServerSettings.OFF);
+        try (Connection connection = getJdbcConnection(properties)) {
+            try (Statement stmt = connection.createStatement()) {
+                stmt.execute("DROP TABLE IF EXISTS " + table);
+                stmt.execute("CREATE TABLE " + table + " (v1 Int32, v2 String) Engine MergeTree ORDER BY ()");
+            }
+
+            try (PreparedStatement ps = connection.prepareStatement(
+                    "INSERT INTO " + table + " (v1, v2) VALUES (?, hex(x'AB'))")) {
+                Assert.assertFalse(ps instanceof WriterStatementImpl,
+                        "An insert with a function in its values list must not be written with the RowBinary writer");
+                ps.setInt(1, 1);
+                Assert.assertEquals(ps.executeUpdate(), 1);
+            }
+
+            try (PreparedStatement ps = connection.prepareStatement(
+                    "INSERT INTO " + table + " (v1, v2) VALUES (?, ?)")) {
+                Assert.assertTrue(ps instanceof WriterStatementImpl);
+                ps.setInt(1, 2);
+                ps.setString(2, "CD");
+                Assert.assertEquals(ps.executeUpdate(), 1);
+            }
+
+            try (Statement stmt = connection.createStatement();
+                 ResultSet rs = stmt.executeQuery("SELECT v1, v2 FROM " + table + " ORDER BY v1")) {
+                Assert.assertTrue(rs.next());
+                Assert.assertEquals(rs.getInt(1), 1);
+                Assert.assertEquals(rs.getString(2), "AB");
+                Assert.assertTrue(rs.next());
+                Assert.assertEquals(rs.getInt(1), 2);
+                Assert.assertEquals(rs.getString(2), "CD");
+                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

@@ -968,4 +968,40 @@ public void testAllowedTableKeywords() throws Exception {
             Assert.fail(failureMessage);
         }
     }
+
+    @Test(dataProvider = "testInsertUseFunctionDP")
+    public void testInsertUseFunction(String sql, boolean useFunction, boolean parseableByGrammar) {
+        ParsedPreparedStatement stmt = parser.parsePreparedStatement(sql);
+        Assert.assertTrue(stmt.isInsert(), "Statement is not recognized as an insert: " + sql);
+        assertEquals(stmt.isUseFunction(), useFunction, "Function usage expectation does not match: " + sql);
+        if (!javaCcBackend) {
+            assertEquals(stmt.isHasErrors(), !parseableByGrammar, "Parse error expectation does not match: " + sql);
+        }
+    }
+
+    @DataProvider
+    public static Object[][] testInsertUseFunctionDP() {
+        return new Object[][]{
+                /* values list of placeholders only */
+                {"INSERT INTO t (v1, v2) VALUES (?, ?)", false, true},
+                {"INSERT INTO t VALUES (?, ?)", 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},
+        };
+    }
+
+    @Test
+    public void testUseFunctionOfUnparseableSelect() {
+        if (javaCcBackend) {
+            return; // the JavaCC backend reports any function use, not only one in an insert values list
+        }
+        ParsedPreparedStatement stmt = parser.parsePreparedStatement("SELECT hex(x'AB') WHERE v = ?");
+        Assert.assertFalse(stmt.isInsert(), "Statement is recognized as an insert");
+        Assert.assertFalse(stmt.isUseFunction(), "Function usage is reported for a statement that is not an insert");
+    }
 }