[SPARK-30842][SQL] Adjust abstraction structure for join operators by Eric5553 · Pull Request #27595 · apache/spark

Eric5553 · 2020-02-15T16:07:22Z

What changes were proposed in this pull request?

Currently the join operators are not well abstracted, since there are lot of common logic. A trait can be created for easier pattern matching and other future handiness. This is a follow-up PR based on comment
#27509 (comment) .

This PR refined from the following aspects:

Refined structure of all physical join operators
Add missing joinType field for CartesianProductExec operator
Refined codes related to Explain Formatted

The EXPLAIN FORMATTED changes are

Converge all join operator verboseStringWithOperatorId implementations to BaseJoinExec. Join condition displayed, and join keys displayed if it’s not empty.
#1 will add Join condition to BroadcastNestedLoopJoinExec.
#1 will NOT affect CartesianProductExec,SortMergeJoin and HashJoins, since they already got there override implementation before.
Converge all join operator simpleStringWithNodeId to BaseJoinExec, which will enhance the one line description for CartesianProductExec with JoinType added.
Override simpleStringWithNodeId in BroadcastNestedLoopJoinExec to show BuildSide, which was only done for HashJoins before.

Why are the changes needed?

Make the code consistent with other operators and for future handiness of join operators.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Existing tests

Eric5553 · 2020-02-15T16:09:30Z

cc @maryannxue @gatorsmile @cloud-fan @maropu

SparkQA · 2020-02-15T20:16:17Z

Test build #118481 has finished for PR 27595 at commit 3ded69e.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
trait BaseJoinExec extends BinaryExecNode
trait HashJoin extends BaseJoinExec

SparkQA · 2020-02-17T19:35:55Z

Test build #118589 has finished for PR 27595 at commit 4397ce8.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-02-20T14:17:51Z

unnecessary change.

cloud-fan · 2020-02-20T14:17:59Z

unnecessary change.

SparkQA · 2020-02-21T08:05:02Z

Test build #118746 has finished for PR 27595 at commit 620c70d.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-02-21T21:58:55Z

Test build #118802 has finished for PR 27595 at commit 8604c67.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-02-24T15:50:32Z

can we include join keys here? Then we can remove the verboseStringWithOperatorId methods in join sub-classes.

And we should print nothing if join keys are empty, instead of [].

Sure, will update. Thanks!

SparkQA · 2020-02-24T20:49:16Z

Test build #118879 has finished for PR 27595 at commit 78a1bc7.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-02-25T11:39:11Z

LGTM. @Eric5553 can you list the EXPLAIN output changes introduced by this PR? Thanks!

Eric5553 · 2020-02-25T15:57:18Z

@cloud-fan Sure
The EXPLAIN FORMATTED changes are:

Converge all join operator verboseStringWithOperatorId implementations to BaseJoinExec. Join condition displayed, and join keys displayed if it’s not empty.
#1 will add Join condition to BroadcastNestedLoopJoinExec.
#1 will NOT affect CartesianProductExec,SortMergeJoin and HashJoins, since they already got there override implementation before.
Converge all join operator simpleStringWithNodeId to BaseJoinExec, which will enhance the one line description for CartesianProductExec with JoinType added.
Override simpleStringWithNodeId in BroadcastNestedLoopJoinExec to show BuildSide, which was only done for HashJoins before.

Also updated in PR description.

cloud-fan · 2020-02-27T13:41:33Z

@Eric5553 can you fix the conflicts? thanks!

Eric5553 · 2020-02-27T15:01:14Z

retest this please

SparkQA · 2020-02-27T19:47:53Z

Test build #119036 has finished for PR 27595 at commit 17af74f.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-02-27T20:46:09Z

Test build #119038 has finished for PR 27595 at commit 17af74f.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Eric5553 · 2020-02-28T01:42:34Z

@cloud-fan Sure, the conflicts have been resolved. :-)

cloud-fan · 2020-02-28T10:42:27Z

thanks, merging to master!

