add a table hint to access table from a specific type of storage. by lzmhhh123 · Pull Request #446 · pingcap/parser

lzmhhh123 · 2019-08-07T07:11:48Z

What problem does this PR solve?

At now, tidb need a hint to make table reading from tiflash.

What is changed and how it works?

Add a hint to enforce table reading form tiflash.

Check List

Tests

Unit test
Integration test

Related changes

Need to update the documentation

codecov · 2019-08-07T07:23:43Z

Codecov Report

Merging #446 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #446      +/-   ##
==========================================
+ Coverage   71.61%   71.63%   +0.02%     
==========================================
  Files          32       32              
  Lines        7736     7735       -1     
==========================================
+ Hits         5540     5541       +1     
+ Misses       1671     1670       -1     
+ Partials      525      524       -1

Impacted Files	Coverage Δ
parser.go	`70.58% <ø> (ø)`	⬆️
ast/misc.go	`74.32% <100%> (+0.12%)`	⬆️
misc.go	`96.49% <100%> (+0.06%)`	⬆️

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 64bfe3e...7998b80. Read the comment docs.

foreyes

LGTM

foreyes · 2019-08-08T13:19:55Z

Product team advised hint QUERY_TYPE(OLAP/OLTP), is this what you want?

…cess_flash

winoros

lgtm

foreyes

rest part lgtm

foreyes · 2019-08-20T09:05:05Z

 	case "memory_quota":
 		ctx.WritePlainf("%d M", n.MemoryQuota)
+	case "read_consistent_storage":
+		ctx.WritePlain(n.StoreType.O)


Suggested change

ctx.WritePlain(n.StoreType.O)

ctx.WritePlain(n.StoreType.String())

If we use String(), it may causes TIFLASH ---> `TIFLASH`. Then the SQL hint will like READ_CONSISTENT_STORAGE(`TIFLASH`[`t1`, `t2`]), that's strange.

Only ctx.WriteName() will add those backquotes. BTW you should use ctx.WriteKeyWord() for case-insensitive keyword-like output.

OKay. Thanks for your advisement. @kennytm

??? I'm confused.

func (cis CIStr) String() string { return cis.O }

@foreyes My fault. I thought you suggested using the WriteName.

alivxxx · 2019-08-20T09:17:18Z

 	c.Assert(hints[1].Indexes[0].L, Equals, "c1")

+	// Test READ_CONSISTENT_STORAGE
+	stmt, _, err = parser.Parse("select /*+ READ_CONSISTENT_STORAGE(tiflash[t1, t2], tikv[t3]) */ c1, c2 from t1, t2, t1 t3 where t1.c1 = t2.c1 and t2.c1 = t3.c1", "", "")


Why choose this syntax? Why not READ_CONSISTENT_STORAGE(tiflash t1, t2), READ_CONSISTENT_STORAGE(tikv t3) to be consistent with others？

Because we can regard a tiflash[t1, t2] as a whole. If the "query block" also exists in this hint, the hint will like READ_CONSISTENT_STORAGE(@sel1 tiflash t1, t2). That's incomprehensible.

Also seems incomprehensible in this case: READ_CONSISTENT_STORAGE(tiflash[t1, t2], tikv[t3]) ----> READ_CONSISTENT_STORAGE(tiflash t1, t2 tikv t3)

I don't have strong opinion on this specific syntax, but if you decided to group similiar things, it is better to change all others that has similiar syntax, like index and use_index_merge.

I think it is quite clear what the comment want to say in READ_CONSISTENT_STORAGE(@sel1 tiflash t1, t2).

No, just seperate them into two like READ_CONSISTENT_STORAGE(tiflash t1, t2), READ_CONSISTENT_STORAGE(tikv t3).

Hmmm...So may I keep this format? The product manager says that we can only focus on functions. The form is under consideration now.

…cess_flash

foreyes

LGTM

…cess_flash

alivxxx

LGTM

…ngcap#446)

zz-jason reviewed Aug 8, 2019

View reviewed changes

Comment thread ast/misc.go Outdated

zz-jason requested review from alivxxx, eurekaka and foreyes August 8, 2019 11:39

foreyes reviewed Aug 8, 2019

View reviewed changes

eurekaka reviewed Aug 8, 2019

View reviewed changes

Comment thread misc.go Outdated

lzmhhh123 mentioned this pull request Aug 9, 2019

