planner: fix NATURAL JOIN column resolution through LogicalApply#66811
planner: fix NATURAL JOIN column resolution through LogicalApply#66811qw4990 wants to merge 5 commits intopingcap:masterfrom
Conversation
ti-chi-bot
bot
added
the
release-note-none
|
@qw4990 I've received your pull request and will start the review. I'll conduct a thorough review covering code quality, potential issues, and implementation details. ⏳ This process typically takes 10-30 minutes depending on the complexity of the changes. ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
bot
added
size/S
|
Hi @qw4990. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds LogicalApply traversal for NATURAL/USING name/column resolution, plus new correlated-subquery tests and testdata; updates test harness and BUILD to include testdata, deps, and increase test shard count. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Command failed Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@pantheon-ai please review |
|
Review Complete Findings: 0 issues ℹ️ Learn more details on Pantheon AI. |
|
/ok-to-test |
|
/retest |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/planner/core/casetest/correlated/correlated_test.go (1)
82-88: Please cover the reportedNATURAL LEFT JOINshape too.This regression locks the Apply-wrapped NATURAL JOIN path, but issue
#60602was triggered byNATURAL LEFT JOINwith a right-side qualified column. Swapping one assertion toNATURAL LEFT JOINor adding a third case would pin the exact failing shape with almost no extra test cost.🧪 Minimal test tweak
- tk.MustQuery("select 0 from t t1 where exists (select a2.a from t a1 natural join t a2 where t1.a = (select min(t1.a)))").Check( + tk.MustQuery("select 0 from t t1 where exists (select a2.a from t a1 natural left join t a2 where t1.a = (select min(t1.a)))").Check( testkit.Rows("0", "0"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/casetest/correlated/correlated_test.go` around lines 82 - 88, Add a test covering the NATURAL LEFT JOIN shape: extend the existing correlated_test.go cases that exercise NATURAL JOIN wrapped in an Apply by adding (or replacing one of) the tk.MustQuery assertions to use "natural left join" (e.g. mirror the two existing queries that use "natural join" but change one to "natural left join" so the right-side qualified column path is exercised); this will pin the regression triggered by NATURAL LEFT JOIN and ensures findFieldNameFromNaturalUsingJoin's Apply-recursion is validated.
🤖 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/correlated/correlated_test.go`:
- Around line 82-88: Add a test covering the NATURAL LEFT JOIN shape: extend the
existing correlated_test.go cases that exercise NATURAL JOIN wrapped in an Apply
by adding (or replacing one of) the tk.MustQuery assertions to use "natural left
join" (e.g. mirror the two existing queries that use "natural join" but change
one to "natural left join" so the right-side qualified column path is
exercised); this will pin the regression triggered by NATURAL LEFT JOIN and
ensures findFieldNameFromNaturalUsingJoin's Apply-recursion is validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 461bcdc7-ce0f-4194-8eb4-a5f1ab75c4ea
📒 Files selected for processing (4)
pkg/planner/core/casetest/correlated/BUILD.bazelpkg/planner/core/casetest/correlated/correlated_test.gopkg/planner/core/expression_rewriter.gopkg/planner/core/logical_plan_builder.go
|
/rebuild |
|
/test check-dev |
|
@qw4990: The specified target(s) for Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retest |
![pantheon-ai[bot]](https://avatars.githubusercontent.com/in/2265341?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #66811 +/- ##
================================================
- Coverage 77.6702% 77.0581% -0.6122%
================================================
Files 2009 1930 -79
Lines 549988 539529 -10459
================================================
- Hits 427177 415751 -11426
- Misses 121102 123760 +2658
+ Partials 1709 18 -1691
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/test unit-test |
|
@qw4990: The specified target(s) for Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/rebuild |
|
/test mysql-test |
|
@qw4990: The specified target(s) for Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/test mysql-test |
|
@qw4990: The specified target(s) for Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retest |
|
/test mysql-test |
|
@qw4990: The specified target(s) for Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
| func TestNaturalJoinWithCorrelatedSubquery(t *testing.T) { | ||
| store := testkit.CreateMockStore(t) | ||
| tk := testkit.NewTestKit(t, store) | ||
| tk.MustExec("use test") | ||
| tk.MustExec("drop table if exists t") | ||
| tk.MustExec("create table t (a int)") | ||
| tk.MustExec("insert into t values (1), (2)") | ||
|
|
||
| // NATURAL JOIN wraps the join inside a LogicalApply when there's a | ||
| // correlated scalar subquery; findFieldNameFromNaturalUsingJoin must | ||
| // recurse through the Apply to resolve the right-side qualified column. | ||
| tk.MustQuery("select 0 from t t1 where exists (select a2.a from t a1 natural join t a2 where t1.a = (select min(t1.a)))").Check( | ||
| testkit.Rows("0", "0")) | ||
| tk.MustQuery("select 0 from t t1 where exists (select a1.a from t a1 natural join t a2 where t1.a = (select min(t1.a)))").Check( | ||
| testkit.Rows("0", "0")) | ||
| } |
There was a problem hiding this comment.
Where should we put the test? @hawkingrei
From #66760, these tests themselves also introduce build costs.
There was a problem hiding this comment.
There are only two tests in this directory, so the overhead should be manageable.
|
I'm fine with the change. We can merge it tomorrow if we don't know the best practice of the test. |
|
/test mysql-test |
|
@qw4990: The specified target(s) for Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/test mysql-test |
|
@qw4990: The specified target(s) for Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
ti-chi-bot
bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
|
/retest |
|
/test unit-test |
|
@qw4990: The specified target(s) for Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retest |
|
@qw4990: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/retest |
ti-chi-bot
bot
added
sig/planner
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AilinKid, guo-shaoge, hawkingrei 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
size/L
|
I updated some test cases. |
5 participants
What problem does this PR solve?
Issue Number: close #60602
Problem Summary: planner: fix NATURAL JOIN column resolution through LogicalApply
What changed and how does it work?
The correlated scalar subquery causes the plan builder to wrap the NATURAL JOIN inside a LogicalApply node. Two functions that resolve qualified columns from a NATURAL JOIN's FullSchema/FullNames — findFieldNameFromNaturalUsingJoin and findColFromNaturalUsingJoin — only handle *LogicalJoin in their type switches. Since LogicalApply is a distinct Go type (even though it embeds LogicalJoin), the switch falls through, failing to resolve the column and causing either an "unknown column" error or a nil-pointer panic.
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
Tests
Bug Fixes
Documentation