-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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 mpp join column prune added projection incorrect construction #52943
Conversation
Signed-off-by: yibin <huyibin@pingcap.com>
Hi @yibin87. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/cc @windtalker @SeaRise |
/cc @winoros |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #52943 +/- ##
=================================================
- Coverage 72.4412% 56.0366% -16.4047%
=================================================
Files 1482 1593 +111
Lines 428430 601166 +172736
=================================================
+ Hits 310360 336873 +26513
- Misses 98853 241315 +142462
- Partials 19217 22978 +3761
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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
[LGTM Timeline notifier]Timeline:
|
/cc @AilinKid |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AilinKid, SeaRise, windtalker The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/test unit-test |
@yibin87: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/test unit-test |
@yibin87: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/test unit-test |
@yibin87: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
What problem does this PR solve?
Issue Number: ref #52828
Problem Summary:
In #52836, we added missing "hashCols" to the added projection's schema, since they may be needed by potential type cast projection. However, the way we added is somewhat problemtic, we directly added from the hashCols instead of HashJoin's output schema:
tidb/pkg/planner/core/task.go
Lines 447 to 453 in c73d6c5
While from the existing getProj implementation, we need to add from child executor's schema, and cloned it also(not sure if it is needed), since inside the schema.Clone() function column is cloned actually:
tidb/pkg/planner/core/task.go
Lines 272 to 282 in c73d6c5
After fixing this, we can pass all tiflash's randgen mpp tests in both normal and non-broadcast join mode.
What changed and how does it work?
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.