[SPARK-36652][SQL] AQE dynamic join selection should not apply to non-equi join #33899

c21 · 2021-09-02T14:43:03Z

What changes were proposed in this pull request?

Currently DynamicJoinSelection has two features: 1.demote broadcast hash join, and 2.promote shuffled hash join. Both are achieved by adding join hint in query plan, and only works for equi join. However the rule is matching with Join operator now, so it would add hint for non-equi join by mistake (See added test query in JoinHintSuite.scala for an example).

This PR is to fix DynamicJoinSelection to only apply to equi-join, and improve checkHintNonEquiJoin to check we should not add PREFER_SHUFFLE_HASH for non-equi join.

Why are the changes needed?

Improve the logic of codebase to be better.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Added unit test in JoinHintSuite.scala.

c21 · 2021-09-02T14:43:33Z

@cloud-fan and @ulysses-you could you help take a look when you have time? Thanks.

cloud-fan · 2021-09-02T14:49:14Z

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

+    withLogAppender(hintAppender, level = Some(Level.WARN)) {
+      withSQLConf(
+        SQLConf.ADAPTIVE_EXECUTION_ENABLED.key -> "true",
+        SQLConf.ADAPTIVE_MAX_SHUFFLE_HASH_JOIN_LOCAL_MAP_THRESHOLD.key -> "67108865") {


what's this magic number?

It's due to advisoryPartitionSize <= maxShuffledHashJoinLocalMapThreshold in https://github.com/apache/spark/blob/master/sql/core/src/main/scala/org/apache/spark/sql/execution/adaptive/DynamicJoinSelection.scala#L48. spark.sql.adaptive.advisoryPartitionSizeInBytes by default is 64MB, which is 67108864. +1 one here to trigger the bug. This is just an example.

I can change the number to 64MB if it's preferred.

yea '64MB' seems better

@cloud-fan - sure, updated.

cloud-fan · 2021-09-02T14:49:27Z

is it only a log issue?

c21 · 2021-09-02T14:50:43Z

is it only a log issue?

@cloud-fan - yes I think so. We just logged a warning for these wrongly-applied hint. So it should not be a blocker for release.

ulysses-you · 2021-09-02T15:20:47Z

@c21 thank you for the fix, I think this issue only exists in master branch since the hint check #32355 is only merged into master after branch-3.2 cut.

The origin idea of DynamicJoinSelection is giving some hint which can be applied potentially after match the join strategy so it's no harm for non-eqai join. The only thing changed is we add the hint check. So I think it's safe to only match the equi join at DynamicJoinSelection side.

ulysses-you · 2021-09-02T15:25:03Z

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

@@ -169,7 +169,8 @@ abstract class SparkStrategies extends QueryPlanner[SparkPlan] {
    }

    private def checkHintNonEquiJoin(hint: JoinHint): Unit = {
-      if (hintToShuffleHashJoin(hint) || hintToSortMergeJoin(hint)) {
+      if (hintToShuffleHashJoin(hint) || hintToPreferShuffleHashJoin(hint) ||
+        hintToSortMergeJoin(hint)) {


PreferShuffleHashJoin is an internal hint so user cann't use it, we don't need do this check

Yeah I am aware of this hint is internal only. I am more thinking towards to catch and expose the bug where we apply PREFER_SHUFFLE_HASH internally by mistake (e.g. for the added unit test query in JoinHintSuite.scala). I feel it's no harm to add here and help catch more bugs in the future. But if you guys think we should not add it, I can also remove it. cc @cloud-fan .

dongjoon-hyun · 2021-09-02T16:06:01Z

cc @gengliangwang , too

gengliangwang · 2021-09-02T16:26:27Z

@c21 thank you for the fix, I think this issue only exists in master branch since the hint check #32355 is only merged into master after branch-3.2 cut.

then we should update the affected version in jira SPARK-36652

c21 · 2021-09-02T16:28:38Z

then we should update the affected version in jira SPARK-36652

Yep updated. Sorry for confusion @gengliangwang.

dongjoon-hyun · 2021-09-02T16:38:09Z

Thank you for updating the affected version. :)

…SHOLD.key to 64MB

SparkQA · 2021-09-02T21:01:01Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/47440/

SparkQA · 2021-09-02T21:42:41Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/47440/

dongjoon-hyun · 2021-09-02T22:48:43Z

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

@@ -169,7 +169,8 @@ abstract class SparkStrategies extends QueryPlanner[SparkPlan] {
    }

    private def checkHintNonEquiJoin(hint: JoinHint): Unit = {
-      if (hintToShuffleHashJoin(hint) || hintToSortMergeJoin(hint)) {
+      if (hintToShuffleHashJoin(hint) || hintToPreferShuffleHashJoin(hint) ||
+        hintToSortMergeJoin(hint)) {


nit. indentation

@dongjoon-hyun - sure, updated.

dongjoon-hyun

+1, LGTM (with one nit comment)

SparkQA · 2021-09-03T00:56:15Z

Test build #142939 has finished for PR 33899 at commit f535fdf.

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

SparkQA · 2021-09-03T02:22:09Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/47449/

SparkQA · 2021-09-03T03:04:01Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/47449/

dongjoon-hyun · 2021-09-03T03:05:52Z

Thank you, @c21 , @cloud-fan , @ulysses-you , @gengliangwang .
The last commit is only updating indentation and the tests passed already at the previous commit.
Merged to master.

c21 · 2021-09-03T03:14:42Z

Thank you all for review!

SparkQA · 2021-09-03T06:29:04Z

Test build #142948 has finished for PR 33899 at commit b6002a1.

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

AmplabJenkins · 2021-09-03T06:31:25Z

Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/142948/

AQE dynamic join selection should not apply to non-equi join

37f9d42

github-actions bot added the SQL label Sep 2, 2021

cloud-fan reviewed Sep 2, 2021

View reviewed changes

ulysses-you reviewed Sep 2, 2021

View reviewed changes

cloud-fan approved these changes Sep 2, 2021

View reviewed changes

Change value of SQLConf.ADAPTIVE_MAX_SHUFFLE_HASH_JOIN_LOCAL_MAP_THRE…

f535fdf

…SHOLD.key to 64MB

dongjoon-hyun reviewed Sep 2, 2021

View reviewed changes

dongjoon-hyun approved these changes Sep 2, 2021

View reviewed changes

Fix indentation

b6002a1

dongjoon-hyun closed this in 9054a6a Sep 3, 2021

c21 deleted the aqe-test branch September 7, 2021 17:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-36652][SQL] AQE dynamic join selection should not apply to non-equi join #33899

[SPARK-36652][SQL] AQE dynamic join selection should not apply to non-equi join #33899

c21 commented Sep 2, 2021

c21 commented Sep 2, 2021

cloud-fan Sep 2, 2021

c21 Sep 2, 2021

c21 Sep 2, 2021

cloud-fan Sep 2, 2021

c21 Sep 2, 2021

cloud-fan commented Sep 2, 2021

c21 commented Sep 2, 2021

ulysses-you commented Sep 2, 2021

ulysses-you Sep 2, 2021

c21 Sep 2, 2021

dongjoon-hyun commented Sep 2, 2021

gengliangwang commented Sep 2, 2021

c21 commented Sep 2, 2021

dongjoon-hyun commented Sep 2, 2021

SparkQA commented Sep 2, 2021

SparkQA commented Sep 2, 2021

dongjoon-hyun Sep 2, 2021

c21 Sep 3, 2021

dongjoon-hyun left a comment

SparkQA commented Sep 3, 2021

SparkQA commented Sep 3, 2021

SparkQA commented Sep 3, 2021

dongjoon-hyun commented Sep 3, 2021

c21 commented Sep 3, 2021

SparkQA commented Sep 3, 2021

AmplabJenkins commented Sep 3, 2021

[SPARK-36652][SQL] AQE dynamic join selection should not apply to non-equi join #33899

[SPARK-36652][SQL] AQE dynamic join selection should not apply to non-equi join #33899

Conversation

c21 commented Sep 2, 2021

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?

c21 commented Sep 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan commented Sep 2, 2021

c21 commented Sep 2, 2021

ulysses-you commented Sep 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongjoon-hyun commented Sep 2, 2021

gengliangwang commented Sep 2, 2021

c21 commented Sep 2, 2021

dongjoon-hyun commented Sep 2, 2021

SparkQA commented Sep 2, 2021

SparkQA commented Sep 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongjoon-hyun left a comment

Choose a reason for hiding this comment

SparkQA commented Sep 3, 2021

SparkQA commented Sep 3, 2021

SparkQA commented Sep 3, 2021

dongjoon-hyun commented Sep 3, 2021

c21 commented Sep 3, 2021

SparkQA commented Sep 3, 2021

AmplabJenkins commented Sep 3, 2021