doris/starrocks: make Parse strict, reserve tolerance for ParseBestEffort (BYT-10085) - #402
doris/starrocks: make Parse strict, reserve tolerance for ParseBestEffort (BYT-10085)#402rebelice wants to merge 8 commits into
Conversation
![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: 786059c2cf
ℹ️ 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".
786059c to
18848de
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 18848dee78
ℹ️ 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".
1841353 to
8f229d4
Compare
…fort (BYT-10085) Port the snowflake #303 / mongo #395 discipline: parseSingle rejects unconsumed trailing tokens on the success path (syntax error at the first leftover token, node dropped), threaded as parseAll(input, strictTrailing). Parse — and Diagnose, which rides on it — is now actually strict; ParseBestEffort keeps tolerating partial input for completion-style consumers. The hole dates to the v1 parser-entry framework (#19 → #52/#61) and made any valid statement prefix + junk parse silently with a truncated AST: SELECT secret_col[1] FROM sensitive_table analyzed as a table-less SELECT, so masking and lineage saw less than the engine executed. analysis fails closed to match: GetQuerySpan returns the parse error instead of analyzing a partial AST — including subquery raw-text re-parses (analyzeSubqueryText used to swallow those errors, which would silently drop a subquery's table reads from AccessTables). Verification: - corpus canary per engine: every cleanly-parsing legacy-corpus statement plus appended junk must error (330 doris / 328 starrocks statements). Raw-capture parsers (SHOW args, CREATE FUNCTION, CTAS) absorb the junk by design and are exempted explicitly — tracked as BYT-10089 - trailing-junk table tests, multi-statement isolation (bad segment errors, neighbors survive), ParseBestEffort tolerance regression - container conformance, both engines green, with new negative arms: doris rejects SELECT j->'$.a' and SELECT 1 */ 2 like the engine; starrocks additionally pins TABLESAMPLE and GROUP BY ... WITH ROLLUP as rejected (engine-verified non-syntax there) - the StarRocks run also disproved an assumption inherited from #400: StarRocks DOES have a -> JSON path operator (Doris does not). omni's rejection is the fail-safe direction and is tracked as BYT-10088 — parsing it as a lambda would shadow the column out of lineage Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
8f229d4 to
26ca104
Compare
…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>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b73f38c206
ℹ️ 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".
… subqueries, bare EXPLAIN - Subquery error offsets accumulate across nesting levels: each level's TextStart is relative to its own extracted text, and the walker now carries the ancestor base (spanWalker.textBase) while descending, so SELECT (SELECT (SELECT 1 */ 2 FROM t3) FROM t2) FROM t1 reports the error at the */ in the original statement (byte 25, not 17). - A placeholder body that parses cleanly as a non-query statement fails the span: SELECT EXISTS (DELETE FROM secret) FROM public returned a clean span with secret invisible to access checks. The engine rejects the form; analysis now fails closed on it. (The parser-level placeholder stays content-blind by design — the guarantee lives in analysis, which is why this is an analysis test and not a conformance case.) - Bare EXPLAIN (and EXPLAIN followed only by trivia) is a strict-mode error: the branch that propagates nested failures was only reachable after at least one query token, so EXPLAIN alone parsed clean with a nil Query. Best-effort recovery keeps accepting it. Conformance gains explain_bare_rejected on both engines; both container suites green. 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: 233e6e5b5b
ℹ️ 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".
…rs, strict SET expressions - Empty and comment-only subquery placeholder bodies fail the span: SELECT EXISTS () FROM public and EXISTS (/*comment*/) parsed to zero statements and bypassed the query-node validation entirely. The non-query error paths now share noteNonQuerySubquery, with locations in outer-statement coordinates. - parseSetItem's raw fallback consumed to the comma or EOF on an expression error, so SET x = ( and SET x = 1 + parsed clean under strict Parse. Strict mode propagates the expression error; best-effort keeps the raw recovery. Conformance gains set_expr_incomplete_rejected on both engines; both container suites green. 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: 3f49159dcf
ℹ️ 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".
…uery children, SHOW clauses, filtered segments, ParenSelect
- Strict SET requires the assignment separator and a value: SET x,
SET x =, SET x, y and SET x 1 reached EOF or the comma before the
strict branch could run. The scoped transaction spelling
(SET SESSION TRANSACTION ...) is routed to the transaction form so the
tightened generic path cannot reject it.
- Parsed query children inside non-query statements feed the analyzer:
INSERT INTO dest SELECT * FROM secret returned an empty AccessTables
because validateEmbeddedSubqueries only reacted to raw placeholders.
SelectStmt/SetOpStmt children now route through visitSelect/visitSetOp.
- Structured SHOW clause errors propagate in strict mode:
SHOW TABLES WHERE ( consumed to EOF inside parseShowLikeWhere and
discarded the expression error, so the leftover-token check saw
nothing. parseShowLikeWhere/parseShowFromLikeWhere now return errors.
- Lex errors from segments Split filtered out are promoted:
Parse("/* unterminated") produced zero segments and zero errors.
Strict mode lexes the full input once and adds any diagnostic the
per-segment parses did not already report (deduplicated by position).
- starrocks: a top-level parenthesized query — (SELECT * FROM secret) —
parses as ParenSelect and fell into the non-query default, returning
an empty span. It now dispatches as the outermost query, populating
both AccessTables and Results. (Doris rejects the top-level
parenthesized form at parse time today — a pre-existing
under-acceptance, fail-safe direction.)
Conformance gains set_valueless_rejected and
show_where_incomplete_rejected on both engines, plus paren_select on
starrocks; both container suites green.
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: 4feb7c5187
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
This PR tightens Doris and StarRocks parsing/analysis contracts so that the strict Parse path rejects statements that don’t fully consume their segment (preventing silently-truncated ASTs), while ParseBestEffort retains lenient “prefix parse” behavior for completion-style callers. It also updates query-span analysis to fail closed on parse failures (including failures inside raw-text subquery re-parses) and adds regression/conformance coverage to pin the new behavior.
Changes:
- Make
Parsestrict about trailing tokens via a sharedparseAll(..., strictTrailing)implementation, while keepingParseBestEfforttolerant. - Remove heuristic FROM-subquery detection by introducing explicit
TableRef.Subqueryand subqueryTextStartoffsets for accurate error localization. - Update
GetQuerySpanto return errors on any parse failure (top-level or nested subquery) and add tests/corpus canaries to prevent regressions.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| starrocks/parser/strict_trailing_test.go | Adds strict-trailing regression tests, corpus canary, and best-effort tolerance pins. |
| starrocks/parser/starrocks_syntax_conformance_test.go | Extends engine conformance matrix with new strict-negative cases. |
| starrocks/parser/show.go | Propagates clause parsing errors in strict mode; tightens EXPLAIN/SET/SHOW behaviors that previously masked failures. |
| starrocks/parser/select.go | Marks FROM-subqueries explicitly on TableRef.Subquery while preserving legacy raw text mirroring. |
| starrocks/parser/parser.go | Introduces strict-trailing parsing mode, shared parse pipeline, and strict lex-error promotion sweep. |
| starrocks/parser/expr.go | Adds SubqueryExpr.TextStart for correct outer-coordinate error shifting when re-parsing raw subquery text. |
| starrocks/ast/walk_children.go | Walks TableRef.Subquery explicitly to avoid treating raw-text mirrors as identifiers. |
| starrocks/ast/selectnodes.go | Adds TableRef.Subquery field to represent FROM-subqueries explicitly. |
| starrocks/ast/exprnodes.go | Adds SubqueryExpr.TextStart to support accurate error location remapping. |
| starrocks/analysis/query_span.go | Makes query-span analysis fail closed on parse/subquery errors; removes heuristic subquery detection. |
| starrocks/analysis/query_span_test.go | Adds tests for fail-closed parsing, subquery error remapping, DML subquery validation, and paren-select handling. |
| doris/parser/strict_trailing_test.go | Adds strict-trailing regression tests, corpus canary, and best-effort tolerance pins. |
| doris/parser/show.go | Propagates clause parsing errors in strict mode; tightens EXPLAIN/SET/SHOW behaviors that previously masked failures. |
| doris/parser/select.go | Marks FROM-subqueries explicitly on TableRef.Subquery while preserving legacy raw text mirroring. |
| doris/parser/parser.go | Introduces strict-trailing parsing mode, shared parse pipeline, and strict lex-error promotion sweep. |
| doris/parser/expr.go | Adds SubqueryExpr.TextStart for correct outer-coordinate error shifting when re-parsing raw subquery text. |
| doris/parser/doris_container_test.go | Adds strict-negative conformance cases that are now surfaced by strict trailing-token handling. |
| doris/ast/walk_children.go | Walks TableRef.Subquery explicitly and avoids mis-walking raw-text mirrors. |
| doris/ast/selectnodes.go | Adds TableRef.Subquery field to represent FROM-subqueries explicitly. |
| doris/ast/exprnodes.go | Adds SubqueryExpr.TextStart to support accurate error location remapping. |
| doris/analysis/query_span.go | Makes query-span analysis fail closed on parse/subquery errors; removes heuristic subquery detection. |
| doris/analysis/query_span_test.go | Adds tests for fail-closed parsing, subquery error remapping, and DML subquery validation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…e, LIKE pattern - The scoped transaction routing added in round four consumed the GLOBAL/SESSION/LOCAL qualifier and dropped it, making SET SESSION TRANSACTION ... indistinguishable from the unqualified form (flagged independently by both Codex and Copilot). parseSetTransaction now takes the scope and stores it on the SetItem, with LOCAL normalized to SESSION like the generic assignment path. - SHOW ... LIKE with no pattern: the engines DIVERGE, container-verified. StarRocks rejects the bare LIKE, so its strict mode requires the pattern string; Doris ACCEPTS it, so the Doris parser keeps tolerating the form and the conformance matrix pins the acceptance (show_like_no_pattern) — the review's premise held for one engine only. Conformance gains show_like_missing_pattern_rejected on starrocks and show_like_no_pattern on doris; both container suites green. 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: 5bc11f81aa
ℹ️ 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".
… SHOW FROM, SET NAMES/CHARSET - Physical tables read by DML reach AccessTables: UPDATE ... FROM secret, DELETE ... USING secret and MERGE ... USING secret all returned empty spans because the embedded-subquery pass ignored plain TableRefs. They now route through visitTableRef; this deliberately over-approximates by including any TableRef-shaped write target — for access checks the safe error is an extra entry, never a missing one. - SHOW TABLES FROM / IN with no database name ended at EOF, invisible to the leftover-token check; strict mode requires the operand (engine-verified reject on both engines). - SET NAMES / SET CHARSET without their charset, and a dangling COLLATE, finished the same way; strict mode requires the operands (engine-verified reject on both engines; the well-formed spellings are pinned as accepts). Conformance gains show_from_missing_db_rejected, set_names_missing_charset_rejected, set_names_dangling_collate_rejected, set_names and set_names_collate on both engines; both container suites green. 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: 9522ec0ec1
ℹ️ 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".
…best-effort SET, access locations - starrocks: WITH-prefixed DML installs its CTE scope before the generic walk. WITH c AS (SELECT id FROM secret) DELETE ... reported both secret and the logical name c as physical AccessTables (and dropped c from CTEs) — an access check against nonexistent c could falsely deny a valid statement, the wrong direction. Doris rejects WITH-prefixed DML at parse time, so only the fork needs this. - SET TRANSACTION keeps ParseBestEffort's recovery contract: the round- three structured parser errored unconditionally, predating the strictTrailing gating convention, so completion-style partial input (SET TRANSACTION ISOLATION LEVEL READ) stopped yielding a statement. The characteristics parser now reports what parsed plus the error; strict propagates, best-effort keeps the parsed prefix and captures the remainder raw. - TableAccess locations from reparsed subqueries are rebased into outer-statement coordinates like the parse errors already were: UPDATE dest SET x=(SELECT x FROM secret) pointed at byte 14 instead of 33. The round's other two findings (bare SHOW COLUMNS, dangling AS) are the optional-operand long tail: declined on the PR with rationale and folded into the raw-capture cleanup ticket's scope instead. 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: de5149146a
ℹ️ 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".
| if rest := p.collectRemainingRaw(); rest != "" { | ||
| parts = append(parts, rest) | ||
| } |
There was a problem hiding this comment.
Fresh evidence after the prior best-effort recovery fix is that parseTransactionCharacteristics consumes the incomplete characteristic before returning its error, so collectRemainingRaw() starts at EOF: ParseBestEffort("SET TRANSACTION ISOLATION LEVEL READ") now returns a statement whose SetItem.Raw is empty, and SET TRANSACTION READ ONLY, ISOLATION LEVEL READ retains only READ ONLY. This loses the partial text that the former raw fallback preserved, preventing completion-style consumers from determining what was being entered; capture the characteristic from its starting offset or otherwise append the consumed prefix as well. The mirrored StarRocks path has the same regression.
Useful? React with 👍 / 👎.
| // EOF is asserted only on the success path — a parse that already | ||
| // errored left cur mid-statement, and asserting EOF there would | ||
| // emit a spurious second diagnostic. | ||
| if strictTrailing && err == nil && p.cur.Kind != tokEOF { |
There was a problem hiding this comment.
When the input is HELP or HELP /*comment*/, both dialects reach EOF after parseHelp returns a successful HelpStmt with an empty Mask, so this trailing-token condition does not run and strict Parse/Diagnose report zero errors. Since the HELP grammar requires a topic, have the HELP parser reject a missing operand in strict mode rather than relying solely on leftover tokens.
Useful? React with 👍 / 👎.
Stacked on #401 — merge that first, then retarget/rebase this onto main.
Problem
Ports the snowflake #303 / mongo #395 discipline to the last two engines without it (all 17 omni engines were probed; only doris and starrocks swallow trailing garbage).
parseSinglenow rejects unconsumed trailing tokens on the success path — a syntax error at the first leftover token, node dropped — threaded asparseAll(input, strictTrailing):Parse(andDiagnose, which rides on it) is actually strict.SELECT a[1] FROM tcan never again parse as a table-lessSELECT awith zero errors.ParseBestEffortkeeps tolerating partial input for completion-style consumers, with a regression test pinning the tolerance.The check sits on the success path only: an errored parse left
curmid-statement, and asserting EOF there would double-report (googlesql's parseSingle rationale).analysis fails closed
GetQuerySpanused to discard parse errors (file, _ := parser.Parse(...), documented "tolerant of parse errors") — the parser fix alone would leave it fail-open. It now returns the parse error instead of analyzing a partial AST, including subquery raw-text re-parses:analyzeSubqueryTextused to swallow those errors, which would silently drop a subquery's table reads fromAccessTables. Empty input keeps the zero-span contract.Bytebase-side checklist before release: SQL editor UX for the new parse errors; masking path confirmed fail-closed on a
GetQuerySpanerror.Verification
\n )))is appended — 330 doris / 328 starrocks statements verified. Any future construct that regresses into a swallow trips its corpus entry here.collectRemainingRaw, CREATE FUNCTION, CTAS) absorb the junk into their own fields. They are exempted explicitly by node type and tracked as BYT-10089.))),*/ 2,j->'$.a', unbalanced parens); multi-statement isolation (SELECT 1; SELECT 2 ))); SELECT 3— error localized to segment 2, segments 1 and 3 survive); ParseBestEffort tolerance regressionSELECT j->'$.a' FROM tandSELECT 1 */ 2exactly like its engine; starrocks additionally pinsTABLESAMPLEandGROUP BY ... WITH ROLLUPas rejected (engine-verified non-syntax there)Found along the way
The StarRocks conformance run disproved an assumption inherited from #400: StarRocks HAS a
->JSON path operator (SELECT j->'$.a'is engine-valid there; Doris rejects it). omni's rejection is the fail-safe direction — parsing it as a lambda would shadow the column out of lineage — and the proper implementation (lambda ambiguity, lineage-safe AST shape) is tracked as BYT-10088.🤖 Generated with Claude Code