doris/starrocks: close the parser gaps hidden by the trailing-token swallow (BYT-10084) - #401
Merged
Merged
Conversation
…wallow (BYT-10084)
parseSingle never verifies that a statement parse consumed its whole
segment, so any valid prefix + unparseable tail is silently accepted with
a truncated AST. Before the strict check lands (BYT-10085), everything
the swallow was hiding has to parse for real — otherwise the flip turns
silently-working customer queries into hard editor errors.
Closed, engine-verified against Doris 3.1.4 (17 new conformance cases,
all green):
- LIMIT offset, count
- string aliases: expr AS "20%" / AS 'x' (SELECT items only; table
aliases keep rejecting strings per the grammar's strictIdentifier)
- GROUP BY GROUPING SETS ((a,b),(a),()) — previously GROUPING parsed as
a column and SETS(...) was swallowed; the old conformance pass was a
false positive
- element access and slices: expr[i], expr[b:e], expr[b:] — new
ElementAtExpr / ArraySliceExpr nodes, postfix at primary tightness
- GROUP BY ... WITH ROLLUP (expression-list form only)
- table-valued functions in FROM: BACKENDS(), numbers("k"="v")
(doris; starrocks already had TableFunctionRef)
- TABLET(...) before the alias, TABLESAMPLE(n ROWS|PERCENT)
[REPEATABLE seed] after it
- SHOW DATABASES FROM/IN <catalog>
- BUILD INDEX ... PARTITION p | PARTITION (list) | PARTITIONS (list)
- starrocks: post-column-list PRIMARY KEY(...) key description — the
DefaultExpr test was silently dropping PRIMARY KEY + DISTRIBUTED BY
- splitter: BEGIN WITH LABEL x is TCL, not a compound-block opener;
two-token lookahead, because a block can also open with a CTE
Analysis regression test: GetQuerySpan on
SELECT secret_col[1] FROM sensitive_table now reports the table access
it used to lose — the fail-open masking blind spot behind this series.
Gate for BYT-10085: with the strict trailing-token check patched in
locally, both engines' full suites and legacy corpora run green.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The StarRocks 3.4.10 container run flagged four of the mirrored forms as engine-rejected (omni=true, starrocks=false): - TABLESAMPLE(...) [REPEATABLE seed] — not StarRocks syntax; support removed, TABLET(...) alone is engine-valid and stays - GROUP BY ... WITH ROLLUP — engine-rejected; branch removed (ROLLUP(a) and GROUPING SETS remain) - arr[begin:end] slices — engine-rejected; ElementAt stays, the slice arm and the ArraySliceExpr node go - BUILD INDEX ... — the engine rejects the statement in every form; the PARTITION extension is reverted (the pre-existing base BUILD INDEX over-acceptance from the doris fork is left for a follow-up) The legacy-corpus lines demanding the removed forms were doris leakage the StarRocks engine itself rejects; pruned to their engine-valid parts. The negative conformance arms for TABLESAMPLE / WITH ROLLUP land with BYT-10085 — until the strict trailing-token check, the parser still swallows them as trailing junk, so parity cannot be asserted yet. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1150519196
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…dedup Two findings from the automated review, both verified against live engines before fixing: - Empty string aliases: SELECT c AS '' is engine-valid on both Doris 3.1.4 and StarRocks 3.4.10 (container-probed), but the alias collapsed into the "" no-alias sentinel and GetQuerySpan named the column after the expression instead. parseOptionalAlias now reports presence separately from the value, SelectItem carries Aliased, and the span's result naming keys off presence. Conformance gains string_alias_empty on both engines. - Table-function name walked twice: TableRef.Name mirrors Func.Name (the same *ObjectName), and the walker visited it via both fields. The bare Name is now walked only when there is no Func to cover it; a test pins one visit per name. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9af9af577e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ssion parser Codex P1 on #401, verified and broader than reported: parseSelectItem's qualified-name fast path returned a bare ColumnRef for any ident.ident select item, bypassing the Pratt parser entirely — so EVERY continuation after a leading qualified column fell to the trailing-token swallow, not just subscripts: SELECT t.secret_col[1] FROM sensitive_table t -- truncated, table lost SELECT t.a + 1 FROM t -- truncated to SELECT t.a The first is the qualified twin of the masking fail-open this series closes; the second would have turned into a hard error for everyday SQL once the strict check (BYT-10085) landed. Neither the corpus nor the conformance matrix had a select item starting with a qualified column followed by anything, which is how it survived both fences. The fast path now claims only what it exists for — the qualified star (t.*, db.t.*) — and rolls back via the parser checkpoint for everything else, letting the general expression path parse qualified columns, function calls, subscripts and operators uniformly. Conformance gains element_at_qualified and qualified_column_arith on both engines; span tests pin that t.secret_col[1] keeps its table access. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d44c2fe9c0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex P2 on #401: GetQuerySpan("SELECT * FROM BACKENDS()") reported BACKENDS in AccessTables — visitTableRef recorded Name without checking Func, so authorization and lineage consumers were handed a nonexistent table for BACKENDS(), numbers(...), and friends. Mirror the StarRocks TableFunctionRef handling: walk the call's arguments for lineage, record no table. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
rebelice
added a commit
that referenced
this pull request
Aug 21, 2026
…ngine-grounded - FROM-subquery detection is now an explicit AST discriminator (TableRef.Subquery) instead of a prefix heuristic: tables named selected or within, and a quoted `select`, were classified as query text, and with fail-closed spans that regressed valid queries into errors. The heuristic is deleted; Name.Parts[0] still mirrors the raw text for legacy consumers. - EXPLAIN no longer masks nested parse failures on the strict path: EXPLAIN SELECT * FROM parsed clean by swallowing the garbage into the RawQuery fallback. Strict mode propagates the nested error; best-effort keeps the recovery. - The fail-closed contract now covers statements that produce no lineage: DELETE ... WHERE id IN (SELECT <malformed>) returned an empty span with nil error; embedded subqueries in any statement kind are analyzed, so a malformed one fails the span and a well-formed one contributes its table reads. - Table-function argument subqueries validate the same way (pinned by test; the visitTableRef Func walk from #401 already covered it). - Subquery parse errors are rebased to outer-statement coordinates via SubqueryExpr.TextStart, so diagnostics highlight the offending token, not byte 9 of the extracted text. - The strict trailing-token branch drains the rest of the segment so the lazy lexer reaches later lexical errors (SELECT 1 ))) 'unterminated now reports both diagnostics). - SET TRANSACTION parses its characteristics (ISOLATION LEVEL ... / READ ONLY / READ WRITE) instead of raw-capturing to EOF, which hid trailing junk from the strict check. Conformance gains explain_incomplete_rejected and from_table_named_selected on both engines; both container suites green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
Problem
parseSinglenever verifies that a statement parse consumed its whole segment, so any valid statement prefix + unparseable tail is silently accepted with a truncated AST. The concrete fail-open case:parses with zero errors, the FROM clause vanishes from the AST, and
GetQuerySpanreportsAccessTables=[]— while the engine executes the full query. Masking and lineage see less than what runs.The hole is congenital: inherited from the v1 snowflake parser-entry framework (#19), copied into doris (#52/#61), forked into starrocks (#287) two days before snowflake fixed itself (#303). mongo got the same discipline in #395. The strict flip is BYT-10085 (stacked PR); this PR closes every piece of engine-valid syntax the swallow was hiding first — flipping first would regress silently-working customer queries into hard editor errors.
Method
Prototype measurement: patch the 5-line strict check in locally, run the full
-shortsuites + legacy corpora → 17 failures, fully inventoried, each closed here. Two were defects inside already-merged tests: dorisGROUP BY GROUPING SETS ((a),())was never actually parsed (SETS ((a),())swallowed — the #400 conformance case passed as a false positive), and the starrocksDEFAULT (uuid())test silently droppedPRIMARY KEY(id) DISTRIBUTED BY ....Everything is container-verified: Doris 3.1.4 and StarRocks 3.4.10.
What this closes
LIMIT offset, count(LIMIT 5, 10)expr AS "20%"/AS 'x'— SELECT items only; table aliases keep rejecting strings per the grammar's strictIdentifierGROUP BY GROUPING SETS ((a,b),(a),())for real;GROUP BY a WITH ROLLUP(doris)expr[i],expr[b:e],expr[b:]— newElementAtExpr/ArraySliceExprnodes, postfix at primary tightnessBACKENDS(),numbers("k"="v")(doris; starrocks already hadTableFunctionRef);TABLET(...)before the alias;TABLESAMPLE(n ROWS|PERCENT) [REPEATABLE s]after it (doris)SHOW DATABASES FROM/IN <catalog>;BUILD INDEX ... PARTITION p | PARTITION (list) | PARTITIONS (list)(doris)PRIMARY KEY(...)key descriptionBEGIN WITH LABEL x; COMMITis TCL, not a compound-block opener — two-token lookahead, since a block can also open with a CTEThe StarRocks container corrected four mirrored assumptions
The live 3.4.10 run flagged four forms as engine-rejected that the doris mirror had assumed valid:
TABLESAMPLE,GROUP BY ... WITH ROLLUP,arr[b:e]slices, and every form ofBUILD INDEX. Their starrocks support is removed (TABLET alone stays — engine-valid), and the starrocks legacy-corpus lines demanding them were doris leakage the StarRocks engine itself rejects — pruned to their engine-valid parts. The pre-existing baseBUILD INDEXover-acceptance from the fork is left for a follow-up. Negative conformance arms for TABLESAMPLE / WITH ROLLUP land with BYT-10085, where the parser stops swallowing them as trailing junk.Testing
[1,2][1,2],[1,2][:2],GROUPING SETS ((a)), bnegatives)GetQuerySpan("SELECT secret_col[1] FROM sensitive_table")→AccessTables=[sensitive_table]go test -short ./doris/... ./starrocks/...green; gofmt baseline unchanged🤖 Generated with Claude Code