-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
planner: fix can not found column bug #28067
Conversation
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
@@ -747,6 +747,54 @@ func (s *testIntegrationSerialSuite) TestMPPShuffledJoin(c *C) { | |||
res.Check(testkit.Rows(output[i].Plan...)) | |||
} | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add en empty line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
planner/core/task.go
Outdated
// can not use the task from tasks because it maybe updated. | ||
outerTaskIndex := 1 - p.InnerChildIdx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this happen in convertPartitionKeysIfNeed
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, convertPartitionKeysIfNeed
returns updated mpp task directly.
/label needs-cherry-pick-5.0 |
/label needs-cherry-pick-5.1 |
/label needs-cherry-pick-5.2 |
8f81b02
to
6991281
Compare
6991281
to
02a1ce8
Compare
f12da4a
to
59f8ea3
Compare
@@ -1886,8 +1886,22 @@ func (p *LogicalJoin) tryToGetMppHashJoin(prop *property.PhysicalProperty, useBC | |||
lPartitionKeys, rPartitionKeys := p.GetPotentialPartitionKeys() | |||
if prop.MPPPartitionTp == property.HashType { | |||
var matches []int | |||
if matches = prop.IsSubsetOf(lPartitionKeys); len(matches) == 0 { | |||
if p.JoinType == InnerJoin { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do previous tests not cover these cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/LGTM
/merge |
This pull request has been accepted and is ready to merge. Commit hash: 59f8ea3
|
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
cherry pick to release-5.0 in PR #28147 |
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
cherry pick to release-5.1 in PR #28148 |
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
cherry pick to release-5.2 in PR #28149 |
What problem does this PR solve?
Issue Number: close #30980
Problem Summary:
What is changed and how it works?
Proposal: xxx
What's Changed:
In
(p *PhysicalHashJoin) attach2TaskForMpp(tasks ...task)
, the task maybe changed, so we should use the updated task when construct the final result task.Check List
Tests
Side effects
Documentation
Release note