Fix jdbc-v2: accept WITH RECURSIVE in both SQL grammars - #3128
Open
polyglotAI-bot wants to merge 2 commits into
Open
polyglotAI-bot wants to merge 2 commits into
polyglotAI-bot wants to merge 2 commits into
Conversation
Neither the JavaCC nor the ANTLR4 grammar knew the RECURSIVE keyword, so after WITH the word was taken for the name of the first common table expression and the statement was rejected. The query still ran, because the driver falls back to sending the original SQL, but every createStatement / prepareStatement call for a recursive CTE reported a parse failure (a WARN with the JavaCC backend, DEBUG with the ANTLR4 ones) and classified the statement as unknown. RECURSIVE is now an optional keyword after WITH in the CTE clause of both grammars, and is added to the keyword lists that keep a keyword usable in identifier positions, so `recursive` stays valid as a column, alias, table or CTE name. Fixes: #3122
1 task
|
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
6 tasks
TriageCategory: Summary What this impacts
Concerns
Required reviewer action
|
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
1 participant
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 #3122.
Neither jdbc-v2 SQL grammar knew the
RECURSIVEkeyword (grep -ci recursivereturned 0 for both). AfterWITH,RECURSIVEwas therefore lexed as an ordinary identifier and taken for the name of the first common table expression, soWITH RECURSIVE t AS (...) SELECT ...— valid SQL the server executes — failed client-side parsing. The query itself still ran, because the driver falls back to sending the original SQL, but everycreateStatement/prepareStatementcall for a recursive CTE reported a parse failure (aWARNwith the JavaCC backend,DEBUGwith the ANTLR4 ones) and classified the statement as unknown.RECURSIVEis now an optional keyword afterWITHin the CTE clause of both grammars. Both backends parse recursive CTEs cleanly, which also removes the severity inconsistency between them for this statement: there is no parse failure left to log.Changes
jdbc-v2/src/main/javacc/ClickHouseSqlParser.jj: new<RECURSIVE>token;withClause()accepts an optional<RECURSIVE>after<WITH>;RECURSIVEadded toanyKeyword()so it stays usable in identifier positions.jdbc-v2/src/main/antlr4/.../ClickHouseLexer.g4: newRECURSIVEtoken.jdbc-v2/src/main/antlr4/.../ClickHouseParser.g4:cteClause : WITH RECURSIVE? ...;RECURSIVEadded tokeywordandkeywordForAliasfor the same reason.CHANGELOG.md: bug-fix entry.withClause()/cteClauseare the single productions both the plainSELECTprefix and theSELECTof anINSERT ... SELECTgo through, so one change covers both entry points.Test
BaseSqlParserFacadeTest.testRecursiveCteStatements(a TestNG@DataProvider, so it runs for the JavaCC backend and both ANTLR4 backends) asserts a statement parses without errors and is classified correctly:RECURSIVEapplies to the whole list, as on the server);SELECTof anINSERT, and one inside a subquery;recursivestill works as a column, alias, table and CTE name.Verified failing before the change (12 cases: 4 statement forms x 3 backends) and passing after; the contrast cases pass both before and after, so the change is limited to
WITH RECURSIVE. Expected results were taken from a live ClickHouse 26.8.2.7 server, which accepts every form asserted here (WITH recursive AS (...)— a CTE namedrecursive— is a server syntax error, so the client is only pinned to keep its existing lenient handling of it). Fulljdbc-v2unit suite: 1930 tests, 0 failures.Notes
INSERT ... WITH RECURSIVE ... SELECTcase: the backends disagree there (the ANTLR4 backends report the CTE name instead of the insert target) and that difference is pre-existing and independent ofRECURSIVE. It will be reported separately.clickhouse-jdbc/src/main/javacc/ClickHouseSqlParser.jj). It is left out to keep this change to one module, as with the earlier v2/v1 parser fixes (Fix jdbc-v2: NPE when the JavaCC parser cannot parse an INSERT VALUES list #3014 / Fix clickhouse-jdbc: NPE when the JavaCC parser cannot parse an INSERT VALUES list #3034), and will follow separately.recursiveis now reserved in the few grammar positions that accept only a bare identifier and no keyword (e.g. the ANTLR4userIdentifierrule, the JavaCCFORMAT/INFILE/OUTFILE/COMPRESSIONoperands). This matches how every other keyword in these grammars behaves.Pre-PR validation gate
mainfor all three parser backends)AGENTS.md(targeted module,@DataProviderinstead of near-identical methods, no issue numbers in test code, CHANGELOG updated)docs/features.mdunchanged: no jdbc-v2 feature is added or removed, and the documented SQL parsing behavior is unchanged — a statement that should always have parsed now does