Fix planner panic when pushing predicates into a UNION of literals - #20853
Fix planner panic when pushing predicates into a UNION of literals#20853vatsalpatel wants to merge 1 commit into
UNION of literals#20853Conversation
`Union.AddPredicate` rewrites a predicate once per source, swapping every column for the expression found at the same offset in that branch. On a branch that selects only literals, the rewritten predicate has no table dependency left, so `Horizon.AddPredicate` can't push it anywhere and wraps the branch in a `Filter` instead. That breaks the invariant `Union.GetSelectFor` relies on, and the next predicate pushed into the same union panics with: VT13001: [BUG] expected all sources of the UNION to be horizons A join against such a derived table sends two predicates at the union, the join condition and the WHERE, so it fails every time. The union now checks that every source can take its rewritten predicate before pushing any of them. When one can't, nothing is pushed and the predicate stays in a `Filter` on top of the horizon, which is where the two existing bail-outs in `Horizon.AddPredicate` already put it. The check also covers the two other shapes that end up under a `Filter` for the same reason and panic identically today: a branch predicate that resolves across several tables, and one that resolves to an aggregate. Two planner test cases are added. The first reproduces the panic without the fix. No existing plan expectation changed. Fixes vitessio#20610 Signed-off-by: vatsalpatel <vatsalpatel.me@gmail.com>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
新建 flags
If a workflow is added or modified:
Backward compatibility
|
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Extends UNION predicate pushdown handling so derived-table predicates over UNIONs are only pushed when all UNION sources can absorb them, preventing UNION sources from being wrapped in Filters and breaking planning invariants.
Changes:
- Add
Union.canPushPredicate()and refactor predicate-rewrite logic into helpers (columnOffsets,predicateForSource). - Update
Horizon.AddPredicate()to conditionally push predicates into UNIONs or fall back to wrapping in a Filter. - Add plan test cases for joins against derived tables containing UNIONs of literal-only SELECTs with predicates on the derived table.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| go/vt/vtgate/planbuilder/testdata/union_cases.json | Adds regression test cases covering predicate pushdown with derived tables containing UNIONs. |
| go/vt/vtgate/planbuilder/operators/union.go | Introduces pushability check and refactors UNION predicate rewriting per source. |
| go/vt/vtgate/planbuilder/operators/horizon.go | Changes predicate pushdown behavior to avoid breaking UNION horizon invariants; adds absorb/rewrite helpers. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if !union.canPushPredicate(ctx, expr) { | ||
| return newFilter(h, expr) | ||
| } | ||
| // If we have a derived table on top of a UNION, we can let the UNION do the expression rewriting |
There was a problem hiding this comment.
I tried folding it into one pass and it doesn't work, because the filter has to be placed above the Horizon and Union.AddPredicate can't put it there. It has no reference to its parent, and returning newFilter(u, expr) puts the filter between the Horizon and the UNION instead.
I built the suggestion as two variants and ran the planner tests on both. The test that separates them is already in this PR in union_cases.json
| func (u *Union) AddPredicate(ctx *plancontext.PlanningContext, expr sqlparser.Expr) Operator { | ||
| offsets := make(map[string]int) | ||
| offsets := u.columnOffsets() | ||
|
|
||
| exprPerSource := u.predicatePerSource(ctx, expr, offsets) | ||
| for i, src := range u.Sources { | ||
| u.Sources[i] = src.AddPredicate(ctx, exprPerSource[i]) | ||
| } | ||
|
|
||
| return u | ||
| } |
There was a problem hiding this comment.
I tried returning a Filter wrapping the UNION but it didn't work. It is the second variant above, and it panics in the SQL builder with cant add WHERE to *sqlparser.Union at SQL_builder.go:151, because a Filter inside a union derived table cannot be serialized back into SQL. The filter has to go above the Horizon, which is where the two existing bail-outs in Horizon.AddPredicate already put theirs.
Description
Joining a table to a derived table built from literal only SELECTs kills the planner:
Union.AddPredicaterewrites a predicate once per source, swapping every column for the expression at the same offset in that branch. On a literal only branch,r.ordinal <= 30000becomes0 <= 30000, which has no table dependency left, soHorizon.AddPredicatehas nowhere to push it and wraps the branch in aFilter.Union.GetSelectForneeds every source to be a horizon, so the next predicate pushed at the same union panics. A join sends two predicates at it, the join condition and the WHERE, so this shape fails every time.The union now checks that every source can take its rewritten predicate before pushing any of them. If one can't, nothing is pushed and the predicate stays in a
Filteron top of the horizon. That's where the two bail-outs already inHorizon.AddPredicateput their filters, and it's the only placement that works: putting it between the horizon and the union panics later in the SQL builder, since aFilterinside a union derived table has no WHERE clause to be written into.The check is "would this source stop being a horizon" rather than "does the rewritten predicate have no dependencies", so it also catches the two other shapes that panic identically today, a branch predicate that resolves across several tables and one that resolves to an aggregate.
The query above now plans as a single scatter, with the predicate pushed into the same route it would have reached anyway:
Related Issue(s)
Fixes #20610
Checklist
Two cases added to
union_cases.json. Verified the first fails without the fix, with the exactVT13001from the issue. No existing plan expectation in any testdata file changed.Deployment Notes
Queries that failed with
VT13001: [BUG] expected all sources of the UNION to be horizonsnow plan and run. Predicate placement changes for derived tables whose UNION branches select only literals: the predicate is applied above the derived table rather than inside each branch. Results are unchanged.AI Disclosure
I used Claude Code to understand the package and to add the tests, and implemented the fix myself.