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

Add hint for broadcast join. #818

Merged
merged 20 commits into from
Jun 17, 2020
Merged

Conversation

hanfei1991
Copy link
Member

What problem does this PR solve?

Add hint for broadcast join.

What is changed and how it works?

Let parser support two new hints:

  • BC_JOIN(...)
  • TIDB_BCJ(...)

BC meas broadcast, which is brief and easy to understand :)

Check List

Tests

  • Unit test

Code changes

only change parser

Side effects

on side effects

Related changes

  • Need to cherry-pick to the release branch
  • Need to update the documentation
  • Need to be included in the release note

@hanfei1991 hanfei1991 requested a review from a team April 20, 2020 14:19
@CLAassistant
Copy link

CLAassistant commented Apr 20, 2020

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Apr 20, 2020

Codecov Report

Merging #818 into master will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #818   +/-   ##
=======================================
  Coverage   78.33%   78.33%           
=======================================
  Files          40       40           
  Lines       14765    14765           
=======================================
  Hits        11566    11566           
  Misses       2511     2511           
  Partials      688      688           

Copy link
Contributor

@kennytm kennytm left a comment

Choose a reason for hiding this comment

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

lgtm.

is there a corresponding tidb pr?

@kennytm kennytm added the status/LGT1 LGT1 label Apr 21, 2020
ast/misc.go Outdated
@@ -2539,7 +2539,7 @@ func (n *TableOptimizerHint) Restore(ctx *format.RestoreCtx) error {
switch n.HintName.L {
case "max_execution_time":
ctx.WritePlainf("%d", n.HintData.(uint64))
case "tidb_hj", "tidb_smj", "tidb_inlj", "hash_join", "merge_join", "inl_join", "bc_join", "tidb_bcj":
case "tidb_hj", "tidb_smj", "tidb_inlj", "hash_join", "merge_join", "inl_join", "bc_join", "bcj_local", "tidb_bcj":
Copy link
Member

Choose a reason for hiding this comment

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

Nit: according to https://www.abbreviations.com/abbreviation/BROADCAST, seems BRDCST is a proper abbreviation for broadcast

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems brdcst is for media, I think it's quite verbose. Can we make it brief and easy to write ?

Copy link
Contributor

Choose a reason for hiding this comment

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

if you shorten it to "brdcst" i'd rather you just spell it out entirely

select /*+ broadcast_join(...) */ ...

Copy link
Member

Choose a reason for hiding this comment

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

@hanfei1991 @lzmhhh123 PTAL the comment from @kennytm

Copy link
Contributor

Choose a reason for hiding this comment

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

I support the bc_join as the abbreviation.

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

lzmhhh123
lzmhhh123 previously approved these changes Jun 17, 2020
@lzmhhh123 lzmhhh123 added status/LGT2 LGT2 and removed status/LGT1 LGT1 labels Jun 17, 2020
@lzmhhh123
Copy link
Contributor

/merge

@ti-srebot
Copy link
Contributor

Sorry @lzmhhh123, you don't have permission to trigger auto merge event on this branch. You are not a committer for the related sigs:ddl(slack).

ast/misc.go Outdated
@@ -2539,7 +2539,7 @@ func (n *TableOptimizerHint) Restore(ctx *format.RestoreCtx) error {
switch n.HintName.L {
case "max_execution_time":
ctx.WritePlainf("%d", n.HintData.(uint64))
case "tidb_hj", "tidb_smj", "tidb_inlj", "hash_join", "merge_join", "inl_join", "bc_join", "tidb_bcj":
case "tidb_hj", "tidb_smj", "tidb_inlj", "hash_join", "merge_join", "inl_join", "bc_join", "bcj_local", "tidb_bcj":
Copy link
Member

Choose a reason for hiding this comment

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

@hanfei1991 @lzmhhh123 PTAL the comment from @kennytm

@hanfei1991
Copy link
Member Author

@zz-jason PTAL

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.

LGTM

@ti-srebot
Copy link
Contributor

/run-all-tests

@kennytm
Copy link
Contributor

kennytm commented Jun 17, 2020

just wanna note that both bc_join and broadcast_join are fine for me, but brdcst_join is bad.

@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 failed

hanfei1991 added a commit to hanfei1991/parser that referenced this pull request Jul 29, 2020
* add hint for bc join

* refine

* add hint for bc join

* refine

* refine

* refine

* refine again

* pass test

* fix compile error

* add prefer local

* use bcj_local

* change bc_join to broadcast_join

Co-authored-by: xufei <xufeixw@mail.ustc.edu.cn>
Co-authored-by: ichn-hu <zfhu16@fudan.edu.cn>
hanfei1991 added a commit that referenced this pull request Jul 29, 2020
* add hint for bc join

* refine

* add hint for bc join

* refine

* refine

* refine

* refine again

* pass test

* fix compile error

* add prefer local

* use bcj_local

* change bc_join to broadcast_join

Co-authored-by: xufei <xufeixw@mail.ustc.edu.cn>
Co-authored-by: ichn-hu <zfhu16@fudan.edu.cn>

Co-authored-by: xufei <xufeixw@mail.ustc.edu.cn>
Co-authored-by: ichn-hu <zfhu16@fudan.edu.cn>
tiancaiamao pushed a commit to tiancaiamao/parser that referenced this pull request Apr 27, 2021
* add hint for bc join

* refine

* add hint for bc join

* refine

* refine

* refine

* refine again

* pass test

* fix compile error

* add prefer local

* use bcj_local

* change bc_join to broadcast_join

Co-authored-by: xufei <xufeixw@mail.ustc.edu.cn>
Co-authored-by: ichn-hu <zfhu16@fudan.edu.cn>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants