parser: add several optimizer hints by foreyes · Pull Request #424 · pingcap/parser

foreyes · 2019-08-01T06:58:09Z

What problem does this PR solve?

Add optimizer hints.

What is changed and how it works?

Change TableOptimizerHint struct, change parser.y, add tests.

Check List

Tests

Unit test

Code changes

Has exported struct change

Related changes

Related TiDB PR: planner: rename optimizer hints tidb#11673

codecov · 2019-08-02T06:51:13Z

Codecov Report

Merging #424 into master will decrease coverage by 0.18%.
The diff coverage is 4.76%.

@@            Coverage Diff             @@
##           master     #424      +/-   ##
==========================================
- Coverage   70.66%   70.48%   -0.19%     
==========================================
  Files          32       32              
  Lines        7436     7456      +20     
==========================================
  Hits         5255     5255              
- Misses       1666     1686      +20     
  Partials      515      515

Impacted Files	Coverage Δ
parser.go	`70.58% <ø> (ø)`	⬆️
misc.go	`96.42% <ø> (ø)`	⬆️
ast/misc.go	`70.85% <4.76%> (-1.55%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e92cb39...59e3040. Read the comment docs.

codecov · 2019-08-02T06:51:13Z

Codecov Report

Merging #424 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #424      +/-   ##
==========================================
- Coverage   71.12%   71.07%   -0.05%     
==========================================
  Files          32       32              
  Lines        7542     7540       -2     
==========================================
- Hits         5364     5359       -5     
- Misses       1662     1665       +3     
  Partials      516      516

Impacted Files	Coverage Δ
misc.go	`96.42% <ø> (ø)`	⬆️
parser.go	`70.58% <ø> (ø)`	⬆️
ast/misc.go	`74.05% <100%> (+0.49%)`	⬆️
ast/ddl.go	`78.5% <0%> (-0.52%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f07200...0a13108. Read the comment docs.

foreyes · 2019-08-08T08:15:46Z

Now only TIDB_HJ, TIDB_INLJ, TIDB_SMJ, HASH_JOIN, INL_JOIN, SM_JOIN, HASH_AGG, STREAM_AGG are in use. We can ignore other hints at this moment.

And integration test fails because it rely on pingcap/tidb#11673.
We should merge this first, and then merge TiDB PR.

foreyes · 2019-08-08T08:23:28Z

+			$$ = getUint64FromNUM($1)
+		case "g":
+			$$ = getUint64FromNUM($1) * 1024
+		default:


Do we have better way to do this?

Why not add a syntax for memory unit? Or you cannot raise the right warnings since the quota maybe 0M.

Since syntax start with number and contains a-zA-Z is difficult to handle, we may not be able to use syntax like 233M. And in my opinion, M and G should not become a keyword, so...

0M is a good case

now we can use MEMORY_QUOTA(233 M)

foreyes · 2019-08-08T08:24:56Z

PTAL. @lzmhhh123 @lamxTyler

alivxxx · 2019-08-08T11:48:07Z

 	"IS":                       is,
 	"ISSUER":                   issuer,
 	"ISOLATION":                isolation,
+	"USE_TOJA":                 hintUseToja,


What is toja?

Convert in subquery into inner join and aggregation. Seems like this one.
The name is advised by product team.

How about adding a comment for this abbreviation.

Okay, and I will contact PM team also.

alivxxx · 2019-08-08T11:52:19Z

+	{
+		$$ = &ast.TableOptimizerHint{HintName: model.NewCIStr($1)}
+	}
+|	hintNoIndexMerge


Why it cannot specify table names?

It's just a switch to disable this optimizer rule.
Also advised by product team...

lzmhhh123 · 2019-08-12T03:16:12Z

 	"IS":                       is,
 	"ISSUER":                   issuer,
 	"ISOLATION":                isolation,
+	"USE_TOJA":                 hintUseToja,


How about adding a comment for this abbreviation.

lzmhhh123 · 2019-08-12T03:19:11Z

+	{
+		$$ = &ast.TableOptimizerHint{HintName: model.NewCIStr($1)}
+	}
+|	hintReadConsistentReplica


Is this one enforce table read from follower or learner?

Does it also need a table list?

This is depend on TiKV. It will not be used in 4.0GA, so it's just a draft.

It's about read from follower, but it's not clear to enforce, advise or enable.

lzmhhh123 · 2019-08-12T03:21:01Z

+		$$ = false
+	}
+
+HintQueryType:


