diff --git a/CHANGELOG.md b/CHANGELOG.md index 52c92a57a..8b11dc96d 100644 --- a/CHANGELOG.md +++ b/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) 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..196b18a7f 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); + assumeFunctionInValuesListOfRecoveredParseTree(stmt); } // Combine database and table like JavaCC does String tableName = stmt.getTable(); @@ -189,11 +190,27 @@ public ParsedPreparedStatement parsePreparedStatement(String sql) { 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(); 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 fc56117ed..3f60b1b15 100644 --- a/jdbc-v2/src/test/java/com/clickhouse/jdbc/WriterStatementImplTest.java +++ b/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())) { 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..61b34a223 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 @@ -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"); + } } \ No newline at end of file