*: support a hint to access table from tiflash pingcap/tidb#11700

Closed

alivxxx removed their request for review August 13, 2019 08:58

lzmhhh123 force-pushed the dev/add_hint_for_access_flash branch 2 times, most recently from 413f67a to fc1c96c Compare August 20, 2019 04:26

code improve

c956035

lzmhhh123 force-pushed the dev/add_hint_for_access_flash branch from 60fdde9 to c956035 Compare August 20, 2019 05:33

Merge remote-tracking branch 'origin/master' into dev/add_hint_for_ac…

773ad72

…cess_flash

lzmhhh123 requested review from eurekaka, foreyes and winoros August 20, 2019 05:37

winoros reviewed Aug 20, 2019

View reviewed changes

lzmhhh123 changed the title ~~add a table hint to access table from tiflash.~~ add a table hint to access table from a specific type of storage. Aug 20, 2019

foreyes reviewed Aug 20, 2019

View reviewed changes

alivxxx reviewed Aug 20, 2019

View reviewed changes

lzmhhh123 added 2 commits August 20, 2019 19:14

address comments

b2b6bbf

Merge remote-tracking branch 'origin/master' into dev/add_hint_for_ac…

8696657

…cess_flash

foreyes reviewed Aug 20, 2019

View reviewed changes

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

lzmhhh123 added 3 commits August 21, 2019 10:43

Merge remote-tracking branch 'origin/master' into dev/add_hint_for_ac…

1969f24

…cess_flash

rename "read_consistent_storage" to "read_from_storage"

8497b29

Merge remote-tracking branch 'origin/master' into dev/add_hint_for_ac…

585dbb6

…cess_flash

lzmhhh123 requested a review from alivxxx August 23, 2019 05:56

Merge remote-tracking branch 'origin/master' into dev/add_hint_for_ac…

7998b80

…cess_flash

lzmhhh123 force-pushed the dev/add_hint_for_access_flash branch from d04052a to 7998b80 Compare August 23, 2019 06:54

alivxxx approved these changes Aug 23, 2019

View reviewed changes

lzmhhh123 merged commit c23214d into pingcap:master Aug 23, 2019

lzmhhh123 deleted the dev/add_hint_for_access_flash branch August 23, 2019 07:09

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

add a table hint to access table from a specific type of storage. (pi…

bf283a6

…ngcap#446)

foreyes mentioned this pull request Sep 16, 2019

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

Merged

tangenta mentioned this pull request Apr 28, 2020

Rewrite special comment parser (#711) #837

Closed

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

add a table hint to access table from a specific type of storage. (pi…

a0749f9

…ngcap#446)

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

add a table hint to access table from a specific type of storage. (pi…

ff2524d

…ngcap#446)

	ctx.WritePlain(n.StoreType.O)
	ctx.WritePlain(n.StoreType.String())

Conversation

lzmhhh123 commented Aug 7, 2019

What problem does this PR solve?

What is changed and how it works?

Check List

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Uh oh!

foreyes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

foreyes commented Aug 8, 2019

Uh oh!

winoros left a comment

Choose a reason for hiding this comment

Uh oh!

foreyes left a comment

Choose a reason for hiding this comment

Uh oh!

foreyes Aug 20, 2019

Choose a reason for hiding this comment

Uh oh!

lzmhhh123 Aug 20, 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!

kennytm Aug 20, 2019

Choose a reason for hiding this comment

Uh oh!

lzmhhh123 Aug 20, 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 Aug 20, 2019

Choose a reason for hiding this comment

Uh oh!

lzmhhh123 Aug 20, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alivxxx Aug 20, 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!

lzmhhh123 Aug 20, 2019

Choose a reason for hiding this comment

Uh oh!

lzmhhh123 Aug 20, 2019

Choose a reason for hiding this comment

Uh oh!

alivxxx Aug 20, 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!

alivxxx Aug 20, 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!

lzmhhh123 Aug 20, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

foreyes left a comment

Choose a reason for hiding this comment

Uh oh!

alivxxx left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov Bot commented Aug 7, 2019 •

edited

Loading

lzmhhh123 Aug 20, 2019 •

edited

Loading

lzmhhh123 Aug 20, 2019 •

edited

Loading

alivxxx Aug 20, 2019 •

edited

Loading

alivxxx Aug 20, 2019 •

edited

Loading

alivxxx Aug 20, 2019 •

edited

Loading