planner: fix NATURAL JOIN column resolution through LogicalApply by qw4990 · Pull Request #66811 · pingcap/tidb

qw4990 · 2026-03-09T09:39:05Z

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

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

Summary by CodeRabbit

Tests
- Added extensive test coverage and test data for NATURAL JOIN with correlated subqueries, including multiple case variations, expected plans/results, and harness changes to preload and expose suite data. Sharding for the test run increased to exercise more parallel cases.
Bug Fixes
- Improved column/field resolution so NATURAL JOINs involving correlated subqueries resolve correctly through additional join constructs.
Documentation
- Updated planner test map descriptions to reflect NATURAL JOIN resolution through Apply.

pantheon-ai · 2026-03-09T09:39:10Z

@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.}

tiprow · 2026-03-09T09:39:21Z

Hi @qw4990. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Details

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.

coderabbitai · 2026-03-09T09:40:10Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 68e1ce22-a9a3-41b0-9131-a0f6385c372c

📥 Commits

Reviewing files that changed from the base of the PR and between f5d8181 and dc0e297.

📒 Files selected for processing (3)

pkg/planner/core/casetest/correlated/correlated_test.go
pkg/planner/core/casetest/correlated/testdata/correlated_subquery_suite_out.json
pkg/planner/core/casetest/correlated/testdata/correlated_subquery_suite_xut.json

🚧 Files skipped from review as they are similar to previous changes (3)

pkg/planner/core/casetest/correlated/testdata/correlated_subquery_suite_out.json
pkg/planner/core/casetest/correlated/testdata/correlated_subquery_suite_xut.json
pkg/planner/core/casetest/correlated/correlated_test.go

📝 Walkthrough

Walkthrough

Adds 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

Cohort / File(s)	Summary
Planner field/column resolution `pkg/planner/core/expression_rewriter.go`, `pkg/planner/core/logical_plan_builder.go`	Extend `findFieldNameFromNaturalUsingJoin` and `findColFromNaturalUsingJoin` to descend into `LogicalApply` by delegating lookup to its first child, enabling resolution through Apply nodes.
Correlated tests & harness `pkg/planner/core/casetest/correlated/correlated_test.go`, `pkg/planner/core/casetest/correlated/main_test.go`, `pkg/planner/core/casetest/correlated/BUILD.bazel`	Add new test(s) exercising NATURAL/LEFT JOIN with correlated subqueries, introduce test-data registry and accessor, add `data = glob(["testdata/**"])`, increase `shard_count` 2→3, and add deps `//pkg/testkit/testdata`, `//pkg/testkit/testmain`.
Testdata (new suites) `pkg/planner/core/casetest/correlated/testdata/correlated_subquery_suite_in.json`, `pkg/planner/core/casetest/correlated/testdata/correlated_subquery_suite_out.json`, `pkg/planner/core/casetest/correlated/testdata/correlated_subquery_suite_xut.json`	Add input/output/XUT JSON files defining the TestNaturalJoinWithCorrelatedSubquery suite (multiple cases covering natural/inner/left join variants) with expected plans and results.
Docs / mapping `.agents/skills/tidb-test-guidelines/references/planner-case-map.md`	Expand correlated-tests description to include NATURAL JOIN resolution through Apply nodes.

Sequence Diagram(s)

(omitted)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

planner/core: fix redundant USING-column binding in ALL subquery #66273 — Modifies NATURAL/USING join name-resolution paths in planner functions (related code paths in expression_rewriter.go / logical_plan_builder.go).

Suggested reviewers

winoros
AilinKid
guo-shaoge

Poem

"🐇 I hopped through plans and tests tonight,
Applied my nose where names took flight.
Through Apply I peeked, found columns true,
Testdata stacked, the cases grew.
A rabbit's cheer — the join's in sight!"

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately and specifically describes the main change: fixing NATURAL JOIN column resolution when the join is wrapped in LogicalApply.
Description check	✅ Passed	The description covers all required template sections with clear problem statement, solution explanation, and proper issue linking (`#60602`), though release-note could be more detailed.
Linked Issues check	✅ Passed	The PR fully addresses issue `#60602` by adding LogicalApply case handling to column resolution functions and comprehensive test coverage for NATURAL JOIN with correlated subqueries.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing NATURAL JOIN column resolution through LogicalApply: code fixes, test additions, test data, and documentation updates are all aligned with issue `#60602`.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

