planner: tidb_opt_ordering_index_selectivity_ratio for merge join (#67588)#67906
planner: tidb_opt_ordering_index_selectivity_ratio for merge join (#67588)#67906ti-chi-bot wants to merge 1 commit intopingcap:release-8.5from
Conversation
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
ti-chi-bot
added
do-not-merge/hold
|
@terry1purcell This PR has conflicts, I have hold it. |
|
[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 |
|
This cherry pick PR is for a release branch and has not yet been approved by triage owners. To merge this cherry pick:
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. |
|
@ti-chi-bot: ## If you want to know how to resolve it, please read the guide in TiDB Dev Guide. 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 ti-community-infra/tichi repository. |
ti-chi-bot
bot
added
size/XXL
📝 WalkthroughWalkthroughThis PR introduces merge join physical operator planning with cost estimation, refactors index join construction into static enumeration and completion phases, adds ordering-aware child row count estimation for Apply operations, and provides supporting utility functions for physical plan manipulation and selectivity ratio handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Planner
participant MergeJoinPlan as MergeJoin<br/>Planning
participant IndexJoinPlan as IndexJoin<br/>Planning
participant CostCalc as Cost<br/>Calculator
participant SelectivityRatio as Selectivity<br/>Ratio Handler
Planner->>MergeJoinPlan: Enumerate join candidates<br/>(left/right keys, sort props)
MergeJoinPlan->>MergeJoinPlan: Validate collation &<br/>match sort properties
MergeJoinPlan->>CostCalc: Request cost (v1/v2)
CostCalc->>SelectivityRatio: GetOptOrderingIdxSelRatio
SelectivityRatio-->>CostCalc: ratio value
CostCalc-->>MergeJoinPlan: cost estimate
Planner->>IndexJoinPlan: constructIndexJoinStatic<br/>(before inner built)
IndexJoinPlan->>IndexJoinPlan: Push IndexJoinRuntimeProp<br/>to inner child
Planner->>IndexJoinPlan: Inner task complete
IndexJoinPlan->>IndexJoinPlan: completePhysicalIndexJoin<br/>(fill runtime fields)
IndexJoinPlan->>CostCalc: Request cost
CostCalc-->>IndexJoinPlan: cost estimate
MergeJoinPlan->>Planner: Compare costs
IndexJoinPlan->>Planner: Compare costs
Planner->>Planner: Select operator<br/>with lower cost
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
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.11.4)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 |
|
@ti-chi-bot: 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. |
|
@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. |
![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
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/exhaust_physical_plans.go (1)
2587-2605:⚠️ Potential issue | 🔴 CriticalResolve the merge conflict and fix three critical issues in Apply planning with ordering support.
The file contains unresolved merge conflict markers (lines 2584–2597) that prevent compilation. Beyond that, the incoming branch has two bugs:
Undefined variable
stats0(line 2591): This variable does not exist in theexhaustPhysicalPlans4LogicalApplyfunction scope. Usela.Children()[0].StatsInfo().RowCountinstead to access the outer child's stats.Unused
outerExpectedCnt: The computed ordering-aware count is not wired into Apply's properties. Line 2596 still passes hardcodedmath.MaxFloat64for the outer child's expected count, ignoring the computed value.The proposed fix correctly:
- Replaces undefined
stats0with the outer child's stats viala.Children()[0].StatsInfo().RowCount- Wires
outerExpectedCntinto the outer property'sExpectedCntfield so ordering-based selectivity ratio actually influences planningProposed fix
outerExpectedCnt := math.MaxFloat64 if !prop.IsSortItemEmpty() { - outerExpectedCnt = physicalop.CalcChildExpectedCnt(la.SCtx(), prop, stats0.RowCount, la.StatsInfo().RowCount) + outerExpectedCnt = physicalop.CalcChildExpectedCnt( + la.SCtx(), prop, la.Children()[0].StatsInfo().RowCount, la.StatsInfo().RowCount, + ) } apply := physicalop.PhysicalApply{ PhysicalHashJoin: *join, OuterSchema: la.CorCols, @@ la.StatsInfo().ScaleByExpectCnt(prop.ExpectedCnt), la.QueryBlockOffset(), - &property.PhysicalProperty{ExpectedCnt: math.MaxFloat64, SortItems: prop.SortItems, CTEProducerStatus: prop.CTEProducerStatus}, + &property.PhysicalProperty{ExpectedCnt: outerExpectedCnt, SortItems: prop.SortItems, CTEProducerStatus: prop.CTEProducerStatus}, &property.PhysicalProperty{ExpectedCnt: math.MaxFloat64, CTEProducerStatus: prop.CTEProducerStatus})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/exhaust_physical_plans.go` around lines 2587 - 2605, The merge left conflict markers in exhaustPhysicalPlans4LogicalApply and the ordering-aware outer row estimate isn't applied: replace the undefined stats0 with la.Children()[0].StatsInfo().RowCount when computing outerExpectedCnt, remove the conflict markers, and then pass outerExpectedCnt into the PhysicalApply.Init call by setting the outer property's ExpectedCnt to outerExpectedCnt (instead of math.MaxFloat64) so the computed ordering-aware selectivity is used; ensure the rest of the PhysicalApply.Init arguments (inner property ExpectedCnt) remain unchanged.
🤖 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/cardinality/selectivity_test.go`:
- Around line 1421-1426: Replace the incorrect usage of ast.NewCIStr with the
existing pmodel.NewCIStr in the TableByName calls so the code compiles and
matches the rest of the file; specifically update the two calls around
InfoSchema() where TableByName(context.Background(), ast.NewCIStr("test"),
ast.NewCIStr("t1")) and TableByName(..., ast.NewCIStr("t2")) are used to instead
call TableByName(context.Background(), pmodel.NewCIStr("test"),
pmodel.NewCIStr("t1")) and pmodel.NewCIStr("t2"), keeping the surrounding
variables tb1/tb2, err, tbl1 and require.NoError checks unchanged.
In `@pkg/planner/core/exhaust_physical_plans.go`:
- Around line 739-750: The static enumeration path currently ignores null-safe
equality flags (isNullEQ) from p.GetJoinKeys(), allowing joins using `<=>` to be
considered for IndexJoin; update the enumeration to mirror constructIndexJoin by
checking the isNullEQ slice (e.g., any true entry) after calling GetJoinKeys
(with outerIdx logic) and skip/reject building IndexJoin candidates when a
null-safe equality is present so IndexJoin is not admitted for `<=>` semantics.
---
Outside diff comments:
In `@pkg/planner/core/exhaust_physical_plans.go`:
- Around line 2587-2605: The merge left conflict markers in
exhaustPhysicalPlans4LogicalApply and the ordering-aware outer row estimate
isn't applied: replace the undefined stats0 with
la.Children()[0].StatsInfo().RowCount when computing outerExpectedCnt, remove
the conflict markers, and then pass outerExpectedCnt into the PhysicalApply.Init
call by setting the outer property's ExpectedCnt to outerExpectedCnt (instead of
math.MaxFloat64) so the computed ordering-aware selectivity is used; ensure the
rest of the PhysicalApply.Init arguments (inner property ExpectedCnt) remain
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f6a4faea-719d-4351-ba85-abfdcb94b2be
📒 Files selected for processing (6)
pkg/bindinfo/testdata/binding_auto_suite_out.jsonpkg/planner/cardinality/BUILD.bazelpkg/planner/cardinality/selectivity_test.gopkg/planner/core/exhaust_physical_plans.gopkg/planner/core/operator/physicalop/physical_merge_join.gopkg/planner/core/operator/physicalop/physical_utils.go
| is := dom.InfoSchema() | ||
| tb1, err := is.TableByName(context.Background(), ast.NewCIStr("test"), ast.NewCIStr("t1")) | ||
| require.NoError(t, err) | ||
| tbl1 := tb1.Meta() | ||
| tb2, err := is.TableByName(context.Background(), ast.NewCIStr("test"), ast.NewCIStr("t2")) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 '\bast\.NewCIStr\b|\bpmodel\.NewCIStr\b' pkg/planner/cardinality/selectivity_test.goRepository: pingcap/tidb
Length of output: 4559
Replace ast.NewCIStr with pmodel.NewCIStr for consistency and to fix compile error.
Lines 1422 and 1425 use ast.NewCIStr, which is not imported in this file. Every other TableByName call in the file uses the existing pmodel.NewCIStr alias. These lines will fail to compile as written.
Proposed fix
- tb1, err := is.TableByName(context.Background(), ast.NewCIStr("test"), ast.NewCIStr("t1"))
+ tb1, err := is.TableByName(context.Background(), pmodel.NewCIStr("test"), pmodel.NewCIStr("t1"))
require.NoError(t, err)
tbl1 := tb1.Meta()
- tb2, err := is.TableByName(context.Background(), ast.NewCIStr("test"), ast.NewCIStr("t2"))
+ tb2, err := is.TableByName(context.Background(), pmodel.NewCIStr("test"), pmodel.NewCIStr("t2"))
require.NoError(t, err)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| is := dom.InfoSchema() | |
| tb1, err := is.TableByName(context.Background(), ast.NewCIStr("test"), ast.NewCIStr("t1")) | |
| require.NoError(t, err) | |
| tbl1 := tb1.Meta() | |
| tb2, err := is.TableByName(context.Background(), ast.NewCIStr("test"), ast.NewCIStr("t2")) | |
| require.NoError(t, err) | |
| is := dom.InfoSchema() | |
| tb1, err := is.TableByName(context.Background(), pmodel.NewCIStr("test"), pmodel.NewCIStr("t1")) | |
| require.NoError(t, err) | |
| tbl1 := tb1.Meta() | |
| tb2, err := is.TableByName(context.Background(), pmodel.NewCIStr("test"), pmodel.NewCIStr("t2")) | |
| require.NoError(t, err) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/planner/cardinality/selectivity_test.go` around lines 1421 - 1426,
Replace the incorrect usage of ast.NewCIStr with the existing pmodel.NewCIStr in
the TableByName calls so the code compiles and matches the rest of the file;
specifically update the two calls around InfoSchema() where
TableByName(context.Background(), ast.NewCIStr("test"), ast.NewCIStr("t1")) and
TableByName(..., ast.NewCIStr("t2")) are used to instead call
TableByName(context.Background(), pmodel.NewCIStr("test"),
pmodel.NewCIStr("t1")) and pmodel.NewCIStr("t2"), keeping the surrounding
variables tb1/tb2, err, tbl1 and require.NoError checks unchanged.
| joinType := p.JoinType | ||
| var ( | ||
| innerJoinKeys []*expression.Column | ||
| outerJoinKeys []*expression.Column | ||
| isNullEQ []bool | ||
| ) | ||
| if outerIdx == 0 { | ||
| outerJoinKeys, innerJoinKeys, isNullEQ, _ = p.GetJoinKeys() | ||
| } else { | ||
| innerJoinKeys, outerJoinKeys, isNullEQ, _ = p.GetJoinKeys() | ||
| } | ||
| chReqProps := make([]*property.PhysicalProperty, 2) |
There was a problem hiding this comment.
Preserve the null-safe equality rejection in the static index-join path.
constructIndexJoin rejects hasNullEQ because IndexJoin does not support null-safe join keys. The static enumeration path should keep the same guard; otherwise <=> joins can be admitted and later completed as IndexJoin candidates with incorrect NULL semantics.
🐛 Proposed fix
var (
innerJoinKeys []*expression.Column
outerJoinKeys []*expression.Column
isNullEQ []bool
+ hasNullEQ bool
)
if outerIdx == 0 {
- outerJoinKeys, innerJoinKeys, isNullEQ, _ = p.GetJoinKeys()
+ outerJoinKeys, innerJoinKeys, isNullEQ, hasNullEQ = p.GetJoinKeys()
} else {
- innerJoinKeys, outerJoinKeys, isNullEQ, _ = p.GetJoinKeys()
+ innerJoinKeys, outerJoinKeys, isNullEQ, hasNullEQ = p.GetJoinKeys()
}
+ // TODO: support null equal join keys for index join.
+ if hasNullEQ {
+ return nil
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| joinType := p.JoinType | |
| var ( | |
| innerJoinKeys []*expression.Column | |
| outerJoinKeys []*expression.Column | |
| isNullEQ []bool | |
| ) | |
| if outerIdx == 0 { | |
| outerJoinKeys, innerJoinKeys, isNullEQ, _ = p.GetJoinKeys() | |
| } else { | |
| innerJoinKeys, outerJoinKeys, isNullEQ, _ = p.GetJoinKeys() | |
| } | |
| chReqProps := make([]*property.PhysicalProperty, 2) | |
| joinType := p.JoinType | |
| var ( | |
| innerJoinKeys []*expression.Column | |
| outerJoinKeys []*expression.Column | |
| isNullEQ []bool | |
| hasNullEQ bool | |
| ) | |
| if outerIdx == 0 { | |
| outerJoinKeys, innerJoinKeys, isNullEQ, hasNullEQ = p.GetJoinKeys() | |
| } else { | |
| innerJoinKeys, outerJoinKeys, isNullEQ, hasNullEQ = p.GetJoinKeys() | |
| } | |
| // TODO: support null equal join keys for index join. | |
| if hasNullEQ { | |
| return nil | |
| } | |
| chReqProps := make([]*property.PhysicalProperty, 2) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/planner/core/exhaust_physical_plans.go` around lines 739 - 750, The
static enumeration path currently ignores null-safe equality flags (isNullEQ)
from p.GetJoinKeys(), allowing joins using `<=>` to be considered for IndexJoin;
update the enumeration to mirror constructIndexJoin by checking the isNullEQ
slice (e.g., any true entry) after calling GetJoinKeys (with outerIdx logic) and
skip/reject building IndexJoin candidates when a null-safe equality is present
so IndexJoin is not admitted for `<=>` semantics.
2 participants
This is an automated cherry-pick of #67588
What problem does this PR solve?
Issue Number: ref #62034 , close #67595
Problem Summary:
What changed and how does it work?
Extends the tidb_opt_ordering_index_selectivity_ratio (“ordering penalty”) modeling to MergeJoin by reusing a shared child-ExpectedCnt calculation, and adds a planner/cardinality test intended to validate the new cost behavior.
Changes:
Introduce physicalop.CalcChildExpectedCnt to centralize ordered-join child ExpectedCnt scaling + ordering penalty logic.
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
New Features
Tests