-
Notifications
You must be signed in to change notification settings - Fork 5.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
planner: consider seek cost of probe-side for index join #33867
Conversation
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
"Best": "MergeInnerJoin{TableReader(Table(t))->IndexJoin{TableReader(Table(t))->IndexReader(Index(t.c_d_e)[[NULL,+inf]])}(test.t.a,test.t.c)}(test.t.a,test.t.a)" | ||
"Best": "MergeInnerJoin{TableReader(Table(t))->IndexJoin{TableReader(Table(t))->IndexReader(Index(t.c_d_e)[[NULL,NULL]])}(test.t.a,test.t.c)}(test.t.a,test.t.a)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes like this [NULL, +inf] -> [NULL, NULL]
can be ignored since they are only for display, and the actual ranges used to access data will be calculated when execution.
Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/ee75dc81593d549847de8347ae91137c14306edc |
@@ -802,13 +802,14 @@ func (p *LogicalJoin) buildIndexJoinInner2TableScan( | |||
if !pkMatched { | |||
return nil | |||
} | |||
innerTask = p.constructInnerTableScanTask(ds, pkCol, outerJoinKeys, us, false, false, avgInnerRowCnt) | |||
ranges := ranger.FullIntRange(mysql.HasUnsignedFlag(pkCol.RetType.Flag)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We change ranges
here. Besides calculating seek cost of probe-side, it also affects the calculation of other costs since changing of ranges
causes changing of rowCount
. I'm not sure that whether helper.chosenRanges.Range()
is suitable for estimating row count since it comes from (*indexJoinBuildHelper).buildTemplateRange
, which seems to fill empty datum in some ranges.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After talking with @qw4990, ranges
here only affects calculating seek cost of probe-side, it doesn't affect calculating row count or other stats. Hence it's OK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ranges
won't affect rowCount
here ~
And actually, helper.chosenRanges.Range()
is the only range we can use to calculate seek cost. In most cases, chosenRanges
is [NULL, NULL]
and len(chosenRanges)
is 1, which is equal to what we use now len(FullRange)
. For queries like select /*+ use_index(t1, ab) */ * from t1, t2 where t1.a in (1, 2, 3) and t1.b=t2.b
, it can be large than 1 and more accurate than len(FullRange)
.
/merge |
This pull request has been accepted and is ready to merge. Commit hash: 43c8242
|
/merge |
This pull request has been accepted and is ready to merge. Commit hash: ee75dc8
|
TiDB MergeCI notify✅ Well Done! New fixed [1] after this pr merged.
|
What problem does this PR solve?
Issue Number: close #32362
Problem Summary: planner: consider seek cost of probe-side for index join
What is changed and how it works?
Please see #32362 for more details.
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.