planner: ignore hidden columns in natural/using join matching (#66069) by qw4990 · Pull Request #67023 · pingcap/tidb

qw4990 · 2026-03-16T03:14:55Z

This is an automated cherry-pick of #66069

What problem does this PR solve?

Issue Number: close #65929

Problem Summary: NATURAL/USING join could incorrectly treat hidden columns (e.g. expression index backing columns) as common columns, producing wrong join conditions and empty results.

What changed and how does it work?

Skip hidden columns when matching common column names for NATURAL/USING joins, so only visible columns participate in the join condition.

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

Bug Fixes
- NATURAL and USING joins now ignore hidden/internal columns when detecting and matching common columns, preventing them from affecting column resolution or ambiguity.
Tests
- Added end-to-end integration tests that exercise NATURAL JOIN behavior with hidden/internal columns to verify correct matching and result ordering.

…p#66069) close pingcap#65929

tiprow · 2026-03-16T03:15:13Z

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-16T03:16:16Z

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: 342c1393-c8ef-441f-9a5f-d72a6ec73c50

📥 Commits

Reviewing files that changed from the base of the PR and between a681191 and e7e647d.

📒 Files selected for processing (2)

tests/integrationtest/r/planner/core/issuetest/planner_issue.result
tests/integrationtest/t/planner/core/issuetest/planner_issue.test

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

tests/integrationtest/t/planner/core/issuetest/planner_issue.test
tests/integrationtest/r/planner/core/issuetest/planner_issue.result

📝 Walkthrough

Walkthrough

Excludes hidden/internal columns from NATURAL/USING join common-column detection and matching in the logical plan builder, so hidden index-derived columns no longer participate in join-name resolution. Adds integration test that reproduces the NATURAL JOIN with expression indexes scenario.

Changes

Cohort / File(s)	Summary
Join Logic `pkg/planner/core/logical_plan_builder.go`	Skip hidden (`IsHidden`) columns when detecting and matching common columns for NATURAL/USING joins by switching to index-based iteration and guards.
Integration tests `tests/integrationtest/t/planner/core/issuetest/planner_issue.test`, `tests/integrationtest/r/planner/core/issuetest/planner_issue.result`	Add test case (Issue65929) that creates tables with expression indexes, inserts rows, and runs `SELECT * FROM t1 NATURAL JOIN t2 ORDER BY t1.a` to validate fix.

Sequence Diagram(s)

(omitted — change is a focused planner bugfix without a new multi-component sequential flow)

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

planner: ignore hidden columns in natural/using join matching #66069 — Similar changes to skip IsHidden columns during NATURAL/USING join common-column detection and adds the same integration test.
planner/core: fix redundant USING-column binding in ALL subquery #66273 — Related modifications to NATURAL/USING column coalescing and mappings in the logical planner.
planner: fix NATURAL JOIN column resolution through LogicalApply #66811 — Changes in planner join column-resolution logic that overlap with this fix.

Suggested labels

needs-cherry-pick-release-8.5, ok-to-test

Suggested reviewers

winoros
guo-shaoge
hawkingrei

Poem

🐰 I hopped through plans both far and near,
hid no columns, made joins clear.
With indices tucked out of sight,
NATURAL joins now do what's right. 🥕

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main change: ignoring hidden columns in NATURAL/USING join matching, addressing the core issue in the changeset.
Description check	✅ Passed	The description follows the template structure with Issue Number, Problem Summary, and What Changed sections filled out. Integration test is marked as included.
Linked Issues check	✅ Passed	Code changes in logical_plan_builder.go exclude hidden columns from NATURAL/USING join matching [`#65929`], directly addressing the issue where expression index backing columns were incorrectly treated as common columns.
Out of Scope Changes check	✅ Passed	All changes are scoped to the linked issue: logical_plan_builder.go modifies join matching logic, and test files add integration test cases reproducing the issue.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ 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

📝 Coding Plan

Generate coding plan for human review comments

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

Tip

You can get early access to new features in CodeRabbit.

Enable the early_access setting to enable early access features such as new models, tools, and more.

coderabbitai

🧹 Nitpick comments (2)

tests/integrationtest/t/planner/core/issuetest/planner_issue.test (1)

631-640: Add a USING join assertion for the same hidden-column repro.

Line 639 covers NATURAL JOIN well, but this fix targets NATURAL/USING behavior. Add one JOIN ... USING (a) query here to protect both paths.

➕ Suggested test extension

 # Issue65929
 drop table if exists t1, t2;
 create table t1(a int);
 create table t2(a int);
 create index idx on t1((a+1));
 create index idx on t2((a+2));
 insert into t1 values (1);
 insert into t2 values (1);
 select * from t1 natural join t2 order by t1.a;
+select * from t1 join t2 using(a) order by a;
 drop table t1, t2;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/integrationtest/t/planner/core/issuetest/planner_issue.test` around
lines 631 - 640, Add a second query exercising the USING join path next to the
existing NATURAL JOIN line that contains "select * from t1 natural join t2 order
by t1.a;": insert a statement that performs the same join using the USING clause
(e.g., "select * from t1 join t2 using (a) order by t1.a;") before the "drop
table t1, t2;" so both NATURAL JOIN and JOIN ... USING (a) are covered in the
test.

tests/integrationtest/r/planner/core/issuetest/planner_issue.result (1)

990-1000: Add a companion USING(a) case to match fix scope.

Line 997 covers NATURAL JOIN, but the fix scope includes USING joins too. A mirrored JOIN ... USING(a) regression would better protect against path-specific regressions.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/integrationtest/r/planner/core/issuetest/planner_issue.result` around
lines 990 - 1000, Add a mirrored test case that exercises a JOIN ... USING(a)
path to match the NATURAL JOIN case: duplicate the existing setup (create tables
t1/t2, create indexes idx on t1((a+1)) and on t2((a+2)), insert values) and
replace "select * from t1 natural join t2 order by t1.a;" with an equivalent
"select * from t1 join t2 using(a) order by t1.a;" (or add it alongside) so the
test covers the USING(a) regression path as well.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@tests/integrationtest/r/planner/core/issuetest/planner_issue.result`:
- Around line 990-1000: Add a mirrored test case that exercises a JOIN ...
USING(a) path to match the NATURAL JOIN case: duplicate the existing setup
(create tables t1/t2, create indexes idx on t1((a+1)) and on t2((a+2)), insert
values) and replace "select * from t1 natural join t2 order by t1.a;" with an
equivalent "select * from t1 join t2 using(a) order by t1.a;" (or add it
alongside) so the test covers the USING(a) regression path as well.

In `@tests/integrationtest/t/planner/core/issuetest/planner_issue.test`:
- Around line 631-640: Add a second query exercising the USING join path next to
the existing NATURAL JOIN line that contains "select * from t1 natural join t2
order by t1.a;": insert a statement that performs the same join using the USING
clause (e.g., "select * from t1 join t2 using (a) order by t1.a;") before the
"drop table t1, t2;" so both NATURAL JOIN and JOIN ... USING (a) are covered in
the test.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 668ed945-54a4-4763-ad47-7f64d29a23fb

📥 Commits

Reviewing files that changed from the base of the PR and between 4436672 and a681191.

📒 Files selected for processing (3)

pkg/planner/core/logical_plan_builder.go
tests/integrationtest/r/planner/core/issuetest/planner_issue.result
tests/integrationtest/t/planner/core/issuetest/planner_issue.test

ti-chi-bot · 2026-03-16T03:43:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AilinKid, 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,hawkingrei]
~~pkg/planner/OWNERS~~ [AilinKid,hawkingrei]

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

ti-chi-bot · 2026-03-16T03:43:09Z

[LGTM Timeline notifier]

Timeline:

2026-03-16 03:32:10.552048903 +0000 UTC m=+155057.639706440: ☑️ agreed by hawkingrei.
2026-03-16 03:43:08.671901445 +0000 UTC m=+155715.759558981: ☑️ agreed by AilinKid.

codecov · 2026-03-16T04:26:06Z

Codecov Report

❌ Patch coverage is 80.00000% with 2 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (release-8.5@4436672). Learn more about missing BASE report.

Additional details and impacted files

@@               Coverage Diff                @@
##             release-8.5     #67023   +/-   ##
================================================
  Coverage               ?   55.1736%           
================================================
  Files                  ?       1821           
  Lines                  ?     653928           
  Branches               ?          0           
================================================
  Hits                   ?     360796           
  Misses                 ?     266442           
  Partials               ?      26690

Flag	Coverage Δ
integration	`38.4005% <80.0000%> (?)`
unit	`65.1571% <40.0000%> (?)`

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

Components	Coverage Δ
dumpling	`52.9954% <0.0000%> (?)`
parser	`∅ <0.0000%> (?)`
br	`55.4847% <0.0000%> (?)`

🚀 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.

EmmaDuDu · 2026-03-16T08:40:58Z

/retest

tiprow · 2026-03-16T08:41:25Z

@EmmaDuDu: 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.

Details

In response to this:

/retest

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.

EmmaDuDu · 2026-03-16T12:46:28Z

/retest

tiprow · 2026-03-16T12:46:53Z

@EmmaDuDu: 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.

Details

In response to this:

/retest

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.

EmmaDuDu · 2026-03-16T14:03:14Z

/retest

EmmaDuDu · 2026-03-16T14:03:15Z

/retest

tiprow · 2026-03-16T14:03:40Z

@EmmaDuDu: 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.

Details

In response to this:

/retest

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.

tiprow · 2026-03-16T14:03:40Z

@EmmaDuDu: 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.

Details

In response to this:

/retest

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.

EmmaDuDu · 2026-03-17T01:00:13Z

/retest

tiprow · 2026-03-17T01:00:38Z

@EmmaDuDu: 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.

Details

In response to this:

/retest

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-17T01:35:55Z

/ok-to-test

qw4990 · 2026-03-17T03:00:17Z

/retest

planner: ignore hidden columns in natural/using join matching (pingca…

a681191

…p#66069) close pingcap#65929

ti-chi-bot Bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/cherry-pick-not-approved size/M Denotes a PR that changes 30-99 lines, ignoring generated files. sig/planner SIG: Planner labels Mar 16, 2026

coderabbitai Bot reviewed Mar 16, 2026

View reviewed changes

fixup

e7e647d

hawkingrei approved these changes Mar 16, 2026

View reviewed changes

ti-chi-bot Bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Mar 16, 2026

AilinKid approved these changes Mar 16, 2026

View reviewed changes

ti-chi-bot Bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Mar 16, 2026

ti-chi-bot Bot added cherry-pick-approved Cherry pick PR approved by release team. and removed do-not-merge/cherry-pick-not-approved labels Mar 16, 2026

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

ti-chi-bot Bot merged commit d7a5796 into pingcap:release-8.5 Mar 17, 2026
20 checks passed

coderabbitai Bot mentioned this pull request Mar 20, 2026

planner: preserve full schema columns in join pruning #67179

Merged

13 tasks

Uh oh!

Conversation

qw4990 commented Mar 16, 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!

tiprow Bot commented Mar 16, 2026

Uh oh!

coderabbitai Bot commented Mar 16, 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 labels

Suggested reviewers

Poem

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot Bot commented Mar 16, 2026

Uh oh!

ti-chi-bot Bot commented Mar 16, 2026

[LGTM Timeline notifier]

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

EmmaDuDu commented Mar 16, 2026

Uh oh!

tiprow Bot commented Mar 16, 2026

Uh oh!

EmmaDuDu commented Mar 16, 2026

Uh oh!

tiprow Bot commented Mar 16, 2026

Uh oh!

EmmaDuDu commented Mar 16, 2026

Uh oh!

EmmaDuDu commented Mar 16, 2026

Uh oh!

tiprow Bot commented Mar 16, 2026

Uh oh!

tiprow Bot commented Mar 16, 2026

Uh oh!

EmmaDuDu commented Mar 17, 2026

Uh oh!

tiprow Bot commented Mar 17, 2026

Uh oh!

qw4990 commented Mar 17, 2026

Uh oh!

qw4990 commented Mar 17, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

qw4990 commented Mar 16, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Mar 16, 2026 •

edited

Loading

codecov Bot commented Mar 16, 2026 •

edited

Loading