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.

Sign up for GitHub

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

Jump to bottom

planner: Fix SMJ hint, support SMJ with descending order. #14505

Merged
merged 3 commits into from
Jan 25, 2020
Merged

planner: Fix SMJ hint, support SMJ with descending order. #14505

merged 3 commits into from
Jan 25, 2020

Conversation

ichn-hu
Copy link
Contributor

@ichn-hu ichn-hu commented Jan 16, 2020 •
edited by SunRunAway
Loading

What problem does this PR solve?

This PR fixes #14483, fixes #14506

What is changed and how it works?

Do not create SMJ physical plan if PhysicalProperty has desc equals to true.

Check List

Tests

  • Unit test

Side effects

  • Possible performance regression

Release note

  • Fixing a correctness bug in execution, need to be cherry-picked to release version.

@ichn-hu ichn-hu requested a review from a team as a code owner January 16, 2020 09:07
@ghost ghost requested review from eurekaka and francis0407 and removed request for a team January 16, 2020 09:07
@ichn-hu ichn-hu added sig/planner SIG: Planner type/bugfix This PR fixes a bug. labels Jan 16, 2020
zz-jason
zz-jason reviewed Jan 16, 2020
View reviewed changes
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

@zz-jason zz-jason added needs-cherry-pick-3.0 status/LGT1 Indicates that a PR has LGTM 1. labels Jan 16, 2020
@ichn-hu ichn-hu mentioned this pull request Jan 16, 2020
Closed
@ichn-hu ichn-hu requested a review from SunRunAway January 16, 2020 09:41
lzmhhh123
lzmhhh123 reviewed Jan 16, 2020
View reviewed changes
planner/core/exhaust_physical_plans.go Outdated
@@ -199,7 +199,7 @@ func (p *LogicalJoin) getEnforcedMergeJoin(prop *property.PhysicalProperty) []Ph
// Check whether SMJ can satisfy the required property
offsets := make([]int, 0, len(p.LeftJoinKeys))
all, desc := prop.AllSameOrder()
if !all {
if !all || desc {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why ban the descent merge join plan? It seems to change it in the executor is not hard. Only keep similar with index merge join is ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps we want a quick fix?

Copy link
Member

Choose a reason for hiding this comment

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

We just release v3.0.9, and there is still time for fixing th executor before v3.0.10. Can we just fix the root cause?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough, I'll fix it soon.

@ichn-hu
Copy link
Contributor Author

ichn-hu commented Jan 17, 2020

/run-all-tests

@ichn-hu
Copy link
Contributor Author

ichn-hu commented Jan 17, 2020

@lzmhhh123 @francis0407 PTAL

@ichn-hu ichn-hu requested review from zz-jason and lzmhhh123 January 17, 2020 08:46
@ichn-hu ichn-hu changed the title planner: Fix SMJ hint, don't choose SMJ because it cann't handle desc. planner: Fix SMJ hint, support SMJ with descending order. Jan 17, 2020
lzmhhh123
lzmhhh123 reviewed Jan 17, 2020
View reviewed changes
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

@ichn-hu
Copy link
Contributor Author

ichn-hu commented Jan 19, 2020

/run-all-tests

SunRunAway
SunRunAway reviewed Jan 19, 2020
View reviewed changes
Copy link
Contributor

@SunRunAway SunRunAway left a comment

Choose a reason for hiding this comment

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

LGTM

SunRunAway
SunRunAway previously approved these changes Jan 19, 2020
View reviewed changes
@zz-jason zz-jason added status/LGT2 Indicates that a PR has LGTM 2. status/can-merge Indicates a PR has been approved by a committer. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jan 19, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Jan 19, 2020

Your auto merge job has been accepted, waiting for 14496

@sre-bot
Copy link
Contributor

sre-bot commented Jan 20, 2020

@ichn-hu merge failed.

@ichn-hu
Copy link
Contributor Author

ichn-hu commented Jan 20, 2020

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Jan 20, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Jan 20, 2020

@ichn-hu merge failed.

@bb7133 bb7133 added this to the v4.0.0-beta.1 milestone Jan 20, 2020
@zz-jason
Copy link
Member

/run-all-tests

@ichn-hu
Copy link
Contributor Author

ichn-hu commented Jan 24, 2020

It's wield, it fails UT every time with different test cases all seemingly not related to the modification I've made... This time it is https://internal.pingcap.net/idc-jenkins/blue/rest/organizations/jenkins/pipelines/tidb_ghpr_unit_test/runs/22727/nodes/67/log/?start=0

@ichn-hu
Copy link
Contributor Author

ichn-hu commented Jan 24, 2020

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Jan 24, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Jan 24, 2020

@ichn-hu merge failed.

@ichn-hu
Copy link
Contributor Author

ichn-hu commented Jan 25, 2020

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Jan 25, 2020

/run-all-tests

@sre-bot sre-bot merged commit c2da4ea into pingcap:master Jan 25, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Jan 25, 2020

cherry pick to release-3.0 failed

@sre-bot
Copy link
Contributor

sre-bot commented Jan 25, 2020

cherry pick to release-2.1 failed

ichn-hu added a commit to ichn-hu/tidb that referenced this pull request Feb 6, 2020
@ichn-hu
planner: Fix SMJ hint, support SMJ with descending order. (pingcap#14505
89094ce 
)

cherry-pick "planner: Fix SMJ hint, support SMJ with descending order. pingcap#14505" to 2.1
@ichn-hu ichn-hu mentioned this pull request Feb 6, 2020
Merged
ichn-hu added a commit to ichn-hu/tidb that referenced this pull request Feb 6, 2020
@ichn-hu
planner: Fix SMJ hint, support SMJ with descending order. (pingcap#14505
56a9844 
)

cherry-pick
@ichn-hu ichn-hu mentioned this pull request Feb 6, 2020
Merged
@codecov
Copy link

codecov bot commented Feb 24, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@e03e1cb). Click here to learn what that means.
The diff coverage is 81.5789%.

@@             Coverage Diff             @@
##             master     #14505   +/-   ##
===========================================
  Coverage          ?   80.2454%           
===========================================
  Files             ?        495           
  Lines             ?     127115           
  Branches          ?          0           
===========================================
  Hits              ?     102004           
  Misses            ?      16968           
  Partials          ?       8143

@sre-bot
Copy link
Contributor

sre-bot commented Apr 7, 2020

It seems that, not for sure, we failed to cherry-pick this commit to release-2.1 release-3.0. Please comment '/run-cherry-picker' to try to trigger the cherry-picker if we did fail to cherry-pick this commit before. @ichn-hu PTAL.

@ichn-hu ichn-hu deleted the fix-smj-desc branch May 19, 2020 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lzmhhh123 lzmhhh123 lzmhhh123 left review comments

@zz-jason zz-jason zz-jason approved these changes

@SunRunAway SunRunAway SunRunAway left review comments

@eurekaka eurekaka Awaiting requested review from eurekaka

@francis0407 francis0407 Awaiting requested review from francis0407

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. type/bugfix This PR fixes a bug.
Projects
None yet
Milestone
v4.0.0-beta.1
8 participants
@ichn-hu @sre-bot @zz-jason @SunRunAway @francis0407 @lzmhhh123 @bb7133 @ngaut