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: add IGNORE_INDEX hint #12059

Merged
merged 5 commits into from
Sep 12, 2019

Conversation

foreyes
Copy link
Contributor

@foreyes foreyes commented Sep 6, 2019

What problem does this PR solve?

add IGNORE_INDEX hint and change INDEX into USE_INDEX hint.

What is changed and how it works?

Just handle hintIgnoreIndex as what hintIndex do.

Check List

Tests

  • Unit test

Code changes

  • None

Side effects

  • None

Related changes

  • Need parser change

Release note

  • None

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

@alivxxx alivxxx added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 6, 2019
@@ -1972,7 +1974,7 @@ func (b *PlanBuilder) pushTableHints(hints []*ast.TableOptimizerHint, nodeType n
preferAggType |= preferHashAgg
case HintStreamAgg:
preferAggType |= preferStreamAgg
case HintIndex:
case HintUseIndex:
Copy link
Contributor

Choose a reason for hiding this comment

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

What will happen if using both ignore_index(t, idx_a) and use_index(t, idx_a) in a sql block?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to add a test for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems that we will ignore all indexes... I'll add test and fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

now it works just like normal index hint. Added some tests, you can check it out.

@foreyes foreyes requested review from lzmhhh123 and removed request for lzmhhh123 September 9, 2019 13:53
@codecov
Copy link

codecov bot commented Sep 11, 2019

Codecov Report

Merging #12059 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #12059   +/-   ##
===========================================
  Coverage   81.6305%   81.6305%           
===========================================
  Files           453        453           
  Lines         98370      98370           
===========================================
  Hits          80300      80300           
  Misses        12413      12413           
  Partials       5657       5657

@foreyes
Copy link
Contributor Author

foreyes commented Sep 11, 2019

Add some tests and resolved conflict, PTAL. @lzmhhh123 @lamxTyler

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 previously approved these changes Sep 12, 2019
Copy link
Contributor

@lzmhhh123 lzmhhh123 left a comment

Choose a reason for hiding this comment

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

LGTM.

@foreyes
Copy link
Contributor Author

foreyes commented Sep 12, 2019

/run-all-tests

@foreyes
Copy link
Contributor Author

foreyes commented Sep 12, 2019

/run-all-tests

@lzmhhh123 lzmhhh123 added status/all tests passed status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 12, 2019
@lzmhhh123 lzmhhh123 merged commit 3fc05c4 into pingcap:master Sep 12, 2019
@foreyes foreyes deleted the dev/add_ignore_index_hint branch September 12, 2019 03:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/planner SIG: Planner status/LGT2 Indicates that a PR has LGTM 2. type/new-feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants