planner/core: discourage degenerate index joins when probe rows approach a full scan | tidb-test=pr/2738 - #67646
Conversation
Signed-off-by: guo-shaoge <shaoge1994@163.com>
…e/tidb into refine_cost_indexjoin_hashjoin
ti-chi-bot
Bot
added
release-note-none
|
@guo-shaoge 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a scan-ratio pruning heuristic for IndexJoin enumeration controlled by a new sysvar ChangesIndexJoin Scan-Ratio Pruning & Tests
sequenceDiagram
participant TestRunner as Test Runner
participant Session as SessionVars/Sysvar
participant Planner as Optimizer (Enumerator & Cost)
participant Stats as Table Stats (HistColl)
TestRunner->>Session: set tidb_cost_model_version, set tidb_opt_index_join_max_scan_rows_ratio
TestRunner->>Stats: load precomputed stats for t_small / t_big
TestRunner->>Planner: request EXPLAIN (STRAIGHT_JOIN query)
Planner->>Session: read IndexJoinMaxScanRowsRatio
Planner->>Stats: fetch probe / full-scan row estimates
Planner->>Planner: shouldPruneIndexJoinByScanRatio(...) -> prune or enumerate IndexJoin
Planner->>Planner: compute IndexJoin / HashJoin costs
Planner->>TestRunner: return chosen plan
TestRunner->>TestRunner: assert plan matches expected
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Signed-off-by: guo-shaoge <shaoge1994@163.com>
…e/tidb into refine_cost_indexjoin_hashjoin
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #67646 +/- ##
================================================
- Coverage 77.7059% 76.4697% -1.2363%
================================================
Files 1991 1992 +1
Lines 552094 563492 +11398
================================================
+ Hits 429010 430901 +1891
- Misses 122164 132080 +9916
+ Partials 920 511 -409
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![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
🤖 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/casetest/cbotest/cbo_test.go`:
- Around line 173-189: The test sets session variable
tidb_opt_index_join_scan_ratio_threshold to 0.5 inside the
TestReproHashJoinIssue block and never restores it, causing later tests to
inherit the altered scan-ratio; fix this by capturing the original session value
(or the default) before calling tk.MustExec("set
@@session.tidb_opt_index_join_scan_ratio_threshold = 0.5") and restore it after
the repro loop (e.g., with a defer or an explicit tk.MustExec to reset the
variable) so subsequent tests in TestAnalyzeSuiteRegression/IndexJoin revert to
the prior behavior.
In `@pkg/planner/core/plan_cost_ver2_test.go`:
- Around line 557-580: Add a regression test in
pkg/planner/core/plan_cost_ver2_test.go that exercises the no-full-scan-stats
fallback (the branch fixed to avoid a panic when full-scan rows cannot be
derived from the probe tree). Specifically, extend the existing test block that
uses MustExec/MustQuery/Explain format=verbose (the same pattern around the
IndexJoin scan ratio checks) with an additional query variant that produces a
probe tree where full-scan row counts are unavailable (e.g., use a derived table
or a probe with expressions/filters that prevent deriving full-scan rows), set
@@session.tidb_opt_index_join_scan_ratio_threshold to trigger the fallback, run
Explain and assert the plan is produced (no panic) and that the planner picks a
valid join type; use the same helpers (tk.MustExec, tk.MustQuery,
strconv.ParseFloat, require.*) and keep assertions similar to the surrounding
tests so the regression is covered.
🪄 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: ab62511a-c931-472c-886c-a34a8daf9f4c
⛔ Files ignored due to path filters (1)
pkg/planner/core/casetest/cbotest/testdata/stats.zipis excluded by!**/*.zip
📒 Files selected for processing (10)
pkg/planner/core/casetest/cbotest/cbo_test.gopkg/planner/core/casetest/cbotest/testdata/analyze_suite_in.jsonpkg/planner/core/casetest/cbotest/testdata/analyze_suite_out.jsonpkg/planner/core/casetest/cbotest/testdata/analyze_suite_xut.jsonpkg/planner/core/operator/physicalop/physical_utils.gopkg/planner/core/plan_cost_ver2.gopkg/planner/core/plan_cost_ver2_test.gopkg/sessionctx/vardef/tidb_vars.gopkg/sessionctx/variable/session.gopkg/sessionctx/variable/sysvar.go
| // Test IndexJoin scan ratio threshold | ||
| tk.MustExec("drop table if exists t_outer, t_inner") | ||
| tk.MustExec("create table t_outer(a int primary key, b int, key(b))") | ||
| tk.MustExec("create table t_inner(a int primary key, b int, key(b))") | ||
| tk.MustExec("insert into t_outer values (1,1),(2,2),(3,3),(4,4),(5,5)") | ||
| tk.MustExec("insert into t_inner values (1,1),(2,2),(3,3),(4,4),(5,5)") | ||
| tk.MustExec("analyze table t_outer, t_inner") | ||
| tk.MustExec("set @@session.tidb_opt_hash_join_cost_factor=1") | ||
| tk.MustExec("set @@session.tidb_opt_merge_join_cost_factor=100") | ||
| tk.MustExec("set @@session.tidb_opt_index_join_cost_factor=1") | ||
| tk.MustExec("set @@session.tidb_opt_index_join_scan_ratio_threshold=0") | ||
| rs = tk.MustQuery("explain format=verbose select * from t_outer o straight_join t_inner i on o.b = i.b").Rows() | ||
| planCost1, err1 = strconv.ParseFloat(rs[0][2].(string), 64) | ||
| require.Nil(t, err1) | ||
| require.Contains(t, rs[0][0].(string), "IndexJoin") | ||
| tk.MustExec("set @@session.tidb_opt_index_join_scan_ratio_threshold=0.8") | ||
| rs = tk.MustQuery("explain format=verbose select * from t_outer o straight_join t_inner i on o.b = i.b").Rows() | ||
| planCost2, err2 = strconv.ParseFloat(rs[0][2].(string), 64) | ||
| require.Nil(t, err2) | ||
| require.Less(t, planCost1, planCost2) | ||
| require.Contains(t, rs[0][0].(string), "HashJoin") | ||
| tk.MustExec("set @@session.tidb_opt_index_join_scan_ratio_threshold=0") | ||
| tk.MustExec("set @@session.tidb_opt_hash_join_cost_factor=1") | ||
| tk.MustExec("set @@session.tidb_opt_merge_join_cost_factor=1") |
There was a problem hiding this comment.
Add a regression for the no-full-scan-stats fallback.
This covers the analyzed-stats happy path only. The PR also fixes a panic when full-scan rows cannot be derived from the probe tree, but nothing here drives that branch, so the panic fix can regress unnoticed.
Based on learnings, for planner rule or logical/physical plan changes, perform targeted planner unit tests and update rule testdata when needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/planner/core/plan_cost_ver2_test.go` around lines 557 - 580, Add a
regression test in pkg/planner/core/plan_cost_ver2_test.go that exercises the
no-full-scan-stats fallback (the branch fixed to avoid a panic when full-scan
rows cannot be derived from the probe tree). Specifically, extend the existing
test block that uses MustExec/MustQuery/Explain format=verbose (the same pattern
around the IndexJoin scan ratio checks) with an additional query variant that
produces a probe tree where full-scan row counts are unavailable (e.g., use a
derived table or a probe with expressions/filters that prevent deriving
full-scan rows), set @@session.tidb_opt_index_join_scan_ratio_threshold to
trigger the fallback, run Explain and assert the plan is produced (no panic) and
that the planner picks a valid join type; use the same helpers (tk.MustExec,
tk.MustQuery, strconv.ParseFloat, require.*) and keep assertions similar to the
surrounding tests so the regression is covered.
qw4990
left a comment
qw4990
left a comment
There was a problem hiding this comment.
Could we implement this strategy on the physical optimization stage (like skyline pruning for index selection) instead of the cost model? The cost model is not forced; even we add this large penalty, we can't guarantee the optimizer will choose HashJoin instead of IndexJoin.
Signed-off-by: guo-shaoge <shaoge1994@163.com>
Signed-off-by: guo-shaoge <shaoge1994@163.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/planner/core/exhaust_physical_plans.go (2)
542-548: 💤 Low valueAdd a brief doc comment explaining the stats semantics
HistColl.RealtimeCountis the total live row count of the underlying table (not the filtered selectivity-adjusted count), which is the right denominator for the scan-ratio check. Future readers unfamiliar with this distinction may mistakenly believeStatsInfo.RowCount(already filtered) was the intent.📝 Proposed doc comment
+// getProbeFullScanRowsForIndexJoinPrune returns the estimated total row count +// of the inner (probe) side as if it were a full table scan, by reading +// HistColl.RealtimeCount from the plan's StatsInfo. This is distinct from +// StatsInfo.RowCount, which reflects selectivity-adjusted estimates. +// Returns 0 when reliable statistics are unavailable (safe: prune is skipped). func getProbeFullScanRowsForIndexJoinPrune(p base.LogicalPlan) float64 {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/planner/core/exhaust_physical_plans.go` around lines 542 - 548, Add a short doc comment above getProbeFullScanRowsForIndexJoinPrune explaining that HistColl.RealtimeCount represents the total live row count of the underlying table (unfiltered) and is intentionally used as the denominator for scan-ratio checks, as opposed to StatsInfo.RowCount which is a selectivity-adjusted/filtered count; mention this distinction so future readers understand why RealtimeCount is used for the probe full-scan rows calculation.
550-563: 💤 Low valueAdd a doc comment for the scan-ratio heuristic
The function encodes a non-obvious heuristic. Per the coding guidelines, a brief comment should explain the intent and constraints so future readers can reason about the pruning conditions without consulting external documents.
📝 Proposed doc comment
+// shouldPruneIndexJoinByScanRatio reports whether an IndexJoin candidate +// should be pruned because its total estimated probe work approaches a full +// inner-side scan. It computes (buildRows * probeRowsOne) / innerFullScanRows +// and prunes when the ratio is >= threshold. +// Pruning is skipped when threshold is 0 (disabled), buildRows is <= 1 +// (degenerate outer side), or reliable full-scan statistics are unavailable. func shouldPruneIndexJoinByScanRatio(threshold, buildRows, probeRowsOne float64, probe base.LogicalPlan) bool {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/planner/core/exhaust_physical_plans.go` around lines 550 - 563, The function shouldPruneIndexJoinByScanRatio encodes a heuristic determining when to prune an index join based on the ratio of total probe rows to inner full-scan rows but lacks documentation; add a concise doc comment above shouldPruneIndexJoinByScanRatio that explains the intent (prune when estimated total probe work divided by inner full-scan rows meets/exceeds threshold), documents parameters (threshold, buildRows, probeRowsOne, and the probe plan used to compute innerFullScanRows via getProbeFullScanRowsForIndexJoinPrune), and notes important constraints/early-exit conditions (threshold <= 0, buildRows <= 1, innerFullScanRows <= 0, or probeRowsTot <= 0 all return false) so future readers understand why those checks exist.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/planner/core/exhaust_physical_plans.go`:
- Around line 516-518: The pruning call to shouldPruneIndexJoinByScanRatio must
be skipped when the user has set a force index-join hint; modify
enumerateIndexJoinByOuterIdx (the place that currently calls
shouldPruneIndexJoinByScanRatio) to first detect a force index-join hint (use
the existing join hint indicator, e.g. p.PreferJoinType or the same flag used by
tryToEnumerateIndexJoin/exhaustPhysicalPlans4LogicalJoin to signal
INL_JOIN/INL_HASH_JOIN) and only call shouldPruneIndexJoinByScanRatio when no
force hint is active, so force hints override the scan-ratio heuristic and avoid
misleading “inapplicable” warnings from recordIndexJoinHintWarnings.
---
Nitpick comments:
In `@pkg/planner/core/exhaust_physical_plans.go`:
- Around line 542-548: Add a short doc comment above
getProbeFullScanRowsForIndexJoinPrune explaining that HistColl.RealtimeCount
represents the total live row count of the underlying table (unfiltered) and is
intentionally used as the denominator for scan-ratio checks, as opposed to
StatsInfo.RowCount which is a selectivity-adjusted/filtered count; mention this
distinction so future readers understand why RealtimeCount is used for the probe
full-scan rows calculation.
- Around line 550-563: The function shouldPruneIndexJoinByScanRatio encodes a
heuristic determining when to prune an index join based on the ratio of total
probe rows to inner full-scan rows but lacks documentation; add a concise doc
comment above shouldPruneIndexJoinByScanRatio that explains the intent (prune
when estimated total probe work divided by inner full-scan rows meets/exceeds
threshold), documents parameters (threshold, buildRows, probeRowsOne, and the
probe plan used to compute innerFullScanRows via
getProbeFullScanRowsForIndexJoinPrune), and notes important
constraints/early-exit conditions (threshold <= 0, buildRows <= 1,
innerFullScanRows <= 0, or probeRowsTot <= 0 all return false) so future readers
understand why those checks exist.
🪄 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: b238bf8b-288f-4e6e-bd65-c832cdb64110
📒 Files selected for processing (4)
pkg/planner/core/casetest/cbotest/cbo_test.gopkg/planner/core/exhaust_physical_plans.gopkg/planner/core/plan_cost_ver2.gopkg/planner/core/plan_cost_ver2_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/planner/core/plan_cost_ver2.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/planner/core/casetest/cbotest/cbo_test.go
- pkg/planner/core/plan_cost_ver2_test.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/planner/core/exhaust_physical_plans.go (1)
516-518:⚠️ Potential issue | 🟠 Major | ⚡ Quick winForce INL hints are still bypassed by early scan-ratio pruning
At Line 516, pruning runs before force-hint handling, so
INL_JOIN/INL_HASH_JOINcan still be eliminated and later reported as “inapplicable” even though the real cause is heuristic pruning. Please skip this pruning path when a force index-join hint is present.Proposed minimal fix
- if shouldPruneIndexJoinByScanRatio(p.SCtx().GetSessionVars().IndexJoinMaxProbeScanRatio, buildRows, avgInnerRowCnt, p.Children()[1-outerIdx]) { + forceIndexJoin := p.PreferAny( + h.PreferLeftAsINLJInner, h.PreferRightAsINLJInner, + h.PreferLeftAsINLHJInner, h.PreferRightAsINLHJInner, + h.PreferLeftAsINLMJInner, h.PreferRightAsINLMJInner, + ) + if !forceIndexJoin && + shouldPruneIndexJoinByScanRatio(p.SCtx().GetSessionVars().IndexJoinMaxProbeScanRatio, buildRows, avgInnerRowCnt, p.Children()[1-outerIdx]) { return nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/planner/core/exhaust_physical_plans.go` around lines 516 - 518, The pruning call to shouldPruneIndexJoinByScanRatio in exhaust_physical_plans.go is being executed before honoring force index-join hints, causing INL_JOIN/INL_HASH_JOIN to be incorrectly discarded; fix by skipping that pruning branch when a forced index-join hint is present for this join node (i.e., detect INL_JOIN or INL_HASH_JOIN force hint on the current join) and only call shouldPruneIndexJoinByScanRatio(IndexJoinMaxProbeScanRatio, buildRows, avgInnerRowCnt, p.Children()[1-outerIdx]) when no such force hint exists; locate the check around the existing shouldPruneIndexJoinByScanRatio call and gate it on the presence/absence of the force-index-join hint for this plan node.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@pkg/planner/core/exhaust_physical_plans.go`:
- Around line 516-518: The pruning call to shouldPruneIndexJoinByScanRatio in
exhaust_physical_plans.go is being executed before honoring force index-join
hints, causing INL_JOIN/INL_HASH_JOIN to be incorrectly discarded; fix by
skipping that pruning branch when a forced index-join hint is present for this
join node (i.e., detect INL_JOIN or INL_HASH_JOIN force hint on the current
join) and only call shouldPruneIndexJoinByScanRatio(IndexJoinMaxProbeScanRatio,
buildRows, avgInnerRowCnt, p.Children()[1-outerIdx]) when no such force hint
exists; locate the check around the existing shouldPruneIndexJoinByScanRatio
call and gate it on the presence/absence of the force-index-join hint for this
plan node.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1917532c-3ea8-433a-864b-768fd1248e8a
📒 Files selected for processing (6)
pkg/planner/core/casetest/cbotest/cbo_test.gopkg/planner/core/exhaust_physical_plans.gopkg/planner/core/plan_cost_ver2_test.gopkg/sessionctx/vardef/tidb_vars.gopkg/sessionctx/variable/session.gopkg/sessionctx/variable/sysvar.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/sessionctx/vardef/tidb_vars.go
- pkg/planner/core/casetest/cbotest/cbo_test.go
- pkg/planner/core/plan_cost_ver2_test.go
| @@ -1480,6 +1461,7 @@ const ( | |||
| DefOptMergeJoinCostFactor = 1.0 | |||
| DefOptHashJoinCostFactor = 1.0 | |||
| DefOptIndexJoinCostFactor = 1.0 | |||
| DefOptIndexJoinMaxProbeScanRatio = 0.0 | |||
There was a problem hiding this comment.
Could we set the default value to 1.0 or 1.5? If the IndexJoin is going to scan 100% or 150% rows than HashJoin's FullScan, it seems like there is no reason to choose IndexJoin.
Signed-off-by: guo-shaoge <shaoge1994@163.com>
Signed-off-by: guo-shaoge <shaoge1994@163.com>
Signed-off-by: guo-shaoge <shaoge1994@163.com>
ti-chi-bot
Bot
added
size/XXL
…exjoin_hashjoin Signed-off-by: guo-shaoge <shaoge1994@163.com>
|
/test pull-unit-test-next-gen |
|
@AilinKid: 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 pull-unit-test-next-gen |
|
@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-required |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AilinKid, qw4990, terry1purcell 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 |
|
/retest |
Signed-off-by: guo-shaoge <shaoge1994@163.com>
|
/retest |
ti-chi-bot
Bot
added
the
needs-cherry-pick-release-8.5
|
In response to a cherrypick label: new pull request created to branch |
ti-chi-bot
Bot
removed
the
needs-cherry-pick-release-8.5
…ach a full scan (pingcap#67646) close pingcap#67610
5 participants
What problem does this PR solve?
Issue Number: close #67610
Problem Summary: When
IndexJoinorIndexHashJoinis estimated to probe a large number of inner rows, the optimizer may still prefer it overHashJoin, even though the index join has already degenerated into a near full-scan pattern and can be slower in practice.What changed and how does it work?
This PR introduces a new optimizer sysvar:
tidb_opt_index_join_scan_ratio_thresholdFor
IndexJoin/IndexHashJoin, the optimizer now evaluates the following ratio:Where:
If the ratio is greater than or equal to tidb_opt_index_join_scan_ratio_threshold, the optimizer adds a large penalty to the index join plan, making it less likely to win against HashJoin.
This helps avoid plans where the total probe work of an index join is already close to a full scan, which is a common shape for degenerate IndexHashJoin / IndexJoin choices.
The change is disabled by default with 0, so it is backward compatible unless explicitly enabled.
This PR also fixes a panic in the new cost path when full-scan stats cannot be derived from the probe plan tree.
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
Improvements
Bug Fixes
Tests