vtgate: validate the declared column list length of CTEs and derived tables - #20633
vtgate: validate the declared column list length of CTEs and derived tables#20633GrahamCampbell wants to merge 3 commits into
Conversation
vitess-bot
Bot
added
NeedsWebsiteDocsUpdate
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
40f4d18 to
7197760
Compare
7197760 to
ae94a0a
Compare
ae94a0a to
073c6e5
Compare
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
073c6e5 to
4865729
Compare
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
The seed selected * from user_metadata, which testdata/schemas/user.sql defines with five columns, against a single-column recursive term. MySQL rejects that with error 1222, so no plan recorded for it could ever have run. cte_cases.json is not one of the files the plan e2e test executes, so nothing caught it. Spell out all five columns in the recursive term instead. Drop the declared column list variant. Writing it with a matching count, x(a, b, c, d, e), panics in extractColumnsFromCTE, which compares the declared list against the length of the unexpanded projection: one StarExpr. The version that was here passed only because a single declared column happened to equal that length, which is not a property worth resting a test on. That panic belongs with the column list validation in #20633, not here, and the remaining case already covers this fix. Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
4865729 to
af92512
Compare
arthurschreiber
added
Backport to: release-23.0
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #20633 +/- ##
===========================================
+ Coverage 69.67% 87.10% +17.43%
===========================================
Files 1614 21 -1593
Lines 216793 5040 -211753
===========================================
- Hits 151044 4390 -146654
+ Misses 65749 650 -65099
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thanks I left some feedback on that other PR. I'll rebase this one and apply your feedback here once that one lands. |
|
@GrahamCampbell I think this PR can land before the draft PR I opened, no? No need to be blocked on an unrelated fix. |
0c2bdd6 to
0ced915
Compare
|
You're right — the star-seed panic pre-dates this change and lands in the same place with or without it, so there's nothing to wait for. I've applied the naming suggestion, kept it squashed, and rebased this and #20631 on current main. |
Signed-off-by: Graham Campbell <hello@gjcampbell.co.uk>
…tables Signed-off-by: Graham Campbell <hello@gjcampbell.co.uk>
0ced915 to
3500478
Compare
|
Two fixes from the latest review round, now at 3500478. Validation is lazier, matching MySQL: a use inside another definition only counts if that definition is itself used, so |
![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: 3500478d87
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Signed-off-by: Graham Campbell <hello@gjcampbell.co.uk>
|
Follow-up to the stack re-review: both findings here plus the one on #20631 are addressed, at 02ad377 here and 6efd084 there. An unpairable declared list read through an unused dependency chain no longer panics: extractColumnsFromCTE falls back to the select list names, matching the CTETable rule, and the same unpairing now marks the table non-authoritative, so a dormant self-referencing body reading a declared name stays silent as MySQL does (checked against 8.4.10). Derived-table mismatches inside unused definitions defer through the same mechanism as CTE references: recorded against the enclosing definition and surfaced only when a use reaches it, with the dormant table kept on unpaired select-list names non-authoritatively so nothing indexes past the declared list. On #20631, CTETable.getExprFor now maps declared names positionally, so projecting the declared names of an unmerged recursive CTE plans instead of failing with VT13001. One deliberate deviation from the suggested patch: when the list pairs, the declared names mask the seed names entirely rather than keeping a name-based fallback, because that fallback resolves the wrong column when declared names swap the seed aliases; a golden pins the swapped case and MySQL 8.4.10 confirms the positional semantics. The scratch test file is deleted, ErrorContains is actually applied this time, and the two longest comment blocks are trimmed. The unmerged-path UNION DISTINCT loss went to #20703 as scope expansion rather than inline feedback here. |
Description
A declared column list whose length does not match the select list it names passes analysis and fails much later inside vtgate, or does not fail at all. Recursive CTEs hit recovered panics or internal errors while resolving columns, ordinary CTEs and direct derived tables hit an index out of range panic through the derived table path, and a too long list on that path is silently accepted with the extra names ignored. MySQL rejects every form up front. Since malformed client SQL can drive the recovered panic paths, this is probably worth backporting.
The validation lives on the paths every such definition flows through: derived table collection, which is also where non-recursive CTEs are inlined, and the construction of the table info for a recursive CTE reference. Both run after star expansion, so a star the analyzer has expanded is validated like any other projection; an unexpanded star cannot be fully validated, but a list with fewer names than select expressions is still rejected, since every star expands to at least one column. A mismatch is rejected with
VT03033, the error vtgate already returns for the same mistake on insert row aliases, and MySQL's error 1353 for every non-self-referencing form.Self-referencing recursive CTEs follow MySQL's resolution model, which testing against 8.0.46 and 8.4.10 pinned down: a declared list that pairs with the seed select renames the columns everywhere and hides the seed names, while a list that cannot be paired leaves the recursive reference on the seed select names, with the count check applying where the CTE is used. When the term then references a declared name, MySQL reports an unknown column (1054); vtgate's strict analysis surfaces the count check instead, because the unresolved column parks as a sharded error that only reports when no hard error follows, and unsharded pass-through returns MySQL's own error. Definitions that are never referenced are accepted, exactly as MySQL accepts them, and a reference inside another definition only counts as a use if that definition is itself used, so unused dependency chains stay silent too. A dormant body binds safely even when it reads a column an unpairable list fails to provide, and a derived table mismatch inside an unused definition defers the same way, surfacing only when a use reaches its enclosing definition. #20631 relies on this validation when resolving declared columns positionally, and only projections with a statically known cardinality plan far enough to reach that resolution. Thirty-nine single-fault analyzer cases cover the short, long, star, union, matching, name-visibility, unused, and unused-chain forms across recursive CTEs, plain CTEs, and derived tables, plus a qualified-star pair that stacks on the corrected expansion in #20759, and all existing plans are unchanged.
Related Issue(s)
#20631 depends on this change.
Checklist
Deployment Notes
CTEs and derived tables with a declared column list whose length does not match their select list previously caused a recovered panic or an internal error, or were silently accepted with the extra names ignored. They now fail with the error MySQL returns, except inside a self-referencing recursive CTE definition, where an unpairable list exposes the seed select names to the recursive reference and sharded queries report the count mismatch where MySQL reports an unknown column. Unreferenced definitions stay accepted, as in MySQL, including derived table mismatches nested inside them. No migrations or configuration changes.
AI Disclosure
This PR was written primarily by Fable with review from GPT 5.6 Sol.
graph LR subgraph json [JSON support] direction TB PR20625["#20625 (merged)<br>mysql/json: fix MarshalTo discarding accumulated output for nested blob and bit values"] PR20632["#20632<br>vtgate: preserve IN value lists in complex aggregate projections"] PRA["#20691<br>evalengine: disable the static IN hash table for JSON operands"] PR20682["#20682 (draft)<br>evalengine: support constant-folded JSON values as literals"] PR20683["#20683 (draft)<br>evalengine, sqlparser: MySQL comparison domains; nested BETWEEN parentheses"] PR20626["#20626 (draft)<br>vtgate: support cross-shard JSON_ARRAYAGG and JSON_OBJECTAGG"] PR20625 --> PR20682 PRA --> PR20682 PR20682 --> PR20683 PR20632 --> PR20626 PR20683 --> PR20626 end subgraph union [Union routing] direction TB PR20628["#20628<br>vtgate: track per-source copies of pushed join predicates so merges skip them"] PR20629["#20629<br>vtgate: fix None routing handling when merging unions"] PR20630["#20630 (draft)<br>vtgate: merge unions on join-predicate-free routings when all sources agree"] PR20755["#20755<br>vtgate: fix union merging through reference-table alternates"] PR20756["#20756 (draft)<br>vtgate: merge empty reference branches through rewritten copies"] PR20628 --> PR20630 PR20629 --> PR20630 PR20630 --> PR20756 PR20755 --> PR20756 end subgraph cte [Recursive CTEs] direction TB PR20759["#20759<br>vtgate: expand qualified stars without JOIN USING coalescing"] PR20633["#20633 (draft)<br>vtgate: validate the declared column list length of CTEs and derived tables"] PR20631["#20631 (draft)<br>vtgate: bind recursive CTE column filters as arguments in unmerged term queries"] PR20759 --> PR20633 PR20633 --> PR20631 end json ~~~ union ~~~ cte