Skip to content

planner: rename optimizer hints#11673

Merged
foreyes merged 11 commits into
pingcap:masterfrom
foreyes:dev/rename_optimizer_hints
Aug 14, 2019
Merged

planner: rename optimizer hints#11673
foreyes merged 11 commits into
pingcap:masterfrom
foreyes:dev/rename_optimizer_hints

Conversation

@foreyes
Copy link
Copy Markdown
Contributor

@foreyes foreyes commented Aug 8, 2019 •
edited
Loading

What problem does this PR solve?

Support optimizer hint alias HASH_JOIN: TIDB_HJ, SM_JOIN: TIDB_SMJ, INL_JOIN: TIDB_INLJ.

Rename optimizer hints TIDB_HASHAGG() -> HASH_AGG, TIDB_STREAMAGG() -> STREAM_AGG, and remove brackets.

What is changed and how it works?

Add HintHJ, HintSMJ, HintINLJ have the same effect as TIDB hints.

We still keep TIDB style hints, for example, TIDB_HJ will have the same HintName as HASH_JOIN now.

Rename aggregation hints in parser and TiDB.

Check List

Tests

  • Unit test

Code changes

  • Has exported variables name changed.

Related changes

@foreyes foreyes force-pushed the dev/rename_optimizer_hints branch from b832063 to d668790 Compare August 8, 2019 05:46
@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 8, 2019 •
edited
Loading

Codecov Report

Merging #11673 into master will decrease coverage by 0.0837%.
The diff coverage is 93.75%.

@@               Coverage Diff                @@
##             master     #11673        +/-   ##
================================================
- Coverage   81.4841%   81.4003%   -0.0838%     
================================================
  Files           433        433                
  Lines         93525      93190       -335     
================================================
- Hits          76208      75857       -351     
- Misses        11881      11890         +9     
- Partials       5436       5443         +7

@foreyes foreyes removed the status/WIP label Aug 8, 2019
@SunRunAway SunRunAway mentioned this pull request Aug 8, 2019
Merged
1 task
@foreyes foreyes requested review from lzmhhh123 and removed request for lzmhhh123 August 8, 2019 06:20
@foreyes foreyes changed the title [WIP] planner: rename optimizer hints planner: rename optimizer hints Aug 8, 2019
@foreyes foreyes mentioned this pull request Aug 8, 2019
Merged
@foreyes
Copy link
Copy Markdown
Contributor Author

foreyes commented Aug 8, 2019

Idc fails because use go mod edit -replace, this will be fixed after parser PR is merged.

@foreyes
Copy link
Copy Markdown
Contributor Author

foreyes commented Aug 8, 2019

PTAL. @lzmhhh123 @lamxTyler @XuHuaiyu

alivxxx
alivxxx reviewed Aug 9, 2019
View reviewed changes
Copy link
Copy Markdown
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

@foreyes
Copy link
Copy Markdown
Contributor Author

foreyes commented Aug 12, 2019

/run-all-tests

lzmhhh123
lzmhhh123 reviewed Aug 12, 2019
View reviewed changes
Comment thread planner/core/logical_plan_builder.go
@foreyes foreyes force-pushed the dev/rename_optimizer_hints branch from e08391d to 720d9d3 Compare August 13, 2019 04:58
@foreyes
Copy link
Copy Markdown
Contributor Author

foreyes commented Aug 13, 2019

/run-all-tests

@foreyes foreyes force-pushed the dev/rename_optimizer_hints branch from 27d96ce to f69753e Compare August 13, 2019 06:51
@foreyes
Copy link
Copy Markdown
Contributor Author

foreyes commented Aug 13, 2019

/run-all-tests

@foreyes
Copy link
Copy Markdown
Contributor Author

foreyes commented Aug 13, 2019

The solution of TIDB_HJ -> HASH_JOIN is not looks well because of the error and warning message. But I think we can improve it by solving this Issue: #11632

@foreyes
Copy link
Copy Markdown
Contributor Author

foreyes commented Aug 13, 2019

Ready for review, PTAL. @lzmhhh123

lzmhhh123
lzmhhh123 reviewed Aug 13, 2019
View reviewed changes
Copy link
Copy Markdown
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.

Do we also need to check the ineffective hints like use_index_merge and append a warning for it?

@foreyes
Copy link
Copy Markdown
Contributor Author

foreyes commented Aug 13, 2019 •
edited
Loading

I think so, I will file an issue for it, like #11632 @lzmhhh123

Extract common warning message.

@foreyes
Copy link
Copy Markdown
Contributor Author

foreyes commented Aug 14, 2019

PTAL again. Just change some warning messages. @lamxTyler

lzmhhh123
lzmhhh123 reviewed Aug 14, 2019
View reviewed changes
Copy link
Copy Markdown
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 foreyes added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 14, 2019
@foreyes
Copy link
Copy Markdown
Contributor Author

foreyes commented Aug 14, 2019

/run-all-test

@foreyes
Copy link
Copy Markdown
Contributor Author

foreyes commented Aug 14, 2019

/run-all-tests

@foreyes
Copy link
Copy Markdown
Contributor Author

foreyes commented Aug 14, 2019

/run-ci-test

@lzmhhh123 lzmhhh123 added status/all tests passed 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 Aug 14, 2019
@sre-bot
Copy link
Copy Markdown
Contributor

sre-bot commented Aug 14, 2019

/run-all-tests

@sre-bot
Copy link
Copy Markdown
Contributor

sre-bot commented Aug 14, 2019

@foreyes merge failed.

@foreyes
Copy link
Copy Markdown
Contributor Author

foreyes commented Aug 14, 2019

/run-unit-test

1 similar comment
@foreyes
Copy link
Copy Markdown
Contributor Author

foreyes commented Aug 14, 2019

/run-unit-test

@foreyes foreyes merged commit 4491bf8 into pingcap:master Aug 14, 2019
@foreyes foreyes deleted the dev/rename_optimizer_hints branch August 14, 2019 11:31
foreyes added a commit to foreyes/tidb that referenced this pull request Sep 16, 2019
@foreyes
planner: rename optimizer hints (pingcap#11673)
489c094
@foreyes foreyes mentioned this pull request Sep 16, 2019
Merged
@zz-jason zz-jason mentioned this pull request Oct 10, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@XuHuaiyu XuHuaiyu Awaiting requested review from XuHuaiyu

2 more reviewers

@alivxxx alivxxx alivxxx left review comments

@lzmhhh123 lzmhhh123 lzmhhh123 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@foreyes @sre-bot @alivxxx @lzmhhh123