Fix jdbc-v2: keep INSERT INTO [TABLE] FUNCTION off the RowBinary insert path - #3016
Open
polyglotAI-bot wants to merge 4 commits into
Open
Fix jdbc-v2: keep INSERT INTO [TABLE] FUNCTION off the RowBinary insert path#3016polyglotAI-bot wants to merge 4 commits into
polyglotAI-bot wants to merge 4 commits into
Conversation
…rt path Both SQL parser backends failed to report a table-function insert target as a function, so with beta.row_binary_for_simple_insert=true the statement was routed to the RowBinary writer, which looked the function name (or the JavaCC "unknown" placeholder) up as a table and failed with UNKNOWN_TABLE. The ANTLR4 listener now flags an insert whose target is a tableFunctionExpr, the JavaCC grammar recognizes the optional TABLE keyword before FUNCTION (previously TABLE was consumed and FUNCTION swallowed as the table name) and flags both insert-target function branches, and the RowBinary gate additionally requires a resolved table name. Fixes: #3015
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
…rt-into-table-function
TriageCategory: Summary What this impacts
Concerns
Required reviewer action
|
…rt-into-table-function
…rt-into-table-function # Conflicts: # jdbc-v2/src/test/java/com/clickhouse/jdbc/WriterStatementImplTest.java
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #3015.
With
beta.row_binary_for_simple_insert=true,ConnectionImpl.prepareStatementroutes asingle-values-group INSERT to the RowBinary
WriterStatementImpland looks the parsedtarget up with
client.getTableSchema(...). ForINSERT INTO [TABLE] FUNCTION f(...) VALUES (?)the target is a table function, not a table, but neither parser backend reported it as a
function: the ANTLR4 listener only set
useFunctionfor functions inside the values list, andthe JavaCC grammar set
funcUsedonly for values-list functions too. So the statement took theRowBinary path and the function name (ANTLR4) or the
unknownplaceholder (JavaCC) was lookedup as a table:
A statement the server accepts therefore failed. There is no plain table to write RowBinary
into here, so it must stay on the regular SQL path (as it already does with the beta flag off).
Changes
SqlParserFacade(ANTLR4 prepared-statement listener): an INSERT whose target is atableFunctionExpris reported withuseFunction = true.ClickHouseSqlParser.jj(insertStmt): added an alternative forTABLE FUNCTION f(, so theTABLEkeyword is no longer consumed bytableIdentifier()— previouslyFUNCTIONwasswallowed as the table name, the subsequent parse error was silently absorbed by
dataClause(), and the values-group count was lost (valuesGroups = 0). Bothinsert-target-function branches now set
token_source.funcUsed = true.ClickHouseSqlParser.jj(reset()): clearfuncUsedbetween statements — it was the onlybuild-time field left un-reset, which now matters because
insertStmtsets it.ConnectionImpl.prepareStatement: the RowBinary gate additionally requires a resolved tablename (non-null and not the
unknownplaceholder), so any other statement whose target theparser cannot resolve falls back to the SQL path instead of issuing a doomed schema lookup.
Inserts into a plain table are unaffected and still use the RowBinary writer.
Test
BaseSqlParserFacadeTest.testInsertTargetTableFunctionIsReportedAsFunction(runs for all threeparser backends: JAVACC, ANTLR4, ANTLR4_PARAMS_PARSER) pins
useFunctionand the values-groupcount for the table-function forms (
FUNCTION,TABLE FUNCTION, with a column list, withSELECT, lowercase, multi-line, and with a comment before the target) plus contrast rows thatmust keep their current behavior: plain table,
TABLE t,db.t, a table literally namedfunction(with and withoutTABLEand a column list), and a function inside the values list.BaseSqlParserFacadeTest.testInsertPlainTableTargetKeepsTableNamepins the extracted table namefor the plain-table forms, including the table named
function.WriterStatementImplTest.testInsertIntoTableFunctionUsesSqlPath(integration, beta flag on,each parser backend) asserts the prepared statement is not a
WriterStatementImpland thatINSERT INTO [TABLE] FUNCTION null('id UInt32') VALUES (?)actually executes against the server.Without the fix, 5 of these 6 cases fail with the reported
UNKNOWN_TABLE(the sixth — JavaCCTABLE FUNCTION— only escaped it accidentally, via the mis-parse that leftvaluesGroups = 0).WriterStatementImplTest.testInsertIntoPlainTableStillUsesWriteris the contrast case: with thebeta flag on, a plain-table INSERT (with and without the optional
TABLEkeyword, each backend)must still be a
WriterStatementImpland insert successfully.Verified in a devbox against ClickHouse 26.5.1:
mvn -pl jdbc-v2 test→ 1355 tests, 0 failures;mvn -pl jdbc-v2 -am -DskipUTs=true -Dit.test=WriterStatementImplTest ... verify→ 14 tests, 0failures (5 failures on the unpatched tree). No existing test was modified.
docs/changes_checklist.md— "Conditional logic or guard changed"TABLEkeyword, a table literally namedfunction(with/without
TABLE, with/without a column list), a function inside the values list,INSERT ... SELECT, and a null/placeholder table name are all covered by the data providers.fallback is the pre-existing SQL path, which already handled these statements correctly.
checklist's context check for parser code requires.
docs/features.mdneedsno update; the
beta.row_binary_for_simple_insertbehavior change is a bug fix and is recordedin
CHANGELOG.md.Pre-PR validation gate
main, passes with the fix)useFunctioncontract), not the symptom siteAGENTS.md/docs/changes_checklist.mdConnection.prepareStatement+executeUpdate)