how about change:

Suggested change

HintQueryType:

HintSQLType:

You mean change hint name from "query_type" into "sql_type" ?

foreyes · 2019-08-13T07:30:56Z

PTAL. @lzmhhh123 @lamxTyler .
We can ignore problems about hint name or hint arguments now.
Those are decided by PM team, and many won't be used in 4.0GA. Some of them are just draft.
Integration test fails because it rely on TiDB PR pingcap/tidb#11673

alivxxx

LGTM

lzmhhh123 · 2019-08-14T07:04:59Z

@foreyes pls resolve the conflicts, then LGTM.

foreyes added the WIP label Aug 1, 2019

foreyes force-pushed the dev/rewrite_hints branch 2 times, most recently from 208793b to c85b15d Compare August 2, 2019 06:42

foreyes force-pushed the dev/rewrite_hints branch from 99ef044 to 71ccd47 Compare August 8, 2019 02:52

foreyes mentioned this pull request Aug 8, 2019

planner: rename optimizer hints pingcap/tidb#11673

Merged

foreyes force-pushed the dev/rewrite_hints branch 5 times, most recently from f4e7abd to c36e23c Compare August 8, 2019 08:03

foreyes force-pushed the dev/rewrite_hints branch from c36e23c to 554c383 Compare August 8, 2019 08:18

foreyes commented Aug 8, 2019

View reviewed changes

foreyes requested review from alivxxx and lzmhhh123 August 8, 2019 08:24

foreyes removed the WIP label Aug 8, 2019

foreyes changed the title ~~[WIP] parser: add several optimizer hints~~ parser: add several optimizer hints Aug 8, 2019

alivxxx reviewed Aug 8, 2019

View reviewed changes

zz-jason reviewed Aug 8, 2019

View reviewed changes

Comment thread parser.y

lzmhhh123 reviewed Aug 12, 2019

View reviewed changes

foreyes force-pushed the dev/rewrite_hints branch 7 times, most recently from 05bad36 to 9df303f Compare August 13, 2019 07:04

foreyes requested review from alivxxx and lzmhhh123 August 13, 2019 07:31

alivxxx reviewed Aug 14, 2019

View reviewed changes

foreyes added the status/LGT1 LGT1 label Aug 14, 2019

foreyes added 6 commits August 14, 2019 15:13

rewrite optimizer hints

a5e792f

add tests

48fc6c7

fix

0fa388e

add test

5c008e5

add index hint

aec6fe1

update parser.go

0a13108

foreyes force-pushed the dev/rewrite_hints branch from 9df303f to 0a13108 Compare August 14, 2019 07:15

lzmhhh123 approved these changes Aug 14, 2019

View reviewed changes

foreyes merged commit 43d2d68 into pingcap:master Aug 14, 2019

foreyes deleted the dev/rewrite_hints branch August 14, 2019 11:32

foreyes added a commit to foreyes/parser that referenced this pull request Sep 16, 2019

parser: add several optimizer hints (pingcap#424)

3cdc73e

foreyes mentioned this pull request Sep 16, 2019

cherry-pick sql hint related PRs into release-3.1 #559

Merged

tiancaiamao pushed a commit to tiancaiamao/parser that referenced this pull request Apr 27, 2021

parser: add several optimizer hints (pingcap#424)

3d63dd8

lyonzhi pushed a commit to lyonzhi/parser that referenced this pull request Apr 25, 2024

parser: add several optimizer hints (pingcap#424)

b799e84

Conversation

foreyes commented Aug 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Check List

Uh oh!

codecov Bot commented Aug 2, 2019

Codecov Report

Uh oh!

codecov Bot commented Aug 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

foreyes commented Aug 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

foreyes commented Aug 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

foreyes Aug 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

foreyes Aug 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

foreyes commented Aug 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alivxxx left a comment

Choose a reason for hiding this comment

Uh oh!

lzmhhh123 commented Aug 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

foreyes commented Aug 1, 2019 •

edited

Loading

codecov Bot commented Aug 2, 2019 •

edited

Loading

foreyes Aug 12, 2019 •

edited

Loading

foreyes Aug 12, 2019 •

edited

Loading

foreyes commented Aug 13, 2019 •

edited

Loading