Skip to content
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: generate index path as candidate when it is a single scan #12022

Merged
merged 19 commits into from Sep 9, 2019

Conversation

francis0407
Copy link
Member

@francis0407 francis0407 commented Sep 4, 2019

What problem does this PR solve?

fix #12021

When we only want to get primary key, we can consider index path if there is no condition on PK. For example:

create table t1 (a int primary key, b int, .... many columns, index idx_b(b));
create table t2 (a int, b int);

In this query, we can use IndexScan for t1.a:

select t2.a, t2.b
from t1, t2
where t1.a = t2.a;

We can also use a IndexScan for this one:

select count(1) from t1;

More general, we should consider the index path as a candidate when it is a single scan.

What is changed and how it works?

Change the condition that generates index path.

We can use index to generate physical plan if any of the following conditions is satisfied:

  1. This path's access cond is not nil.
  2. We have a non-empty prop to match.
  3. This index is forced to choose.
  4. The needed columns are all covered by index columns(and handleCol).

Check List

Tests

  • Unit test
  • Explain test

@francis0407 francis0407 changed the title planner: don't prune index path when the needed columns only contain handleCol planner: generate index path as candidate when it is a single scan Sep 4, 2019
@francis0407
Copy link
Member Author

Some unit tests about admin clean up index and admin check index failed. I'm gonna check these tests, maybe fix the issue in another PR.

@francis0407
Copy link
Member Author

/rebuild

@francis0407
Copy link
Member Author

/rebuild

@codecov
Copy link

codecov bot commented Sep 6, 2019

Codecov Report

Merging #12022 into master will increase coverage by 0.1112%.
The diff coverage is n/a.

@@               Coverage Diff               @@
##            master     #12022        +/-   ##
===============================================
+ Coverage   81.456%   81.5673%   +0.1112%     
===============================================
  Files          448        448                
  Lines        95945      96280       +335     
===============================================
+ Hits         78153      78533       +380     
+ Misses       12218      12178        -40     
+ Partials      5574       5569         -5

@francis0407
Copy link
Member Author

All the tests have been fixed.
Some of the tests failed before because sql select count(*) from t chooses index path, but acutally, they want a table path. If we want to use a table scan, we must use index hint now.

Copy link
Contributor

@alivxxx alivxxx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lzmhhh123 lzmhhh123 added status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 9, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Sep 9, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Sep 9, 2019

@francis0407 merge failed.

@francis0407
Copy link
Member Author

/run-common-test tidb-test=pr/884

3 similar comments
@francis0407
Copy link
Member Author

/run-common-test tidb-test=pr/884

@francis0407
Copy link
Member Author

/run-common-test tidb-test=pr/884

@francis0407
Copy link
Member Author

/run-common-test tidb-test=pr/884

@francis0407
Copy link
Member Author

/run-integration-common-test tidb-test=pr/884

@winoros winoros removed their request for review September 9, 2019 08:57
Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@francis0407 please resolve conflicts

@francis0407
Copy link
Member Author

/run-common-test

@francis0407
Copy link
Member Author

/run-integration-common-test

@lzmhhh123
Copy link
Member

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Sep 9, 2019

/run-all-tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/planner SIG: Planner status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

planner: consider index path when the needed columns only contain handleCol
5 participants