[SPARK-32220][SQL]SHUFFLE_REPLICATE_NL Hint should not change Non-Cartesian Product join result #29035

AngersZhuuuu · 2020-07-08T07:05:33Z

What changes were proposed in this pull request?

In current Join Hint strategies, if we use SHUFFLE_REPLICATE_NL hint, it will directly convert join to Cartesian Product Join and loss join condition making result not correct.

For Example:

spark-sql> select * from test4 order by a asc;
1 2
Time taken: 1.063 seconds, Fetched 4 row(s)20/07/08 14:11:25 INFO SparkSQLCLIDriver: Time taken: 1.063 seconds, Fetched 4 row(s)
spark-sql>select * from test5 order by a asc
1 2
2 2
Time taken: 1.18 seconds, Fetched 24 row(s)20/07/08 14:13:59 INFO SparkSQLCLIDriver: Time taken: 1.18 seconds, Fetched 24 row(s)spar
spark-sql>select /*+ shuffle_replicate_nl(test4) */ * from test4 join test5 where test4.a = test5.a order by test4.a asc ;
1 2 1 2
1 2 2 2
Time taken: 0.351 seconds, Fetched 2 row(s)
20/07/08 14:18:16 INFO SparkSQLCLIDriver: Time taken: 0.351 seconds, Fetched 2 row(s)

Why are the changes needed?

Fix wrong data result

Does this PR introduce any user-facing change?

NO

How was this patch tested?

Added UT

…tesian Product join redsult

SparkQA · 2020-07-08T10:53:22Z

Test build #125314 has finished for PR 29035 at commit 27b41e5.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-07-09T06:28:10Z

Test build #125421 has finished for PR 29035 at commit aeccc25.

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

dongjoon-hyun · 2020-07-09T22:24:11Z

sql/core/src/test/scala/org/apache/spark/sql/JoinHintSuite.scala

    }
  }
