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

executor,planner/core,util/plancodec: extend executor.ShuffleExec and planner.core.PhysicalShuffle to support multiple data sources #20942

Merged
merged 17 commits into from Nov 24, 2020
Merged

executor,planner/core,util/plancodec: extend executor.ShuffleExec and planner.core.PhysicalShuffle to support multiple data sources #20942

merged 17 commits into from Nov 24, 2020

Conversation

huang-b
Copy link
Contributor

@huang-b huang-b commented Nov 9, 2020 •
edited by qw4990

What problem does this PR solve?

Issue Number: related to #14441

Problem Summary: The original implementation of shuffle is designed for Executors with single input, such as WindowExec. However, some Executor with multiple input also has the potential for parallel acceleration. The original implementation is not extendable for this situation.

What is changed and how it works?

What's Changed
image

  1. In executor.ShuffleExec, shuffleWorker is no more an Executor and its only responsibility is to invoke the actual Executor.
  2. As an alternative, the new type shuffleReceiver implements interface of Executor and it is responsible for receiving chunks from DataSources.

How it Works:

  • Each shuffleWorker can has one or more shuffleReceiver so that it can receives data from one or more DataSources.

Release note

  • executor,planner/core,util/plancodec: extend executor.ShuffleExec and planner.core.PhysicalShuffle to support multiple data sources

@huang-b huang-b requested review from a team as code owners November 9, 2020 13:14
@huang-b huang-b requested review from qw4990 and lzmhhh123 and removed request for a team November 9, 2020 13:14
@ti-srebot ti-srebot added the first-time-contributor Indicates that the PR was contributed by an external member and is a first-time contributor. label Nov 9, 2020
@CLAassistant
Copy link

CLAassistant commented Nov 9, 2020 •
edited

CLA assistant check
All committers have signed the CLA.

@sre-bot
Copy link
Contributor

sre-bot commented Nov 9, 2020

No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md

@github-actions github-actions bot added sig/execution SIG execution sig/planner SIG: Planner labels Nov 9, 2020
@qw4990 qw4990 requested review from SunRunAway and removed request for lzmhhh123 November 10, 2020 02:35
qw4990
qw4990 reviewed Nov 10, 2020
View reviewed changes
util/plancodec/id.go Outdated Show resolved Hide resolved
@qw4990
Copy link
Contributor

qw4990 commented Nov 10, 2020

@pingyu PTAL

pingyu
pingyu suggested changes Nov 14, 2020
View reviewed changes
Copy link
Contributor

@pingyu pingyu left a comment

Choose a reason for hiding this comment

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

Great job ! A small suggestion, rest LGTM.

executor/shuffle.go Show resolved Hide resolved
@qw4990
Copy link
Contributor

qw4990 commented Nov 20, 2020

Please fix this CI problem:

[2020-11-20T03:02:38.205Z] FAIL: physical_plan_test.go:1469: testPlanSuite.TestDAGPlanBuilderWindowParallel
[2020-11-20T03:02:38.205Z] 
[2020-11-20T03:02:38.205Z] physical_plan_test.go:1480:
[2020-11-20T03:02:38.205Z]     s.doTestDAGPlanBuilderWindow(c, vars, input, output)
[2020-11-20T03:02:38.205Z] physical_plan_test.go:1517:
[2020-11-20T03:02:38.205Z]     c.Assert(core.ToString(p), Equals, output[i].Best, comment)
[2020-11-20T03:02:38.205Z] ... obtained string = "TableReader(Table(t))->Sort->Window(lead(test.t.a, 1)->Column#14 over(partition by test.t.b))->Partition(execution info: concurrency:4, data sources:[TableReader_10])->Projection"
[2020-11-20T03:02:38.205Z] ... expected string = "TableReader(Table(t))->Sort->Window(lead(test.t.a, 1)->Column#14 over(partition by test.t.b))->Partition(execution info: concurrency:4, data source:TableReader_10)->Projection"
[2020-11-20T03:02:38.205Z] ... case:1 sql:select lead(a, 1) over (partition by b) as c from t

@qw4990
Copy link
Contributor

qw4990 commented Nov 20, 2020

Rest LGTM, PTAL @huang-b

@ti-challenge-bot
Copy link

There is no reward for this challenge pull request, so you can request a reward from @qw4990.

Details

Tip : About reward you can refs to reward-command.

Warning: None

@pingyu
Copy link
Contributor

pingyu commented Nov 21, 2020

/rebuild

@huang-b
Copy link
Contributor Author

huang-b commented Nov 23, 2020

Please fix this CI problem:

[2020-11-20T03:02:38.205Z] FAIL: physical_plan_test.go:1469: testPlanSuite.TestDAGPlanBuilderWindowParallel
[2020-11-20T03:02:38.205Z] 
[2020-11-20T03:02:38.205Z] physical_plan_test.go:1480:
[2020-11-20T03:02:38.205Z]     s.doTestDAGPlanBuilderWindow(c, vars, input, output)
[2020-11-20T03:02:38.205Z] physical_plan_test.go:1517:
[2020-11-20T03:02:38.205Z]     c.Assert(core.ToString(p), Equals, output[i].Best, comment)
[2020-11-20T03:02:38.205Z] ... obtained string = "TableReader(Table(t))->Sort->Window(lead(test.t.a, 1)->Column#14 over(partition by test.t.b))->Partition(execution info: concurrency:4, data sources:[TableReader_10])->Projection"
[2020-11-20T03:02:38.205Z] ... expected string = "TableReader(Table(t))->Sort->Window(lead(test.t.a, 1)->Column#14 over(partition by test.t.b))->Partition(execution info: concurrency:4, data source:TableReader_10)->Projection"
[2020-11-20T03:02:38.205Z] ... case:1 sql:select lead(a, 1) over (partition by b) as c from t

Already fixed.

@qw4990
Copy link
Contributor

qw4990 commented Nov 23, 2020

/run-all-tests

@qw4990
Copy link
Contributor

qw4990 commented Nov 23, 2020

/run-unit-test

qw4990
qw4990 reviewed Nov 23, 2020
View reviewed changes
Copy link
Contributor

@qw4990 qw4990 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 ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 23, 2020
@qw4990
Copy link
Contributor

qw4990 commented Nov 23, 2020

PTAL @pingyu

@qw4990
Copy link
Contributor

qw4990 commented Nov 23, 2020

/run-all-tests

@pingyu
Copy link
Contributor

pingyu commented Nov 23, 2020

LGTM

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Nov 23, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Nov 23, 2020
@ti-challenge-bot
Copy link

There is no reward for this challenge pull request, so you can request a reward from @qw4990.

Details

Tip : About reward you can refs to reward-command.

Warning: None

@qw4990
Copy link
Contributor

qw4990 commented Nov 24, 2020

/run-all-tests

@qw4990
Copy link
Contributor

qw4990 commented Nov 24, 2020

/run-integration-compatibility-test

@qw4990 qw4990 merged commit ceddc10 into pingcap:master Nov 24, 2020
@huang-b huang-b deleted the dev_shuffle branch November 24, 2020 03:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qw4990 qw4990 qw4990 left review comments

@pingyu pingyu pingyu requested changes

@ti-srebot ti-srebot ti-srebot approved these changes

@SunRunAway SunRunAway Awaiting requested review from SunRunAway

Assignees
No one assigned
Labels
first-time-contributor Indicates that the PR was contributed by an external member and is a first-time contributor. sig/execution SIG execution sig/planner SIG: Planner status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@huang-b @CLAassistant @sre-bot @qw4990 @pingyu @ti-srebot