doris/starrocks: close the engine-parity leftovers from the #400 review battery - #403
Merged
Conversation
…ew battery
Every item container-adjudicated up front (Doris 3.1.4 / StarRocks
3.4.10); the two engines split almost every call, so each side is fixed
to its own engine and the conformance matrices pin the divergences.
Doris (15 mismatches closed):
- BINARY binds at primary level to an identifier or string literal —
BINARY 1 / (1) / now() / concat(...) were accepted and the operand
swallowed comparisons; BINARY a = 'x' now parses as (BINARY a) = 'x'.
StarRocks keeps its general prefix form (engine-verified accept).
- Collection literal elements are constants: [1+1], [a], {'a': 1+1} and
even [-1] are engine syntax errors, while nested literals ([[1],[2]],
{'a': [1,2]}) parse. StarRocks arrays keep full expressions
(engine-verified). New parseCollectionConstant feeds arrays, maps and
the new struct form.
- Struct literals {c1, c2, ...} — brace constructors without key:value
pairs — were rejected; new StructLiteral node, empty braces stay a map.
- String-form user variables @'name' / @"name" (both engines).
- Top-level parenthesized queries: (SELECT 1), ((SELECT 1)) and
(SELECT 1) UNION (SELECT 2) now parse; parens are grouping only, so
analysis sees the ordinary SelectStmt/SetOpStmt, and set-op right-hand
sides accept parenthesized operands.
StarRocks (5 mismatches closed):
- @'name' string variables (shared fix).
- ((SELECT 1)) — the top-level gate only admitted SELECT/WITH after the
paren, and parseParenSelect could not nest.
- BUILD INDEX is rejected in every form like the engine (the fork
carried it from doris); the dead parser path is removed and the corpus
was already pruned.
Both parity batteries replay at zero mismatches; the cases live on as
permanent conformance entries with divergence comments. Both container
suites green.
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: 6184bedafc
ℹ️ 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".
Container-verified against Doris 3.1.4 and StarRocks 3.4.10: - doris: a set-op tail now follows any parenthesized operand kind, so ((SELECT 1) UNION SELECT 2) and (WITH c AS (SELECT 1) SELECT 1 UNION SELECT 2) parse (both engine-accepted). - doris: outer ORDER BY / LIMIT after a grouped query attach to the returned node instead of being rejected — (SELECT 1) ORDER BY 1 LIMIT 5. SetOpStmt gains OrderBy/Limit/Offset, wired into Walk. - starrocks: a parenthesized WITH must resolve to a query; (WITH c AS (SELECT 1) DELETE FROM t) is engine-rejected and now errors instead of wrapping DML in ParenSelect. - starrocks: trailing clauses reach through nested ParenSelect layers, so ((SELECT 1)) LIMIT 5 keeps its LIMIT instead of silently dropping it. Both syntax-conformance matrices extended with the new shapes (including the engine-accepted double-clause edge (SELECT 1 ORDER BY 1) ORDER BY 1) and replayed 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: 5bd95c8c2f
ℹ️ 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".
…the second #403 review round Container-verified against Doris 3.1.4 and StarRocks 3.4.10: - doris analysis: visitSetOp now walks SetOpStmt.OrderBy/Limit/Offset, so (SELECT 1) UNION (SELECT 2) ORDER BY (SELECT x FROM secret) reports the secret read in AccessTables instead of dropping it from lineage. - doris parser: every query position (statement-level SELECT and WITH, CTE bodies, INSERT and MTMV sources, paren operands) now routes through parseQueryTail = set-op tail + trailing ORDER BY / LIMIT groups. This closes SELECT 1 UNION (SELECT 2) LIMIT 5 and the statement-level WITH ... SELECT ... UNION over-reject the strict parse had exposed. The engine proved lenient here — SELECT 1 LIMIT 5 ORDER BY 1 and even repeated groups like SELECT 1 ORDER BY 1 ORDER BY 2 are accepts — so the attach loops and the last group wins. - doris parser: a parenthesized query's NodeLoc now covers the parens and any trailing clauses ((SELECT 1) is [0,10), not [1,9)). - starrocks: outer trailing clauses land on the ParenSelect wrapper instead of overwriting the inner query's own clauses — ((SELECT 1 LIMIT 1)) LIMIT 2 keeps both limits, and the subquery in an inner ORDER BY keeps contributing its table read. visitSetOpArm walks the wrapper clauses. - starrocks: the statement-level trailing attach no longer duplicates a clause the set operation already carries: the engine rejects SELECT 1 UNION SELECT 2 LIMIT 5 ORDER BY 1 (Doris accepts it — the divergence is pinned in both conformance matrices). Both conformance matrices extended by 26 engine-verified cases and replayed green; span tests cover the new clause walks on both sides. 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: c4f1c61b26
ℹ️ 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".
…the span (third #403 review round) Container-verified against Doris 3.1.4 and StarRocks 3.4.10: - doris: a trailing ORDER BY / LIMIT group that repeats a clause the query already carries no longer overwrites it. The parser wraps the query in a new ast.GroupedQuery node (walked, located, span-analyzed), so (SELECT 1 ORDER BY (SELECT x FROM secret) LIMIT 1) ORDER BY 2 — and the engine-lenient bare form SELECT 1 ORDER BY (...) ORDER BY 2 — keep the inner subquery, and GetQuerySpan reports the secret read again instead of understating access. No conflict means no wrapper: the common shapes keep their existing AST. - doris/starrocks: the WITH clause on the leftmost SELECT scopes over the entire set operation (engine-verified: WITH c AS (...) SELECT 1 UNION SELECT * FROM c resolves c in the right arm). visitSetOp now installs those CTE names before walking the arms, so the right arm's c is reported as a CTE reference, not a physical table c whose access check could falsely deny the statement. The StarRocks spine stops at a ParenSelect boundary, where a WITH stays scoped to its group. Both conformance matrices replayed green; span tests pin the preserved subquery read and the CTE-only classification on both engines. 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: 796f131954
ℹ️ 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".
…classification (fourth #403 review round) Container-verified against Doris 3.1.4: - doris: parentheses bound a CTE's scope — the engine rejects (WITH c AS (SELECT 1) SELECT 1) UNION SELECT * FROM c with "Table [c] does not exist" — so a paren group whose leading query carries a WITH keeps an explicit GroupedQuery boundary node, and leftmostWith no longer hoists across one. The right arm's c is a physical table read again instead of being omitted from AccessTables. - doris: CTE bodies that parse to a GroupedQuery (repeated clause groups, engine-accepted) are now dispatched by the CTE-body walk, so WITH c AS (SELECT 1 FROM secret ORDER BY 1 ORDER BY 2) ... keeps the secret read. CTE bodies also admit a parenthesized query — WITH c AS ((SELECT 1)) ... is engine-verified valid. - doris: a trailing group now wraps whenever the query already carries ANY clause, not only the same kind — (SELECT a FROM t LIMIT 1) ORDER BY a limits first and then orders, which is not SELECT ... ORDER BY a LIMIT 1, so the two groups stay on distinct nodes. - doris/starrocks: Classify treats query-grouping parentheses as transparent, so (SELECT ...) and ((SELECT ...)) carry QueryTypeSelect instead of QueryTypeUnknown. Doris conformance extended by four engine-verified cases and replayed green (the StarRocks parser is untouched this round); span tests pin the scope boundary and the grouped CTE body on the Doris side. 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: d04d34b8f6
ℹ️ 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".
rebelice
added a commit
that referenced
this pull request
Aug 28, 2026
…runners (#405) The redshift-container-tests job intermittently failed (3 times in one day on PR #403) with "first-set oracle unavailable in CI: container start: ... create container: context deadline exceeded" across all four LeadTokensMatchPG tests. Root cause: startFirstSetOracle gave container creation only 15 seconds, which a busy GitHub runner can exceed on the postgres:17-alpine image pull alone. The sibling harnesses (paren_oracle_test.go, redshift/catalog/container_helper_test.go) were already bumped to 120s; this helper was left behind. - Extract one bootstrap attempt (create, conn string, open, ping) into startFirstSetOracleAttempt with the repo-standard 120s per-attempt timeout; on failure it tears down every partially-created resource, including the container tcpg.Run can return alongside its error, so retries don't leak containers on the runner. - Retry the attempt 3 times inside the sync.Once body, 2s apart, with a t.Logf per failed attempt so CI logs show the retries. The docker preflight stays outside the loop (deterministic failure, keeps the local skip path fast). - Keep the fail-in-CI / skip-locally policy: after 3 failures the errors.Join'd attempt errors flow into the existing t.Fatalf, so the tests still fail loudly rather than skip when the oracle is genuinely unavailable. Verified with `go test ./redshift/parser -run 'LeadTokensMatchPG' -count=1` against local Docker: all four tests pass. Worst case in CI is 3x120s plus pauses, well inside the job's 30-minute limit. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…(fifth #403 review round) Container-verified against Doris 3.1.4: - analyzeSubqueryText now dispatches a GroupedQuery body through visitGroupedQuery, so SELECT EXISTS (SELECT 1 FROM secret ORDER BY 1 ORDER BY 2) — engine-accepted — analyzes the secret read instead of failing with "subquery must be a SELECT statement". Pinned in the conformance matrix. - The suggested WITH-scope extension over grouped trailing clauses is deliberately NOT applied: the engine resolves a repeated trailing group OUTSIDE the CTE scope (WITH zzg AS (SELECT 1) SELECT 1 LIMIT 1 ORDER BY (SELECT count(*) FROM zzg) fails with "Table [zzg] does not exist", while a select-list subquery resolves the CTE fine), so the span's physical-table report for that shape matches the engine. A regression test pins the verdict. 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
The #400 review ran a live-engine battery that found more mismatches than the pre-merge fixes absorbed. The remainder was never tracked anywhere: Doris-side BINARY over-acceptance (with a wrong AST shape), collection-literal elements taking full expressions, missing struct literals, string-form user variables, top-level parenthesized queries — plus the StarRocks fork's BUILD INDEX over-acceptance noted during #401. They have sat on main since.
The two failure directions matter differently: over-acceptances let the SQL editor pass statements the engine then rejects at execution; under-acceptances are the BYT-10070 class — engine-valid SQL dying in the editor with a syntax error.
Method
Probe first, fix second. A 22-probe battery ran against both live engines (Doris 3.1.4, StarRocks 3.4.10) before any code changed, because the engines split almost every call:
BINARY 1/BINARY now()[1+1],[a]array elements[-1]{1, 2}struct literalmapprefix)@'name'((SELECT 1))BUILD INDEX(any form)The
[-1]row earns its place: the fix originally allowed signed numeric constants, and the conformance run overturned that too — every call in this PR is engine-adjudicated, not grammar-assumed.What this closes
Doris (15 mismatches)
BINARY a = 'x'now parses as(BINARY a) = 'x', matching engine semantics, instead ofBINARY(a = 'x').parseCollectionConstant— literals and nested collection literals only, so[[1],[2]]and{'a': [1,2]}keep parsing while[1+1],[a]and[-1]are rejected like the engine does.StructLiteralnode for{c1, c2, ...}; empty braces stay aMapLiteral, a colon keeps the map form.@'name'/@"name".(SELECT 1),((SELECT 1))and(SELECT 1) UNION (SELECT 2): parens are grouping only, so the innerSelectStmt/SetOpStmtcomes back directly and analysis needs no new node; set-op right-hand sides accept parenthesized operands.StarRocks (5 mismatches)
@'name'string variables (shared fix).((SELECT 1))nesting — the top-level gate admitted onlySELECT/WITHafter the paren, andparseParenSelectcould not recurse.BUILD INDEXrejected in every form like the engine; the dead parser path is removed, four fork-inherited tests replaced with a rejection test (the corpus was already pruned in doris/starrocks: close the parser gaps hidden by the trailing-token swallow (BYT-10084) #401).Testing
-shortsuites 6/6; the strict corpus canary passes untouchedNot in this PR
BYT-10088 (StarRocks
->JSON operator), BYT-10089 (raw-capture family), BYT-10086 (elasticsearch) — tracked separately.🤖 Generated with Claude Code