diff --git a/CHANGELOG.md b/CHANGELOG.md index 52c92a57a..bd3b5b3e1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,14 @@ ### Bug Fixes +- **[jdbc-v2]** Fixed `INSERT INTO [TABLE] FUNCTION f(...) VALUES (?)` failing with + `Code: 60 ... does not exist. (UNKNOWN_TABLE)` when the `beta.row_binary_for_simple_insert` feature was + enabled. Neither SQL parser reported a table-function insert target as a function, so the statement was + routed to the `RowBinary` writer, which looked the function name (or a placeholder such as `unknown`) up as + a table. Both parsers now report such a statement as using a function, so it stays on the regular SQL path; + additionally the JavaCC grammar no longer mis-parses `INSERT INTO TABLE FUNCTION f(...)` by consuming + `FUNCTION` as the table name. Inserts into a plain table are unaffected and still use the `RowBinary` + writer. (https://github.com/ClickHouse/clickhouse-java/issues/3015) - **[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/ConnectionImpl.java b/jdbc-v2/src/main/java/com/clickhouse/jdbc/ConnectionImpl.java index 053acc874..f614ccde6 100644 --- a/jdbc-v2/src/main/java/com/clickhouse/jdbc/ConnectionImpl.java +++ b/jdbc-v2/src/main/java/com/clickhouse/jdbc/ConnectionImpl.java @@ -13,6 +13,7 @@ import com.clickhouse.jdbc.internal.JdbcConfiguration; import com.clickhouse.jdbc.internal.ParsedPreparedStatement; import com.clickhouse.jdbc.internal.SqlParserFacade; +import com.clickhouse.jdbc.internal.parser.javacc.ClickHouseSqlStatement; import com.clickhouse.jdbc.metadata.DatabaseMetaDataImpl; import com.google.common.collect.ImmutableMap; import org.slf4j.Logger; @@ -447,10 +448,14 @@ public PreparedStatement prepareStatement(String sql, int resultSetType, int res * - INSERT INTO t VALUES (now(), ?, ?) !# there is a function 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 + * - the target table could not be resolved from the statement */ + String table = parsedStatement.getTable(); if (!parsedStatement.isInsertWithSelect() && parsedStatement.getAssignValuesGroups() == 1 - && !parsedStatement.isUseFunction()) { - TableSchema tableSchema = client.getTableSchema(parsedStatement.getTable(), schema); + && !parsedStatement.isUseFunction() + && table != null && !ClickHouseSqlStatement.DEFAULT_TABLE.equals(table)) { + TableSchema tableSchema = client.getTableSchema(table, schema); return new WriterStatementImpl(this, sql, tableSchema, parsedStatement); } } 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..682681fd0 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 @@ -333,6 +333,12 @@ public void enterTableExprIdentifier(ClickHouseParser.TableExprIdentifierContext @Override public void enterInsertStmt(ClickHouseParser.InsertStmtContext ctx) { + if (ctx.tableFunctionExpr() != null) { + // INSERT INTO [TABLE] FUNCTION f(...) has no plain table to write into, so the + // parsed target must not be treated as a table name. + parsedStatement.setUseFunction(true); + } + ClickHouseParser.TableIdentifierContext tableId = ctx.tableIdentifier(); if (tableId != null) { extractAndSetDatabaseAndTable(tableId); diff --git a/jdbc-v2/src/main/javacc/ClickHouseSqlParser.jj b/jdbc-v2/src/main/javacc/ClickHouseSqlParser.jj index d0c088615..bc084568f 100644 --- a/jdbc-v2/src/main/javacc/ClickHouseSqlParser.jj +++ b/jdbc-v2/src/main/javacc/ClickHouseSqlParser.jj @@ -265,6 +265,7 @@ TOKEN_MGR_DECLS: { compressLevel = null; format = null; file = null; + funcUsed = false; parameters.clear(); positions.clear(); settings.clear(); @@ -588,9 +589,12 @@ void grantStmt(): {} { // not interested void insertStmt(): {} { ( - LOOKAHEAD({ getToken(1).kind == FUNCTION + LOOKAHEAD({ getToken(1).kind == TABLE && getToken(2).kind == FUNCTION + && !tokenIn(3, VALUES, FORMAT, SETTINGS, SELECT, WITH, INFILE) + && getToken(4).kind == LPAREN }) functionExpr() { token_source.funcUsed = true; } + | LOOKAHEAD({ getToken(1).kind == FUNCTION && !tokenIn(2, VALUES, FORMAT, SETTINGS, SELECT, WITH, INFILE) - && getToken(3).kind == LPAREN }) functionExpr() + && getToken(3).kind == LPAREN }) functionExpr() { token_source.funcUsed = true; } | ( LOOKAHEAD({ getToken(1).kind == TABLE && !tokenIn(2, VALUES, FORMAT, SETTINGS, SELECT, WITH, LPAREN) })
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..da807561d 100644 --- a/jdbc-v2/src/test/java/com/clickhouse/jdbc/WriterStatementImplTest.java +++ b/jdbc-v2/src/test/java/com/clickhouse/jdbc/WriterStatementImplTest.java @@ -138,6 +138,76 @@ public void close() throws IOException { } } + @DataProvider(name = "insertTargetFunctionForms") + Object[][] insertTargetFunctionForms() { + return new Object[][]{ + {"JAVACC", "INSERT INTO FUNCTION null('id UInt32') VALUES (?)"}, + {"JAVACC", "INSERT INTO TABLE FUNCTION null('id UInt32') VALUES (?)"}, + {"ANTLR4", "INSERT INTO FUNCTION null('id UInt32') VALUES (?)"}, + {"ANTLR4", "INSERT INTO TABLE FUNCTION null('id UInt32') VALUES (?)"}, + {"ANTLR4_PARAMS_PARSER", "INSERT INTO FUNCTION null('id UInt32') VALUES (?)"}, + {"ANTLR4_PARAMS_PARSER", "INSERT INTO TABLE FUNCTION null('id UInt32') VALUES (?)"}, + }; + } + + /** + * An INSERT whose target is a table function has no plain table to fetch a schema for, so it must stay + * on the regular SQL path instead of being routed to the RowBinary writer, which would look the + * function name up as a table and fail with UNKNOWN_TABLE. + */ + @Test(groups = {"integration"}, dataProvider = "insertTargetFunctionForms") + public void testInsertIntoTableFunctionUsesSqlPath(String parser, String sql) throws SQLException { + 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); + PreparedStatement ps = connection.prepareStatement(sql)) { + Assert.assertFalse(ps instanceof WriterStatementImpl, + "INSERT into a table function must not use the RowBinary writer: " + sql); + ps.setInt(1, 42); + Assert.assertEquals(ps.executeUpdate(), 1); + } + } + + @DataProvider(name = "insertTargetPlainTableForms") + Object[][] insertTargetPlainTableForms() { + return new Object[][]{ + {"JAVACC", "INSERT INTO %s VALUES (?)"}, + {"JAVACC", "INSERT INTO TABLE %s VALUES (?)"}, + {"ANTLR4", "INSERT INTO %s VALUES (?)"}, + {"ANTLR4", "INSERT INTO TABLE %s VALUES (?)"}, + {"ANTLR4_PARAMS_PARSER", "INSERT INTO %s VALUES (?)"}, + {"ANTLR4_PARAMS_PARSER", "INSERT INTO TABLE %s VALUES (?)"}, + }; + } + + @Test(groups = {"integration"}, dataProvider = "insertTargetPlainTableForms") + public void testInsertIntoPlainTableStillUsesWriter(String parser, String sqlTemplate) throws SQLException { + String table = "bt_writer_plain_table"; + 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("DROP TABLE IF EXISTS " + table); + stmt.execute("CREATE TABLE " + table + " (id Int32) Engine MergeTree ORDER BY ()"); + } + + try (PreparedStatement ps = connection.prepareStatement(String.format(sqlTemplate, table))) { + Assert.assertTrue(ps instanceof WriterStatementImpl, + "INSERT into a plain table must keep using the RowBinary writer: " + sqlTemplate); + ps.setInt(1, 42); + Assert.assertEquals(ps.executeUpdate(), 1); + } 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..60d2541e7 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 @@ -311,6 +311,58 @@ private void assertInsertColumns(String sql, String... expectedColumns) { Assert.assertEquals(actualColumns, expectedColumns, "Insert column names mismatch for: " + sql); } + @Test(dataProvider = "insertTargetFunctionDP") + public void testInsertTargetTableFunctionIsReportedAsFunction(String sql, boolean expectedUseFunction, + int expectedValuesGroups) { + ParsedPreparedStatement stmt = parser.parsePreparedStatement(sql); + Assert.assertFalse(stmt.isHasErrors(), "Query should parse without errors: " + sql); + Assert.assertTrue(stmt.isInsert(), "Should be an INSERT: " + sql); + Assert.assertEquals(stmt.isUseFunction(), expectedUseFunction, "useFunction mismatch for: " + sql); + Assert.assertEquals(stmt.getAssignValuesGroups(), expectedValuesGroups, + "Values group count mismatch for: " + sql); + } + + @DataProvider + public static Object[][] insertTargetFunctionDP() { + return new Object[][] { + // An INSERT whose target is a table function has no plain table behind it + {"INSERT INTO FUNCTION null('id UInt32') VALUES (?)", true, 1}, + {"INSERT INTO TABLE FUNCTION null('id UInt32') VALUES (?)", true, 1}, + {"INSERT INTO FUNCTION remoteSecure('h', 'db', 't', 'u', 'p') (id, name) VALUES (?, ?)", true, 1}, + {"INSERT INTO FUNCTION s3('url', 'key', 'secret', 'CSV') SELECT * FROM t", true, 0}, + {"insert into function null('id UInt32') values (?)", true, 1}, + {"INSERT INTO\n TABLE FUNCTION null('id UInt32')\n VALUES (?)", true, 1}, + {"INSERT INTO /* target */ FUNCTION null('id UInt32') VALUES (?)", true, 1}, + // Contrast: plain table targets, including a table literally named "function" + {"INSERT INTO t VALUES (?)", false, 1}, + {"INSERT INTO TABLE t VALUES (?)", false, 1}, + {"INSERT INTO db.t VALUES (?)", false, 1}, + {"INSERT INTO function VALUES (?)", false, 1}, + {"INSERT INTO TABLE function VALUES (?)", false, 1}, + {"INSERT INTO function (id) VALUES (?)", false, 1}, + // Contrast: a function inside the values list is already reported as a function + {"INSERT INTO t VALUES (now(), ?)", true, 1}, + }; + } + + @Test(dataProvider = "insertTargetTableNameDP") + public void testInsertPlainTableTargetKeepsTableName(String sql, String expectedTable) { + ParsedPreparedStatement stmt = parser.parsePreparedStatement(sql); + Assert.assertFalse(stmt.isHasErrors(), "Query should parse without errors: " + sql); + Assert.assertEquals(stmt.getTable(), expectedTable, "Table name mismatch for: " + sql); + } + + @DataProvider + public static Object[][] insertTargetTableNameDP() { + return new Object[][] { + {"INSERT INTO t VALUES (?)", "t"}, + {"INSERT INTO TABLE t VALUES (?)", "t"}, + {"INSERT INTO db.t VALUES (?)", "db.t"}, + {"INSERT INTO function VALUES (?)", "function"}, + {"INSERT INTO TABLE function VALUES (?)", "function"}, + }; + } + @Test(dataProvider = "testCreateStmtDP") public void testCreateStatement(String sql) { ParsedPreparedStatement stmt = parser.parsePreparedStatement(sql);