### What changes were proposed in this pull request? Currently the join operators are not well abstracted, since there are lot of common logic. A trait can be created for easier pattern matching and other future handiness. This is a follow-up PR based on comment apache#27509 (comment) . This PR refined from the following aspects: 1. Refined structure of all physical join operators 2. Add missing joinType field for CartesianProductExec operator 3. Refined codes related to Explain Formatted The EXPLAIN FORMATTED changes are 1. Converge all join operator `verboseStringWithOperatorId` implementations to `BaseJoinExec`. Join condition displayed, and join keys displayed if it’s not empty. 2. `apache#1` will add Join condition to `BroadcastNestedLoopJoinExec`. 3. `apache#1` will **NOT** affect `CartesianProductExec`,`SortMergeJoin` and `HashJoin`s, since they already got there override implementation before. 4. Converge all join operator `simpleStringWithNodeId` to `BaseJoinExec`, which will enhance the one line description for `CartesianProductExec` with `JoinType` added. 5. Override `simpleStringWithNodeId` in `BroadcastNestedLoopJoinExec` to show `BuildSide`, which was only done for `HashJoin`s before. ### Why are the changes needed? Make the code consistent with other operators and for future handiness of join operators. ### Does this PR introduce any user-facing change? No ### How was this patch tested? Existing tests Closesapache#27595 from Eric5553/RefineJoin. Authored-by: Eric Wu <492960551@qq.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

Eric5553 mentioned this pull request Feb 15, 2020
[SPARK-30764][SQL] Improve the readability of EXPLAIN FORMATTED style #27509
Closed

maropu reviewed Feb 16, 2020
View reviewed changes

Comment threadsql/core/src/main/scala/org/apache/spark/sql/execution/joins/BaseJoinExec.scala Outdated

hvanhovell reviewed Feb 16, 2020
View reviewed changes

Comment threadsql/core/src/main/scala/org/apache/spark/sql/execution/joins/CartesianProductExec.scala Outdated

cloud-fan reviewed Feb 20, 2020
View reviewed changes

Comment threadsql/core/src/main/scala/org/apache/spark/sql/execution/SparkStrategies.scala Outdated
cloud-fanFeb 20, 2020
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unnecessary change.

cloud-fan reviewed Feb 20, 2020
View reviewed changes

Comment threadsql/core/src/main/scala/org/apache/spark/sql/execution/SparkStrategies.scala Outdated
cloud-fanFeb 20, 2020
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unnecessary change.

Eric5553 force-pushed the RefineJoin branch from 620c70d to 8604c67Compare February 21, 2020 17:37

cloud-fan reviewed Feb 24, 2020
View reviewed changes

Comment threadsql/core/src/main/scala/org/apache/spark/sql/execution/joins/BroadcastNestedLoopJoinExec.scala Outdated

cloud-fan reviewed Feb 24, 2020
View reviewed changes

Eric5553 added 5 commits February 27, 2020 22:25

Adjust Join exec abstraction
be06b37

address comment
de2198e

Address comments and fix PySpark test
e9022a1

minor fix for rebase
79d2ac2

address comment
17af74f

Eric5553 force-pushed the RefineJoin branch from 78a1bc7 to 17af74fCompare February 27, 2020 14:34

dongjoon-hyun added the SQL label Feb 28, 2020

cloud-fan closed this in eba2076Feb 28, 2020

Eric5553 deleted the RefineJoin branch March 13, 2020 06:51

Uh oh!

Conversation

Eric5553 commented Feb 15, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

Eric5553 commented Feb 15, 2020

Uh oh!

SparkQA commented Feb 15, 2020

Uh oh!

Uh oh!

Uh oh!

SparkQA commented Feb 17, 2020

Uh oh!

cloud-fanFeb 20, 2020

Choose a reason for hiding this comment

Uh oh!

cloud-fanFeb 20, 2020

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Feb 21, 2020

Uh oh!

SparkQA commented Feb 21, 2020

Uh oh!

Uh oh!

cloud-fanFeb 24, 2020

Choose a reason for hiding this comment

Uh oh!

cloud-fanFeb 24, 2020

Choose a reason for hiding this comment

Uh oh!

Eric5553Feb 24, 2020

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Feb 24, 2020

Uh oh!

cloud-fan commented Feb 25, 2020

Uh oh!

Eric5553 commented Feb 25, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cloud-fan commented Feb 27, 2020

Uh oh!

Eric5553 commented Feb 27, 2020

Uh oh!

SparkQA commented Feb 27, 2020

Uh oh!

SparkQA commented Feb 27, 2020

Uh oh!

Eric5553 commented Feb 28, 2020

Uh oh!

cloud-fan commented Feb 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Eric5553 commented Feb 15, 2020•
edited
Loading

Eric5553 commented Feb 25, 2020•
edited
Loading