qw4990 · 2026-03-09T09:45:01Z

@pantheon-ai please review

pantheon-ai · 2026-03-09T09:45:09Z

Review Complete

Findings: 0 issues
Posted: 0
Duplicates/Skipped: 0

_{ℹ️ Learn more details on Pantheon AI.}

hawkingrei · 2026-03-09T09:45:22Z

/ok-to-test

qw4990 · 2026-03-09T09:54:40Z

/retest

coderabbitai

🧹 Nitpick comments (1)

pkg/planner/core/casetest/correlated/correlated_test.go (1)
82-88: Please cover the reported NATURAL LEFT JOIN shape too.

This regression locks the Apply-wrapped NATURAL JOIN path, but issue #60602 was triggered by NATURAL LEFT JOIN with a right-side qualified column. Swapping one assertion to NATURAL LEFT JOIN or 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

📥 Commits

Reviewing files that changed from the base of the PR and between 960e7c5 and c1ff220.

📒 Files selected for processing (4)

pkg/planner/core/casetest/correlated/BUILD.bazel
pkg/planner/core/casetest/correlated/correlated_test.go
pkg/planner/core/expression_rewriter.go
pkg/planner/core/logical_plan_builder.go

qw4990 · 2026-03-09T10:03:30Z

/rebuild

qw4990 · 2026-03-09T10:21:24Z

/test check-dev
/test mysql-test

tiprow · 2026-03-09T10:21:49Z

@qw4990: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test fast_test_tiprow

/test tidb_parser_test

Use /test all to run all jobs.

Details

In response to this:

/test check-dev
/test mysql-test

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.

qw4990 · 2026-03-09T11:05:05Z

/retest

pantheon-ai

✅ Code looks good. No issues found.

codecov · 2026-03-09T11:20:42Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.5992%. Comparing base (960e7c5) to head (dc0e297).
⚠️ Report is 14 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #66811        +/-   ##
================================================
- Coverage   77.6702%   77.5992%   -0.0711%     
================================================
  Files          2009       1933        -76     
  Lines        549988     540254      -9734     
================================================
- Hits         427177     419233      -7944     
+ Misses       121102     121003        -99     
+ Partials       1709         18      -1691

Flag	Coverage Δ
integration	`41.2518% <33.3333%> (-6.9359%)`	⬇️
unit	`76.6315% <100.0000%> (+0.3808%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	`56.7974% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`48.8990% <ø> (-12.0267%)`	⬇️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

qw4990 · 2026-03-09T11:32:43Z

/test unit-test

tiprow · 2026-03-09T11:33:05Z

@qw4990: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test fast_test_tiprow

/test tidb_parser_test

Use /test all to run all jobs.

Details

In response to this:

/test unit-test

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.

qw4990 · 2026-03-09T11:35:02Z

/rebuild

qw4990 · 2026-03-09T11:45:13Z

/test mysql-test

tiprow · 2026-03-09T11:45:36Z

@qw4990: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test fast_test_tiprow

/test tidb_parser_test

Use /test all to run all jobs.

Details

In response to this:

/test mysql-test

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.

qw4990 · 2026-03-09T14:09:27Z

/test mysql-test

tiprow · 2026-03-09T14:09:52Z

@qw4990: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test fast_test_tiprow

/test tidb_parser_test

Use /test all to run all jobs.

Details

In response to this:

/test mysql-test

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.

hawkingrei · 2026-03-09T14:24:51Z

/retest

ti-chi-bot · 2026-03-10T02:23:21Z

[LGTM Timeline notifier]

Timeline:

2026-03-09 14:29:01.1043521 +0000 UTC m=+273972.616409771: ☑️ agreed by hawkingrei.
2026-03-10 02:23:20.599415518 +0000 UTC m=+316832.111473179: ☑️ agreed by AilinKid.

hawkingrei · 2026-03-10T04:14:25Z

/retest

qw4990 · 2026-03-10T05:02:36Z

/test unit-test

tiprow · 2026-03-10T05:02:58Z

@qw4990: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test fast_test_tiprow

/test tidb_parser_test

Use /test all to run all jobs.

