planner/core: fix redundant USING-column binding in ALL subquery - #66273
Conversation
ti-chi-bot
Bot
added
the
release-note
|
This PR fixes a planner failure with
|
ti-chi-bot
Bot
added
size/L
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #66273 +/- ##
================================================
- Coverage 77.6677% 77.5035% -0.1642%
================================================
Files 2008 1930 -78
Lines 549877 538707 -11170
================================================
- Hits 427077 417517 -9560
- Misses 121092 121181 +89
+ Partials 1708 9 -1699
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds plan-scoped tracking and resolution for redundant columns produced by NATURAL/USING joins, applies remapping during name-resolution and expression rewriting, exposes LogicalJoin APIs to register/resolve mappings, and adds new schema regression tests and a Bazel go_test target; plus a minor test formatting tweak. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Parser
participant LogicalPlanBuilder
participant LogicalJoin
participant ExprRewriter
Client->>Parser: submit SQL
Parser->>LogicalPlanBuilder: build logical plan (includes JOIN nodes)
LogicalPlanBuilder->>LogicalJoin: RegisterRedundantColumnMapping(redundant, visible)
LogicalJoin-->>LogicalPlanBuilder: store mapping
LogicalPlanBuilder->>ExprRewriter: request name-resolution for WHERE/ORDER/HAVING
ExprRewriter->>LogicalJoin: ResolveRedundantColumn(col)
LogicalJoin-->>ExprRewriter: return canonical column + FieldName (if mapped)
ExprRewriter->>LogicalPlanBuilder: emit resolved ColumnNameEx
LogicalPlanBuilder-->>Client: final plan
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
@pantheon-bot review |
|
Review Complete Findings: 1 issues |
|
/retest |
ti-chi-bot
Bot
added
size/XL
fba27ce to
9a3aa19
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/planner/core/expression_rewriter.go`:
- Around line 2691-2698: The current logic in
resolveRedundantColumnFromNaturalUsingJoinPlan stops traversal whenever an
ancestor join's FullSchema contains col by immediately returning
x.ResolveRedundantColumn(col), which can prematurely return nil if that ancestor
has no mapping; instead, call x.ResolveRedundantColumn(col) and inspect its
result, and only return if a non-nil mappedCol (or non-empty mappedName) is
produced; otherwise continue iterating over x.Children() to let descendant joins
provide the actual mapping. Keep references to x.FullSchema,
x.ResolveRedundantColumn(col),
resolveRedundantColumnFromNaturalUsingJoinPlan(child, col), and x.Children() to
locate and implement the conditional check.
In `@pkg/planner/core/logical_plan_builder.go`:
- Around line 920-922: coalesceCommonColumns currently registers mappings via
p.RegisterRedundantColumnMapping using redundantColMappings too early, before
buildUsingClause/buildNaturalJoin may overwrite p.Schema()/OutputNames for
UPDATE/DELETE; move or defer registering these redundant column mappings until
after buildUsingClause and buildNaturalJoin (or any code path that mutates
p.Schema()/OutputNames for UPDATE/DELETE) so stored output indices are computed
against the final schema/output names; alternatively, conditionally skip
registration when the planner is in an UPDATE/DELETE path and perform
registration once the schema/output names are stabilized.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
pkg/planner/core/casetest/join/join_test.gopkg/planner/core/casetest/schema/BUILD.bazelpkg/planner/core/casetest/schema/cannot_find_column_test.gopkg/planner/core/casetest/schema/main_test.gopkg/planner/core/casetest/schema/testdata/cannot_find_column_suite_in.jsonpkg/planner/core/casetest/schema/testdata/cannot_find_column_suite_out.jsonpkg/planner/core/casetest/schema/testdata/cannot_find_column_suite_xut.jsonpkg/planner/core/expression_rewriter.gopkg/planner/core/logical_plan_builder.gopkg/planner/core/operator/logicalop/logical_join.go
💤 Files with no reviewable changes (1)
- pkg/planner/core/casetest/join/join_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/planner/core/casetest/schema/testdata/cannot_find_column_suite_xut.json
- pkg/planner/core/casetest/schema/cannot_find_column_test.go
- pkg/planner/core/casetest/schema/testdata/cannot_find_column_suite_out.json
- pkg/planner/core/casetest/schema/main_test.go
- pkg/planner/core/casetest/schema/testdata/cannot_find_column_suite_in.json
![pantheon-ai[bot]](https://avatars.githubusercontent.com/in/2265341?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/planner/core/expression_rewriter.go (1)
2573-2704:⚠️ Potential issue | 🟡 MinorPlease include targeted planner test evidence for this planner-core change.
Given this modifies planner name resolution/remapping, please add the exact targeted
go test -run ... -tags=intest,deadlockcommand(s) and whether any rule testdata updates were needed.As per coding guidelines
pkg/planner/**: Run targeted planner unit tests (go test -run <TestName> -tags=intest,deadlock) and update rule testdata when needed for changes to planner rules or logical/physical plans.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/expression_rewriter.go` around lines 2573 - 2704, Add targeted planner unit tests that cover name-resolution and remapping around JOIN ... USING/NATURAL by exercising the modified functions toColumn, findFieldNameFromNaturalUsingJoin, and resolveRedundantColumnFromNaturalUsingJoinPlan; run the tests with the exact commands: go test ./pkg/planner -run TestExpressionRewriter -tags=intest,deadlock and (if you add rule/plan-output cases) go test ./pkg/planner -run TestPlannerRules -tags=intest,deadlock; if any planner rule or logical/physical plan output changed, update the corresponding rule testdata files under planner testdata and re-run the above commands until they pass.
♻️ Duplicate comments (1)
pkg/planner/core/expression_rewriter.go (1)
2587-2598:⚠️ Potential issue | 🔴 CriticalPotential DML mis-remap still needs an explicit safety guard.
Line 2587 and Line 2629 apply redundant-column remapping for all WHERE/HAVING rewrites. Please verify this path is safe for UPDATE/DELETE plans where join schema/output names may be reset later; otherwise this can remap to a wrong visible column and alter row qualification.
#!/bin/bash set -euo pipefail echo "== ResolveRedundantColumn internals ==" fd 'logical_join.go$' pkg/planner/core -x sh -c ' f="$1" echo "-- $f" rg -n -C4 "type LogicalJoin|RedundantColsToOutputIdx|RegisterRedundantColumnMapping|ResolveRedundantColumn|UniqueID|ColumnIndex|Schema\\(\\)\\.Columns" "$f" ' sh {} echo echo "== UPDATE/DELETE schema reset points ==" fd 'logical_plan_builder.go$' pkg/planner/core -x sh -c ' f="$1" echo "-- $f" rg -n -C4 "inUpdateStmt|inDeleteStmt|SetSchemaAndNames|MergeSchema|RegisterRedundantColumnMapping" "$f" ' sh {} echo echo "== Remap call sites ==" rg -n -C3 "resolveRedundantColumnFromNaturalUsingJoinPlan\\(|ResolveRedundantColumn\\(" pkg/planner/coreExpected verification result: if mapping is still stored/used by output index across schema resets, this path remains unsafe for DML and should be switched to UniqueID-based remap or DML-gated.
Also applies to: 2629-2637
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/expression_rewriter.go` around lines 2587 - 2598, The redundant-column remapping in expression_rewriter.go (triggered when er.clause() is whereClause/havingClause and name.Redundant && name.OrigTblName != "") currently calls resolveRedundantColumnFromNaturalUsingJoinPlan(planCtx.plan, column) and assigns mappedCol/mappedName which can be unsafe for UPDATE/DELETE flows because schema/output names may be reset later; verify and change the fix by either (A) gating this remap when the plan is in DML (use the plan builder flags/inUpdateStmt or inDeleteStmt context) so you do not remap for DML plans, or (B) change the remap to use a UniqueID-based mapping (the same identity used by ResolveRedundantColumn/RegisterRedundantColumnMapping/RedundantColsToOutputIdx) instead of relying on output column names, ensuring the mapping survives Schema/SetSchemaAndNames/MergeSchema resets; locate the call site in expression_rewriter.go (the block using resolveRedundantColumnFromNaturalUsingJoinPlan) and implement one of these two guards/changes and update any related callers of ResolveRedundantColumn to keep consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/planner/core/expression_rewriter.go`:
- Around line 2573-2704: Add targeted planner unit tests that cover
name-resolution and remapping around JOIN ... USING/NATURAL by exercising the
modified functions toColumn, findFieldNameFromNaturalUsingJoin, and
resolveRedundantColumnFromNaturalUsingJoinPlan; run the tests with the exact
commands: go test ./pkg/planner -run TestExpressionRewriter
-tags=intest,deadlock and (if you add rule/plan-output cases) go test
./pkg/planner -run TestPlannerRules -tags=intest,deadlock; if any planner rule
or logical/physical plan output changed, update the corresponding rule testdata
files under planner testdata and re-run the above commands until they pass.
---
Duplicate comments:
In `@pkg/planner/core/expression_rewriter.go`:
- Around line 2587-2598: The redundant-column remapping in
expression_rewriter.go (triggered when er.clause() is whereClause/havingClause
and name.Redundant && name.OrigTblName != "") currently calls
resolveRedundantColumnFromNaturalUsingJoinPlan(planCtx.plan, column) and assigns
mappedCol/mappedName which can be unsafe for UPDATE/DELETE flows because
schema/output names may be reset later; verify and change the fix by either (A)
gating this remap when the plan is in DML (use the plan builder
flags/inUpdateStmt or inDeleteStmt context) so you do not remap for DML plans,
or (B) change the remap to use a UniqueID-based mapping (the same identity used
by
ResolveRedundantColumn/RegisterRedundantColumnMapping/RedundantColsToOutputIdx)
instead of relying on output column names, ensuring the mapping survives
Schema/SetSchemaAndNames/MergeSchema resets; locate the call site in
expression_rewriter.go (the block using
resolveRedundantColumnFromNaturalUsingJoinPlan) and implement one of these two
guards/changes and update any related callers of ResolveRedundantColumn to keep
consistency.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/planner/core/casetest/schema/cannot_find_column_test.gopkg/planner/core/expression_rewriter.gopkg/planner/core/logical_plan_builder.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/planner/core/casetest/schema/cannot_find_column_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/planner/core/casetest/schema/cannot_find_column_test.go (1)
66-74: Add one inline assertion for the exact= ALL (subquery)failing shape.These explicit checks are useful, but they currently cover nested/having/order-by only. Adding one direct inline query for the original
JOIN ... USING+ qualified predicate += ALL (subquery)case would make this file self-protecting even if testdata is later edited.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/casetest/schema/cannot_find_column_test.go` around lines 66 - 74, Add a single inline assertion that reproduces the original failing shape: use tk.MustQuery with a query that uses JOIN ... USING(id) plus a qualified predicate and an "= ALL (subquery)" construct (for example selecting t1.id from t1 JOIN t3 USING(id) WHERE t1.left_v = 93 AND t1.id = ALL (SELECT id FROM t3 WHERE t3.id BETWEEN 10 AND 20)) and assert the expected result with Check(testkit.Rows("10")); place this new tk.MustQuery(...) alongside the existing related assertions (the lines that call tk.MustQuery currently labeled /* issue:66272-nested */, /* issue:66272-having */, /* issue:66272-orderby */) so the file self-validates the original case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/planner/core/casetest/schema/cannot_find_column_test.go`:
- Around line 66-74: Add a single inline assertion that reproduces the original
failing shape: use tk.MustQuery with a query that uses JOIN ... USING(id) plus a
qualified predicate and an "= ALL (subquery)" construct (for example selecting
t1.id from t1 JOIN t3 USING(id) WHERE t1.left_v = 93 AND t1.id = ALL (SELECT id
FROM t3 WHERE t3.id BETWEEN 10 AND 20)) and assert the expected result with
Check(testkit.Rows("10")); place this new tk.MustQuery(...) alongside the
existing related assertions (the lines that call tk.MustQuery currently labeled
/* issue:66272-nested */, /* issue:66272-having */, /* issue:66272-orderby */)
so the file self-validates the original case.
|
/retest |
|
@pantheon-bot review |
|
Review Complete Findings: 2 issues |
|
@pantheon-bot review |
|
Review Complete Findings: 0 issues ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: guo-shaoge, qw4990 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
|
/retest |
3 participants
What problem does this PR solve?
Issue Number: close #66272
Problem Summary:
JOIN ... USING/NATURAL JOINkeeps two different column views in the planner:FullSchema/FullNamesare used for name resolution and still contain the redundant side of common columns.Join.Schema()/OutputNames()are the executable join outputs and only keep the canonical visible column.Because of that split, a qualified predicate such as
t3.id = 10could be resolved fromFullSchema, but the resolved redundant column was not present inJoin.Schema()anymore. The planner then carried that redundant column into later optimization, and physicalResolveIndicesfinally failed with a missing-column error.The original repro is a
JOIN ... USINGquery with a qualified predicate and= ALL (subquery):This could fail with:
Follow-up review also showed that the fix must preserve several correctness constraints:
NATURAL JOINwith view columns),SELECT t_right.col,USINGcolumns,What changed and how does it work?
USING/NATURAL JOINconstruction.LogicalJoinnow storesredundant column -> canonical visible outputmappings when common columns are coalesced.expression_rewriter,WHERE/HAVINGremap a redundant column to the canonical join output only when:OrigTblName != ""),USING/NATURAL JOIN,RetType.havingWindowAndOrderbyExprResolver, apply the same idea forORDER BY/HAVINGname resolution.findColFromNaturalUsingJoinkeeps reading the original redundant-side identity fromFullSchema/FullNames, soSELECT t3.id FROM t1 JOIN t3 USING(id)still reportst3in result-field metadata.UPDATE/DELETE.Cases and Fixes
JOIN ... USING+ qualified predicate += ALL (subquery)t3.idinto physical planningSELECT t3.id FROM t1 JOIN t3 USING(id)t3FullSchema/FullNames; do not reuse predicate remap for select-list metadataNATURAL JOINwith a view / derived columnOrigTblName != ""USING(id)such asVARCHARvsINTRetTypeLEFT JOIN/RIGHT JOINwith null-preserving semanticsUPDATE/DELETE ... USING(id)Cases covered by regression tests
JOIN ... USING+ qualified predicate += ALL (subquery)missing-column reproHAVINGandORDER BYon qualified redundant columnsNATURAL JOINwith a view column must not be remappedSELECT t3.id FROM t1 JOIN t3 USING(id)must still reportt3VARCHAR/INT USING(id)must remain type-safeUPDATE/DELETEwithUSINGLEFT JOIN/RIGHT JOINnull-side semantics remain unchangedCheck List
Tests
Test commands:
/Users/weizhenwang/.gvm/gos/go1.25.7/bin/go test -run '^TestSchemaCannotFindColumnRegression$' --tags=intest ./pkg/planner/core/casetest/schema/Users/weizhenwang/.gvm/gos/go1.25.7/bin/go test -run 'TestJoinRegression/on' --tags=intest ./pkg/planner/core/casetest/joinSide effects
Documentation
Release note