planner/core: discourage degenerate index joins when probe rows approach a full scan | tidb-test=pr/2747 - #68557
Merged
Conversation
…ach a full scan Signed-off-by: guo-shaoge <shaoge1994@163.com>
ti-chi-bot
Bot
added
release-note-none
|
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 sysvar to control max index-join scan-rows ratio, implements pruning thresholds/helpers, threads an enable flag through index-join enumeration to early-prune expensive index-join candidates when appropriate, and updates tests and expected-plan fixtures. ChangesIndex-Join Scan-Ratio Pruning
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (1)
pkg/planner/core/exhaust_physical_plans.go (1)
48-51: 💤 Low valueConsider documenting the rationale for these threshold values.
These magic numbers drive the pruning heuristic but lack comments explaining why 100K and 100 were chosen. A brief note about their origin (empirical tuning, benchmark results, or theoretical basis) would help future maintainers understand when these might need adjustment.
🤖 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 48 - 51, Add short explanatory comments above the constants indexJoinPruneMinProbeRows and indexJoinPruneMinBuildRows describing why these thresholds (100000.0 and 100.0) were chosen (e.g., empirical tuning, benchmark results, or theoretical reasoning), note the units/meaning (probe rows vs build rows), and mention when they should be revisited (tests/benchmarks or workload change); reference the two constant names in the comment so future maintainers can trace the heuristic origin and any supporting benchmark or issue ID.
🤖 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/plan_cost_ver2_test.go`:
- Around line 637-658: The current block only validates hint precedence because
the queries use the /*+ INL_JOIN(i) */ hint; add an unhinted join explain to
prove ratio-driven pruning: run tk.MustQuery("explain format=verbose select *
from t_outer o straight_join t_inner i on o.b = i.b") and capture rs, then
assert plan selection changes (e.g., require.NotContains(t, rs[0][0].(string),
"IndexJoin") when tk.MustExec("set
@@session.tidb_opt_index_join_max_scan_rows_ratio=0") and require.Contains(t,
rs[0][0].(string), "IndexJoin") after tk.MustExec("set
@@session.tidb_opt_index_join_max_scan_rows_ratio=0.8"); finally after resetting
with tk.MustExec("set
@@session.tidb_opt_index_join_max_scan_rows_ratio=default") add an assertion
that the unhinted plan returns to the default expected choice to cover
regression.
In `@pkg/sessionctx/variable/sysvar.go`:
- Around line 2125-2128: The TiDBOptIndexJoinMaxScanRowsRatio sysvar incorrectly
uses MaxValue: math.MaxUint64 for a ratio; update the sysvar definition (symbol
TiDBOptIndexJoinMaxScanRowsRatio) to use a sensible upper bound (e.g., MaxValue:
1.0) consistent with other ratio vars, or if this value is meant to be a
percentage choose a clearer max (e.g., 100) and consider renaming accordingly;
ensure the SessionVars field IndexJoinMaxScanRowsRatio and default
DefOptIndexJoinMaxScanRowsRatio remain compatible and that the setter
tidbOptFloat64(val, DefOptIndexJoinMaxScanRowsRatio) behavior is preserved.
---
Nitpick comments:
In `@pkg/planner/core/exhaust_physical_plans.go`:
- Around line 48-51: Add short explanatory comments above the constants
indexJoinPruneMinProbeRows and indexJoinPruneMinBuildRows describing why these
thresholds (100000.0 and 100.0) were chosen (e.g., empirical tuning, benchmark
results, or theoretical reasoning), note the units/meaning (probe rows vs build
rows), and mention when they should be revisited (tests/benchmarks or workload
change); reference the two constant names in the comment so future maintainers
can trace the heuristic origin and any supporting benchmark or issue ID.
🪄 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: d9b18008-2bea-4b27-a940-d608c492cfbc
📒 Files selected for processing (5)
pkg/planner/core/exhaust_physical_plans.gopkg/planner/core/plan_cost_ver2_test.gopkg/sessionctx/variable/session.gopkg/sessionctx/variable/sysvar.gopkg/sessionctx/variable/tidb_vars.go
Comment on lines
+637
to
+658
| // 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_max_scan_rows_ratio=0") | ||
| rs = tk.MustQuery("explain format=verbose select /*+ INL_JOIN(i) */ * from t_outer o straight_join t_inner i on o.b = i.b").Rows() | ||
| require.Contains(t, rs[0][0].(string), "IndexJoin") | ||
| tk.MustExec("set @@session.tidb_opt_index_join_max_scan_rows_ratio=0.8") | ||
| rs = tk.MustQuery("explain format=verbose select /*+ INL_JOIN(i) */ * from t_outer o straight_join t_inner i on o.b = i.b").Rows() | ||
| require.Contains(t, rs[0][0].(string), "IndexJoin") | ||
| tk.MustExec("set @@session.tidb_opt_index_join_max_scan_rows_ratio=default") | ||
| tk.MustExec("set @@session.tidb_opt_hash_join_cost_factor=default") | ||
| tk.MustExec("set @@session.tidb_opt_merge_join_cost_factor=default") | ||
|
|
||
| // Reset to default | ||
| tk.MustExec("set @@session.tidb_opt_index_merge_cost_factor=1") | ||
| tk.MustExec("set @@session.tidb_opt_index_merge_cost_factor=default") |
There was a problem hiding this comment.
Pruning regression is not actually asserted in this test block
Line 648/Line 651 force INL_JOIN(i), so this validates hint precedence, not scan-ratio pruning. Also, Line 653 resets to default without asserting behavior after reset. Please add an unhinted join case that proves ratio-driven pruning changes plan selection.
Suggested test extension
// 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_max_scan_rows_ratio=0")
rs = tk.MustQuery("explain format=verbose select /*+ INL_JOIN(i) */ * from t_outer o straight_join t_inner i on o.b = i.b").Rows()
require.Contains(t, rs[0][0].(string), "IndexJoin")
tk.MustExec("set @@session.tidb_opt_index_join_max_scan_rows_ratio=0.8")
rs = tk.MustQuery("explain format=verbose select /*+ INL_JOIN(i) */ * from t_outer o straight_join t_inner i on o.b = i.b").Rows()
require.Contains(t, rs[0][0].(string), "IndexJoin")
+
+ // Unhinted case should reflect ratio pruning.
+ unhinted := "explain format=verbose select * from t_outer o straight_join t_inner i on o.b = i.b"
+ tk.MustExec("set @@session.tidb_opt_index_join_max_scan_rows_ratio=0")
+ rs = tk.MustQuery(unhinted).Rows()
+ require.Contains(t, fmt.Sprint(rs), "IndexJoin")
+ tk.MustExec("set @@session.tidb_opt_index_join_max_scan_rows_ratio=default")
+ rs = tk.MustQuery(unhinted).Rows()
+ require.NotContains(t, fmt.Sprint(rs), "IndexJoin")
+
tk.MustExec("set @@session.tidb_opt_index_join_max_scan_rows_ratio=default")
tk.MustExec("set @@session.tidb_opt_hash_join_cost_factor=default")
tk.MustExec("set @@session.tidb_opt_merge_join_cost_factor=default")As per coding guidelines MUST add a regression test and verify it fails before fix and passes after fix for bug fix changes.
🤖 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/plan_cost_ver2_test.go` around lines 637 - 658, The current
block only validates hint precedence because the queries use the /*+ INL_JOIN(i)
*/ hint; add an unhinted join explain to prove ratio-driven pruning: run
tk.MustQuery("explain format=verbose select * from t_outer o straight_join
t_inner i on o.b = i.b") and capture rs, then assert plan selection changes
(e.g., require.NotContains(t, rs[0][0].(string), "IndexJoin") when
tk.MustExec("set @@session.tidb_opt_index_join_max_scan_rows_ratio=0") and
require.Contains(t, rs[0][0].(string), "IndexJoin") after tk.MustExec("set
@@session.tidb_opt_index_join_max_scan_rows_ratio=0.8"); finally after resetting
with tk.MustExec("set
@@session.tidb_opt_index_join_max_scan_rows_ratio=default") add an assertion
that the unhinted plan returns to the default expected choice to cover
regression.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-8.5-20251125-v8.5.4 #68557 +/- ##
================================================================
Coverage ? 57.2296%
================================================================
Files ? 1784
Lines ? 637096
Branches ? 0
================================================================
Hits ? 364608
Misses ? 247938
Partials ? 24550
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Signed-off-by: guo-shaoge <shaoge1994@163.com>
ti-chi-bot
Bot
added
size/XXL
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/casetest/cbotest/cbo_test.go`:
- Around line 709-763: This change adds a new Go test function
TestAnalyzeSuiteRegression but did not include the Bazel metadata updates
required for new tests; run make bazel_prepare from the repo root to regenerate
BUILD.bazel / **/*.bazel / **/*.bzl and commit any produced metadata files (or
confirm and record that the command is a no-op for this package), ensuring the
Bazel outputs that reference the new test (TestAnalyzeSuiteRegression in
pkg/planner/core/casetest/cbotest/cbo_test.go and any symbols such as
GetAnalyzeSuiteData) are included in the PR.
In `@pkg/planner/core/casetest/testdata/json_plan_suite_out.json`:
- Around line 6-23: The test output file json_plan_suite_out.json has had its
case expectations replaced with empty "SQL" strings and null "JSONPlan" values
which breaks TestJSONPlanInExplain; restore each test case's original expected
SQL and JSONPlan entries (or re-run the recorder to re-record the suite) so the
JSON contains concrete SQL and non-null JSONPlan objects for each case, ensuring
TestJSONPlanInExplain can compare actual explain output against the recorded
"SQL" and "JSONPlan" values.
🪄 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: b2c7b760-c574-407c-8820-958a4e7d959b
📒 Files selected for processing (9)
pkg/executor/testdata/prepare_suite_out.jsonpkg/planner/core/casetest/binaryplan/testdata/binary_plan_suite_out.jsonpkg/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/testdata/integration_suite_out.jsonpkg/planner/core/casetest/testdata/json_plan_suite_out.jsonpkg/planner/core/casetest/tpch/testdata/tpch_suite_out.jsontests/integrationtest/r/planner/core/plan_cost_ver2.result
Signed-off-by: guo-shaoge <shaoge1994@163.com>
Signed-off-by: guo-shaoge <shaoge1994@163.com>
ti-chi-bot
Bot
added
size/XL
guo-shaoge
changed the title
Signed-off-by: guo-shaoge <shaoge1994@163.com>
ti-chi-bot
Bot
added
size/XXL
Signed-off-by: guo-shaoge <shaoge1994@163.com>
…ithub.com:guo-shaoge/tidb into cp_v8.5.4-20260329_refine_cost_indexjoin_hashjoin
guo-shaoge
changed the title
Contributor
Author
|
/retest |
guo-shaoge
commented
May 22, 2026
| tk.MustExec("create table t1(a int, c int, index idx(a))") | ||
| tk.MustExec("set tidb_mem_quota_query=10") | ||
| err := tk.ExecToErr("select /*+hash_join(t1)*/ t.a, t1.a from t use index(idx), t1 use index(idx) where t.a = t1.a") | ||
| err := tk.QueryToErr("select /*+hash_join(t1)*/ t.a, t1.a from t use index(idx), t1 use index(idx) where t.a = t1.a") |
Contributor
Author
There was a problem hiding this comment.
ExecToErr only parse and compile SQL, result will not be fetched from resultSet.
The original code path will build IndexJoin first, and memory error occurs when building IndexJoin.
The new code path will build HashJoin first, and will skip build IndexJoin and return directly if there is a HashJoin hint. That's why we skipped build IndexJoin and memory error will not occurs when compile this SQL.
And we have to trigger the error using QueryToErr, which will fetch result from result set.
terry1purcell
previously approved these changes
May 22, 2026
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
Signed-off-by: guo-shaoge <shaoge1994@163.com>
…ithub.com:guo-shaoge/tidb into cp_v8.5.4-20260329_refine_cost_indexjoin_hashjoin
qw4990
previously approved these changes
May 22, 2026
AilinKid
previously approved these changes
May 22, 2026
Contributor
Author
|
/retest |
0xPoe
approved these changes
May 22, 2026
terry1purcell
approved these changes
May 22, 2026
yudongusa
approved these changes
May 22, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 0xPoe, terry1purcell, yudongusa 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 |
ti-chi-bot
Bot
merged commit May 22, 2026
0253ba5
into
pingcap:release-8.5-20251125-v8.5.4
19 checks passed
13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: close #67610
Problem Summary: maunally cherry pick #67646
What changed and how does it work?
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
Behavior
Tests
Chores