Details

In response to this:

/test unit-test

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.

hawkingrei · 2026-03-10T05:10:56Z

/retest

hawkingrei · 2026-03-10T08:05:47Z

/retest

ti-chi-bot · 2026-03-10T08:22:24Z

[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

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [AilinKid,guo-shaoge,hawkingrei]
~~pkg/planner/OWNERS~~ [AilinKid,guo-shaoge,hawkingrei]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

hawkingrei · 2026-03-10T09:09:20Z

I updated some test cases.

hawkingrei · 2026-03-10T10:25:59Z

/retest

hawkingrei · 2026-03-10T11:40:58Z

/retest

qw4990 added 2 commits March 9, 2026 17:33

fixup

8c7deda

fixup

c1ff220

ti-chi-bot Bot added the release-note-none Denotes a PR that doesn't merit a release note. label Mar 9, 2026

ti-chi-bot Bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. sig/planner SIG: Planner labels Mar 9, 2026

qw4990 added AI-Correction Bugfix by AI and removed sig/planner SIG: Planner labels Mar 9, 2026

ti-chi-bot Bot added the ok-to-test Indicates a PR is ready to be tested. label Mar 9, 2026

coderabbitai Bot reviewed Mar 9, 2026

View reviewed changes

pantheon-ai Bot reviewed Mar 9, 2026

View reviewed changes

ti-chi-bot Bot removed the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Mar 10, 2026

guo-shaoge approved these changes Mar 10, 2026

View reviewed changes

planner/test: merge natural join correlated regression

3a150d2

ti-chi-bot Bot added sig/planner SIG: Planner size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 10, 2026

planner/test: move correlated natural join case to testdata

f5d8181

ti-chi-bot Bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 10, 2026

planner/test: enrich correlated natural join test data

dc0e297

ti-chi-bot Bot merged commit 8582172 into pingcap:master Mar 10, 2026
31 checks passed

coderabbitai Bot mentioned this pull request Mar 28, 2026

planner: fix DEFAULT() resolution after NATURAL JOIN #67388

Merged

13 tasks

This was referenced Apr 7, 2026

planner: correlate subquery rule #66206

Merged

[Do Not Merge/ POC only] planner: support rewrite join to apply rule #67693

Closed

Uh oh!

Conversation

qw4990 commented Mar 9, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

Summary by CodeRabbit

Uh oh!

pantheon-ai Bot commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tiprow Bot commented Mar 9, 2026

Uh oh!

coderabbitai Bot commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

❌ Failed checks (1 warning)

Uh oh!

qw4990 commented Mar 9, 2026

Uh oh!

pantheon-ai Bot commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hawkingrei commented Mar 9, 2026

Uh oh!

qw4990 commented Mar 9, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

qw4990 commented Mar 9, 2026

Uh oh!

qw4990 commented Mar 9, 2026

Uh oh!

tiprow Bot commented Mar 9, 2026

Uh oh!

qw4990 commented Mar 9, 2026

Uh oh!

pantheon-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

qw4990 commented Mar 9, 2026

Uh oh!

tiprow Bot commented Mar 9, 2026

Uh oh!

qw4990 commented Mar 9, 2026

Uh oh!

qw4990 commented Mar 9, 2026

Uh oh!

tiprow Bot commented Mar 9, 2026

Uh oh!

qw4990 commented Mar 9, 2026

Uh oh!

tiprow Bot commented Mar 9, 2026

Uh oh!

hawkingrei commented Mar 9, 2026

Uh oh!

ti-chi-bot Bot commented Mar 10, 2026

[LGTM Timeline notifier]

Uh oh!

hawkingrei commented Mar 10, 2026

Uh oh!

qw4990 commented Mar 10, 2026

Uh oh!

tiprow Bot commented Mar 10, 2026

Uh oh!

hawkingrei commented Mar 10, 2026

Uh oh!

qw4990 commented Mar 9, 2026 •

edited by coderabbitai Bot

Loading

pantheon-ai Bot commented Mar 9, 2026 •

edited

Loading

coderabbitai Bot commented Mar 9, 2026 •

edited

Loading

pantheon-ai Bot commented Mar 9, 2026 •

edited

Loading

codecov Bot commented Mar 9, 2026 •

edited

Loading