+
+  test("SPARK-32220: Non Cartesian Product Join Result Correct with SHUFFLE_REPLICATE_NL hint") {


So, is this a correctness issue, @AngersZhuuuu ?

Yea, I think so. Nice catch, @AngersZhuuuu

Yea, when I try new join hint, I found this result is non-correct.

dongjoon-hyun · 2020-07-09T22:26:17Z

Thank you for reporting and making a PR, @AngersZhuuuu .

dongjoon-hyun · 2020-07-09T22:27:01Z

cc @cloud-fan

maropu · 2020-07-09T23:45:08Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/EliminateResolvedHint.scala

+    }
+  }
+
+  def removeCartesianProductJoinHint(hint: Option[HintInfo]): Option[HintInfo] = {


I think we should pass a correct condition (leftKeys and rightKeys) into CartesianProductExec instead of removing the hint:

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

Line 202 in ac6406e

Some(Seq(joins.CartesianProductExec(planLater(left), planLater(right), condition)))

It seems the spark strategy incrrectly removes a#0 = a#2;

== Optimized Logical Plan == Sort [a#0 ASC NULLS FIRST], true +- Join Inner, (a#0 = a#2), leftHint=(strategy=shuffle_replicate_nl) :- Filter isnotnull(a#0) : +- Relation[a#0,b#1] parquet +- Filter isnotnull(a#2) +- Relation[a#2,b#3] parquet == Physical Plan == *(3) Sort [a#0 ASC NULLS FIRST], true, 0 +- Exchange rangepartitioning(a#0 ASC NULLS FIRST, 200), true, [id=#87] +- CartesianProduct :- *(1) Project [a#0, b#1] : +- *(1) Filter isnotnull(a#0) : +- *(1) ColumnarToRow : +- FileScan parquet default.test4[a#0,b#1] Batched: true, DataFilters: ... +- *(2) Project [a#2, b#3] +- *(2) Filter isnotnull(a#2) +- *(2) ColumnarToRow +- FileScan parquet default.test5[a#2,b#3] Batched: true, DataFilters: ...

I think we should pass a correct condition (leftKeys and rightKeys) into CartesianProductExec instead of removing the hint:

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

Line 202 in ac6406e

Some(Seq(joins.CartesianProductExec(planLater(left), planLater(right), condition)))

Yea, in default Cartesian Product Join situation, it didn't need condition at all. So in default, it seems don't have condition when build data.

@maropu See latest change, it's ok to do like this?

Since we don't know the join keys's condition is EqualTo or EaultNullSafe so it's better just not remove it in ExtractEqualJoinKeys

You cannot use the original condition in logical.Join?

case p @ ExtractEquiJoinKeys(joinType, leftKeys, rightKeys, condition, left, right, hint) => p.condition <-- This?

You cannot use the original condition in logical.Join?

case p @ ExtractEquiJoinKeys(joinType, leftKeys, rightKeys, condition, left, right, hint) => p.condition <-- This?

Don't know we can write like this....updated..

SparkQA · 2020-07-10T07:05:02Z

Test build #125545 has finished for PR 29035 at commit d9b4dcd.

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

SparkQA · 2020-07-10T07:05:02Z

Test build #125540 has finished for PR 29035 at commit a65d332.

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

cloud-fan · 2020-07-10T07:12:54Z

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

        def createCartesianProduct() = {
          if (joinType.isInstanceOf[InnerLike]) {
-            Some(Seq(joins.CartesianProductExec(planLater(left), planLater(right), condition)))
+            Some(Seq(joins.CartesianProductExec(planLater(left), planLater(right), p.condition)))


good catch!

cloud-fan · 2020-07-10T07:14:18Z

retest this please

SparkQA · 2020-07-10T15:19:32Z

Test build #125571 has finished for PR 29035 at commit d9b4dcd.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun

+1, LGTM. I tested PySpark/Spark UT locally. Merged to master.

Tests passed in 978 seconds

OK:       2306
Failed:   0
Warnings: 0
Skipped:  1

Thank you all!

dongjoon-hyun · 2020-07-10T16:04:49Z

Hi, @AngersZhuuuu . Could you make a backport PR against branch-3.0? There is a conflict.

AngersZhuuuu · 2020-07-10T23:03:28Z

Hi, @AngersZhuuuu . Could you make a backport PR against branch-3.0? There is a conflict.

Yea

dongjoon-hyun · 2020-07-10T23:15:18Z

Thanks, @AngersZhuuuu .

maropu · 2020-07-10T23:25:27Z

late LGTM. Thanks, @AngersZhuuuu

gatorsmile · 2020-07-13T07:05:50Z

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

        def createCartesianProduct() = {
          if (joinType.isInstanceOf[InnerLike]) {
-            Some(Seq(joins.CartesianProductExec(planLater(left), planLater(right), condition)))
+            Some(Seq(joins.CartesianProductExec(planLater(left), planLater(right), p.condition)))


We need to write a comment above this line and explain what it is doing.
// instead of using the condition extracted by ExtractEquiJoinKeys, we should use the original join condition, i.e., "p.condition".

We need to write a comment above this line and explain what it is doing.
// instead of using the condition extracted by ExtractEquiJoinKeys, we should use the original join condition, i.e., "p.condition".

Raise a PR #29084

gatorsmile · 2020-07-13T07:11:26Z

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

      //   5. Pick broadcast nested loop join as the final solution. It may OOM but we don't have
      //      other choice.
-      case ExtractEquiJoinKeys(joinType, leftKeys, rightKeys, condition, left, right, hint) =>
+      case p @ ExtractEquiJoinKeys(joinType, leftKeys, rightKeys, condition, left, right, hint) =>


To avoid making the similar mistakes, we need to rename condition to a self-descriptive name. "otherConditions"? It is a little bit hard to name it TBH

…ange Non-Cartesian Product join result ### What changes were proposed in this pull request? follow comment #29035 (comment) Explain for pr ### Why are the changes needed? add comment ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Not need Closes #29084 from AngersZhuuuu/follow-spark-32220. Authored-by: angerszhu <angers.zhu@gmail.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

…ot change Non-Cartesian Product join result ### What changes were proposed in this pull request? follow comment #29035 (comment) Explain for pr ### Why are the changes needed? add comment ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Not need Closes #29093 from AngersZhuuuu/SPARK-32220-3.0-FOLLOWUP. Authored-by: angerszhu <angers.zhu@gmail.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

AngersZhuuuu added 2 commits July 8, 2020 14:56

Update JoinHintSuite.scala

5f1269f

[SPARK-32220][SQL]SHUFFLE_REPLICATE_NL Hint should not change Non-Car…

27b41e5

…tesian Product join redsult

probot-autolabeler bot added the SQL label Jul 8, 2020

Update JoinHintSuite.scala

aeccc25

dongjoon-hyun reviewed Jul 9, 2020

View reviewed changes

dongjoon-hyun changed the title ~~[SPARK-32220][SQL]SHUFFLE_REPLICATE_NL Hint should not change Non-Cartesian Product join redsult~~ [SPARK-32220][SQL]SHUFFLE_REPLICATE_NL Hint should not change Non-Cartesian Product join result Jul 9, 2020

maropu reviewed Jul 9, 2020

View reviewed changes

AngersZhuuuu added 3 commits July 10, 2020 10:14

follow comment

36ff4ca

Update JoinHintSuite.scala

a65d332

follow comment

d9b4dcd

cloud-fan reviewed Jul 10, 2020

View reviewed changes

cloud-fan approved these changes Jul 10, 2020

View reviewed changes

HyukjinKwon approved these changes Jul 10, 2020

View reviewed changes

dongjoon-hyun approved these changes Jul 10, 2020

View reviewed changes

dongjoon-hyun closed this in 560fe1f Jul 10, 2020

maropu mentioned this pull request Jul 10, 2020

[SPARK-32220][SQL][3.0]SHUFFLE_REPLICATE_NL Hint should not change Non-Cartesian Product join result #29070

Closed

gatorsmile reviewed Jul 13, 2020

View reviewed changes

AngersZhuuuu mentioned this pull request Jul 13, 2020

[SPARK-32220][SQL][FOLLOW-UP]SHUFFLE_REPLICATE_NL Hint should not change Non-Cartesian Product join result #29084

Closed

AngersZhuuuu mentioned this pull request Jul 14, 2020

[SPARK-32220][SQL][3.0][FOLLOW-UP]SHUFFLE_REPLICATE_NL Hint should not change Non-Cartesian Product join result #29093

Closed

[SPARK-32220][SQL]SHUFFLE_REPLICATE_NL Hint should not change Non-Cartesian Product join result #29035

[SPARK-32220][SQL]SHUFFLE_REPLICATE_NL Hint should not change Non-Cartesian Product join result #29035

Uh oh!

Conversation

AngersZhuuuu commented Jul 8, 2020

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!

SparkQA commented Jul 8, 2020

Uh oh!

SparkQA commented Jul 9, 2020

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!

dongjoon-hyun commented Jul 9, 2020

Uh oh!

dongjoon-hyun commented Jul 9, 2020

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 Jul 10, 2020

Uh oh!

SparkQA commented Jul 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Jul 10, 2020

Uh oh!

SparkQA commented Jul 10, 2020

Uh oh!

dongjoon-hyun left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Jul 10, 2020

Uh oh!

AngersZhuuuu commented Jul 10, 2020

Uh oh!

dongjoon-hyun commented Jul 10, 2020

Uh oh!

maropu commented Jul 10, 2020

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dongjoon-hyun left a comment •

edited

Loading