[SPARK-10484][SQL] Optimize the cartesian join with broadcast join for some cases #8652

chenghao-intel · 2015-09-08T07:50:50Z

In some cases, we can broadcast the smaller relation in cartesian join, which improve the performance significantly.

SparkQA · 2015-09-08T09:59:31Z

Test build #42126 has finished for PR 8652 at commit 98efb3d.

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

zsxwing · 2015-09-08T10:36:43Z

Looks this is a part of #7417

jameszhouyi · 2015-09-08T12:20:18Z

Hi @chenghao-intel ,
We can pass the case and also get better performance than before optimization for cross join after applying the patch.

chenghao-intel · 2015-09-08T14:38:19Z

Oh, I didn't notice that. Thank you @zsxwing , I leave some comments in #7417.

jameszhouyi · 2015-09-09T00:43:01Z

After optimized patch , we can see "CartesianProduct" optimized to "BroadcastNestedLoopJoin" from physical plan for cross join. The benchmark result showed ~42% performance gain(15m1s vs. 26m37s).

== Physical Plan ==
TungstenProject [concat(cast(s_store_sk#454L as string),_,s_store_name#455) AS store_ID#444,pr_review_date#447,pr_review_content#453]
BroadcastNestedLoopJoin BuildRight, Inner, Some((locate(lower(s_store_name#455),lower(pr_review_content#453),1) >= 1))
HiveTableScan [pr_review_date#447,pr_review_content#453], (MetastoreRelation bigbench, product_reviews, Some(pr))
HiveTableScan [s_store_sk#454L,s_store_name#455], (MetastoreRelation bigbench, temp_stores_with_regression, Some(stores_with_regression))
Code Generation: true

jameszhouyi · 2015-09-24T12:47:24Z

Hi @yhuai , I saw the PR is ready for some time. could you help to review this PR. Hopefully it can be fixed in 1.5.1.

yhuai · 2015-10-14T04:34:44Z

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkStrategies.scala

What will be the joinType at here. Why the exception at https://github.com/apache/spark/blob/master/sql/core/src/main/scala/org/apache/spark/sql/execution/joins/BroadcastNestedLoopJoin.scala is not triggered?

I am not so sure your mean about the exception at BroadcastNestedLoopJoin.scala, actually I also add the code change BroadcastNestedLoopJoin.scala below, to support the INNER join, did that your mean?

ah, sorry. I missed that line. For a CROSS JOIN, we will use INNER as the join type and the condition should be None. Can explicitly match these? I mean to let others easy to understand the code, case logical.Join(CanBroadcast(left), right, JoinType.Inner, None) will be better, right?

Actually, we are trying to support the Cartesian join by using the Broadcast join, so it will support all of the join types.

Cartesian means that there is no join condition, right?

Also, the join type is CROSS_JOIN. At here we just put INNER as a placeholder because the type does not really matter.

SparkQA · 2015-10-15T05:32:45Z

Test build #43763 has finished for PR 8652 at commit 07144ba.

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

chenghao-intel · 2015-10-15T07:33:43Z

@yhuai, after some investigation, I think the we still can optimize the left/right/full outer join in broadcasting way if they don't have condition, but LeftSemi are not included, and it should be a bug in previous implementation. And I also add more unit test by comparing with Hive.

SparkQA · 2015-10-15T07:56:14Z

Test build #43775 has finished for PR 8652 at commit 26818c7.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

chenghao-intel · 2015-10-15T08:20:45Z

retest this please

SparkQA · 2015-10-15T08:46:35Z

Test build #43779 has finished for PR 8652 at commit 26818c7.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2015-10-16T02:42:30Z

Test build #43820 has finished for PR 8652 at commit 4ee1797.

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

liancheng · 2015-10-16T18:07:20Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveQuerySuite.scala

Indentation is off here.

chenghao-intel · 2015-10-19T01:01:43Z

Thank you @liancheng updated!

SparkQA · 2015-10-19T03:18:58Z

Test build #43906 has finished for PR 8652 at commit fab8923.

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

yhuai · 2015-10-19T23:37:17Z

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkStrategies.scala

I may miss some context at here. This is a strategy for CartesianProduct. We have a strategy BroadcastNestedLoopJoin to handle other cases.

Yes, I noticed that also, however, without these changes, the cases I added will transform into the operator CartesianProduct instead of BroadcastNestedLoopJoin, as the strategy BroadcastNestedLoopJoin is the rule after CartesianProduct.

Besides, explicitly providing the rules for the optimization, probably will be helpful for people to understand how the logic behind.

PS: The rule BroadcastNestedLoopJoin has to be the last gate, as it's supposed to handle all kinds of joins.

JoshRosen · 2015-10-22T20:15:55Z

/cc @harsha2010, this PR may interest you given your similar broadcast cartesian join optimizations in Magellan.

yhuai · 2015-10-23T17:49:57Z

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkStrategies.scala

Does this comment related to this case? Also, I still do not think these two rules should be in this Strategy because the name of this strategy is CartesianProduct, but these first two cases are not for CartesianProduct. Actually, can we combine CartesianProduct and BroadcastNestedLoopJoin strategies?

Also, why condition needs to be None?

Yes, the comment is stale.

If we restrict the outer join condition as None here, then it's more like a CartesianProduct, that's why I put the rule in the CartesianProduct, and more importantly, we'd like to take those 2 rules as higher priority than the rule in Line 292.

I am totally agree with you to combine the CartesianProduct and BroadcastNestedLoopJoin, as the later just a special case of former.

Will update the code soon.

yhuai · 2015-10-26T03:04:21Z

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkPlanner.scala

Is it a proper name?

Any suggestion for the name?

how about NonEquiJoinSelection? w.r.t EquiJoinSelection

BroadcastNestedLoopJoin actually supports equi-join, thus we never run into this case, as we have more optimal solution for it in previous rules.

After a double think, I am a little hesitate to combine the rules in CartesianProduct and BroadcastNestedLoopJoin, as the later is supposed to be the last gate for JOIN, and works for all kinds of JOIN type w/ or w/o join condition, the others can be considered as the optimization compared to it.

I am going to revert the code change if @yhuai is not strongly opposite to it. Or we can refactor the JOIN strategy after this PR been merged.

What do you think @yhuai ?

I am not sure if BroadcastNestedLoopJoin is a good last gate. It is possible that we join two large tables and we cannot really use broadcast join, right?

But, it is fine to revert it since we will not make that case worse.

yhuai · 2015-10-26T03:05:40Z

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkStrategies.scala

I think these two rules using CartesianProduct should go to the bottom.

SparkQA · 2015-10-26T05:09:45Z

Test build #44329 has finished for PR 8652 at commit 564abd3.

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

SparkQA · 2015-10-26T05:25:57Z

Test build #44331 has finished for PR 8652 at commit 293e5ff.

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

chenghao-intel · 2015-10-26T05:59:39Z

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkStrategies.scala

The renaming is intended, as some of the users will extends the SQLContext, they may not ware the changing here.

chenghao-intel · 2015-10-26T06:02:38Z

Thank you @yhuai , I've just split the BroadcastNestedLoopJoin into BroadcastNestedLoop and DefaultJoin, which probably makes more sense.

SparkQA · 2015-10-26T08:16:32Z

Test build #44341 has finished for PR 8652 at commit 975eb46.

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

SparkQA · 2015-10-27T08:23:03Z

Test build #44410 has finished for PR 8652 at commit 7fda511.

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

chenghao-intel · 2015-10-28T03:05:26Z

@yhuai Any more comment on this?

yhuai · 2015-10-28T03:25:47Z

LGTM. Merging to master.

chenghao-intel mentioned this pull request Sep 9, 2015

[SPARK-9066][SQL] Improve cartesian performance #7417

Closed

yhuai reviewed Oct 14, 2015
View reviewed changes

chenghao-intel force-pushed the cartesian branch from 07144ba to 26818c7 Compare October 15, 2015 07:28

chenghao-intel force-pushed the cartesian branch from 26818c7 to 4ee1797 Compare October 16, 2015 00:27

liancheng reviewed Oct 16, 2015
View reviewed changes

sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveQuerySuite.scala Outdated

Copy link

Contributor

liancheng Oct 16, 2015

Choose a reason for hiding this comment

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

Indentation is off here.

chenghao-intel added 2 commits October 18, 2015 18:17

support broadcast based Cartesian/Cross Join

a8d3679

update the style issue and add extra assert

024a1fb

yhuai reviewed Oct 19, 2015
View reviewed changes

yhuai reviewed Oct 23, 2015
View reviewed changes

chenghao-intel force-pushed the cartesian branch from fab8923 to 564abd3 Compare October 26, 2015 02:57

yhuai reviewed Oct 26, 2015
View reviewed changes

Add DefaultJoin Strategy

975eb46

chenghao-intel force-pushed the cartesian branch from 293e5ff to 975eb46 Compare October 26, 2015 05:58

chenghao-intel reviewed Oct 26, 2015
View reviewed changes

Add TODO for further improvement.

7fda511

asfgit closed this in d9c6039 Oct 28, 2015

chenghao-intel deleted the cartesian branch October 28, 2015 04:38

[SPARK-10484][SQL] Optimize the cartesian join with broadcast join for some cases #8652

[SPARK-10484][SQL] Optimize the cartesian join with broadcast join for some cases #8652

Uh oh!

Conversation

chenghao-intel commented Sep 8, 2015

Uh oh!

SparkQA commented Sep 8, 2015

Uh oh!

zsxwing commented Sep 8, 2015

Uh oh!

jameszhouyi commented Sep 8, 2015

Uh oh!

chenghao-intel commented Sep 8, 2015

Uh oh!

jameszhouyi commented Sep 9, 2015

Uh oh!

jameszhouyi commented Sep 24, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 15, 2015

Uh oh!

chenghao-intel commented Oct 15, 2015

Uh oh!

SparkQA commented Oct 15, 2015

Uh oh!

chenghao-intel commented Oct 15, 2015

Uh oh!

SparkQA commented Oct 15, 2015

Uh oh!

SparkQA commented Oct 16, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chenghao-intel commented Oct 19, 2015

Uh oh!

SparkQA commented Oct 19, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoshRosen commented Oct 22, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 26, 2015

Uh oh!

SparkQA commented Oct 26, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!