planner: ignore hidden columns in natural/using join matching (#66069) - #67005
planner: ignore hidden columns in natural/using join matching (#66069)#67005ti-chi-bot wants to merge 1 commit into
Conversation
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
ti-chi-bot
added
contribution
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughThis PR fixes incorrect NATURAL/USING join results when expression indices are present. The fix enhances column matching in the logical plan builder by excluding internal hidden columns—such as Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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.11.3)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/integrationtest/t/planner/core/issuetest/planner_issue.test (1)
633-642: Consider adding aUSING (a)variant to this regression block.
This change targets NATURAL/USING matching; addingJOIN ... USING (a)with the same expression-index setup would lock coverage for both entry points.🤖 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 633 - 642, Add a second query variant that tests the USING-clause path alongside the existing NATURAL join: after the existing setup with tables t1 and t2 and indexes idx on the expressions (a+1) and (a+2), add a query that performs "JOIN ... USING (a)" between t1 and t2 (same sort/order conditions as the NATURAL join) so the regression covers both NATURAL and USING matching paths; reference the existing objects t1, t2 and the query "select * from t1 natural join t2 order by t1.a" to place the new USING variant immediately alongside it.
🤖 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/logical_plan_builder.go`:
- Around line 874-881: The NATURAL/USING matching loop currently only skips
_tidb_rowid and therefore can wrongly include _tidb_commit_ts and
_tidb_phys_tbl_id in commonLen and equality predicates; add a shared predicate
(e.g., shouldIgnoreNaturalUsingCol) that returns true for col.IsHidden OR
name.ColName equal to model.ExtraHandleName, model.ExtraCommitTSName, or
model.ExtraPhysTblIDName, then replace the ad-hoc checks in the pre-scan, the
coalescing loop that iterates lColumns/rColumns/commonLen, and the USING
ambiguity check (checkAmbiguous) to reuse this helper so all passes consistently
exclude the same internal columns (also note addExtraPhysTblIDColumn4DS appends
phys_tbl_id without hidden flag).
---
Nitpick comments:
In `@tests/integrationtest/t/planner/core/issuetest/planner_issue.test`:
- Around line 633-642: Add a second query variant that tests the USING-clause
path alongside the existing NATURAL join: after the existing setup with tables
t1 and t2 and indexes idx on the expressions (a+1) and (a+2), add a query that
performs "JOIN ... USING (a)" between t1 and t2 (same sort/order conditions as
the NATURAL join) so the regression covers both NATURAL and USING matching
paths; reference the existing objects t1, t2 and the query "select * from t1
natural join t2 order by t1.a" to place the new USING variant immediately
alongside it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8be3272c-e849-4205-b9ad-00159a3863ca
📒 Files selected for processing (3)
pkg/planner/core/logical_plan_builder.gotests/integrationtest/r/planner/core/issuetest/planner_issue.resulttests/integrationtest/t/planner/core/issuetest/planner_issue.test
| // Hidden columns are internal-only and shuold not participate in NATURAL/USING column matching. | ||
| if lColumns[i].IsHidden { | ||
| continue | ||
| } | ||
| for j := commonLen; j < len(rNames); j++ { | ||
| if rColumns[j].IsHidden { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Apply the full internal-column exclusion in the actual coalescing loop.
Line 870 still only skips _tidb_rowid. That means _tidb_commit_ts and _tidb_phys_tbl_id can still be pulled into commonLen and turned into equality predicates here, even though the earlier pre-scan now excludes them. _tidb_phys_tbl_id is especially reachable because addExtraPhysTblIDColumn4DS later in this file appends it without marking it hidden. Please use one shared predicate for all NATURAL/USING matching passes, and reuse it from the USING ambiguity check as well.
Possible fix
- // Natural join should ignore _tidb_rowid
- if lName.ColName.L == "_tidb_rowid" {
- continue
- }
- // Hidden columns are internal-only and shuold not participate in NATURAL/USING column matching.
- if lColumns[i].IsHidden {
+ if shouldIgnoreNaturalUsingCol(lName, lColumns[i]) {
continue
}
for j := commonLen; j < len(rNames); j++ {
- if rColumns[j].IsHidden {
+ if shouldIgnoreNaturalUsingCol(rNames[j], rColumns[j]) {
continue
}shouldIgnoreNaturalUsingCol := func(name *types.FieldName, col *expression.Column) bool {
return col.IsHidden ||
name.ColName.L == model.ExtraHandleName.L ||
name.ColName.L == model.ExtraCommitTSName.L ||
name.ColName.L == model.ExtraPhysTblIDName.L
}Use the same helper in the earlier pre-scan and in checkAmbiguous so the exclusion list cannot drift again.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/planner/core/logical_plan_builder.go` around lines 874 - 881, The
NATURAL/USING matching loop currently only skips _tidb_rowid and therefore can
wrongly include _tidb_commit_ts and _tidb_phys_tbl_id in commonLen and equality
predicates; add a shared predicate (e.g., shouldIgnoreNaturalUsingCol) that
returns true for col.IsHidden OR name.ColName equal to model.ExtraHandleName,
model.ExtraCommitTSName, or model.ExtraPhysTblIDName, then replace the ad-hoc
checks in the pre-scan, the coalescing loop that iterates
lColumns/rColumns/commonLen, and the USING ambiguity check (checkAmbiguous) to
reuse this helper so all passes consistently exclude the same internal columns
(also note addExtraPhysTblIDColumn4DS appends phys_tbl_id without hidden flag).
ti-chi-bot
Bot
added
cherry-pick-approved
|
/retest |
1 similar comment
|
/retest |
|
@ti-chi-bot: The following tests 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. |
ti-chi-bot
Bot
added
cherry-pick-approved
4 participants
This is an automated cherry-pick of #66069
What problem does this PR solve?
Issue Number: close #65929
Problem Summary:
NATURAL/USINGjoin 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/USINGjoins, so only visible columns participate in the join condition.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
Bug Fixes
Tests