[SPARK-33494][SQL][AQE] Do not use local shuffle reader for repartition #30432

cloud-fan · 2020-11-19T19:27:00Z

What changes were proposed in this pull request?

This PR updates ShuffleExchangeExec to carry more information about how much we can change the partitioning. For repartition(col), we should preserve the user-specified partitioning and don't apply the AQE local shuffle reader.

Why are the changes needed?

Similar to repartition(number, col), we should respect the user-specified partitioning.

Does this PR introduce any user-facing change?

No

How was this patch tested?

a new test

cloud-fan · 2020-11-19T19:27:17Z

cc @maryannxue @JkSelf @manuzhang @wangyum

SparkQA · 2020-11-19T20:12:03Z

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

SparkQA · 2020-11-19T20:32:12Z

Test build #131369 has finished for PR 30432 at commit 7b556cf.

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

SparkQA · 2020-11-19T20:42:45Z

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

JkSelf · 2020-11-20T04:14:37Z

@cloud-fan
If we want to preserve the user-specified partitioning, whether need to disable the CoalesceShufflePartitions rule for the repartition(col) or not? Because the outputPartitioning will be changed to UnknownPartitioning after coalesced.

maryannxue · 2020-11-20T18:00:43Z

I believe this is more of a refactoring than a fix, @JkSelf .
Can we change the title of this PR, @cloud-fan ?

maryannxue · 2020-11-20T19:36:31Z

sql/core/src/main/scala/org/apache/spark/sql/execution/exchange/ShuffleExchangeExec.scala

   * Returns whether the shuffle partition number can be changed.
   */
-  def canChangeNumPartitions: Boolean
+  def canChangeNumPartitions: Boolean = {


I feel like these two methods are redundant if we have partitioningFlexibilty already.

I know that removing these methods would cause more code changes on the caller side, but for the integrity and simplicity of the interface, it's probably worth it.

This is more like a util method as implementations don't need to implement it.

cloud-fan · 2020-11-23T06:34:05Z

whether need to disable the CoalesceShufflePartitions rule for the repartition(col) or not?

We already did. We don't coalesce shuffles if canChangeNumPartitions return false.

cloud-fan · 2020-11-23T06:34:32Z

I believe this is more of a refactoring than a fix

It does fix an issue, see the added test.

JkSelf · 2020-11-23T07:50:45Z

We already did. We don't coalesce shuffles if canChangeNumPartitions return false.

The canChangeNumPartitions return true when call the repartition(col). So aqe will coalesce shuffles and change the user-specified partitioning to UnknownPartitioning .

cloud-fan · 2020-11-23T08:33:26Z

@JkSelf code-wise, you are right, as it becomes UnknownPartitioning. However, here I'm talking about the real partitioning. After coalesce, the data is still "partitioned by col" for repartition(col), so we don't break user expectations. Does it make sense to you?

JkSelf · 2020-11-23T08:50:23Z

I see. It makes sense to me. Thanks for your detailed explanations.

SparkQA · 2020-11-23T09:33:01Z

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

SparkQA · 2020-11-23T09:56:57Z

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

SparkQA · 2020-11-23T13:03:23Z

Test build #131545 has finished for PR 30432 at commit 40a0096.

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

wangyum · 2020-11-23T14:11:39Z

retest this please.

SparkQA · 2020-11-23T14:59:56Z

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

SparkQA · 2020-11-23T15:28:36Z

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

SparkQA · 2020-11-23T15:48:26Z

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

SparkQA · 2020-11-23T16:16:50Z

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

SparkQA · 2020-11-23T17:12:17Z

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

SparkQA · 2020-11-23T17:37:28Z

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

SparkQA · 2020-11-23T18:22:04Z

Test build #131556 has finished for PR 30432 at commit 5f8f4ed.

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

SparkQA · 2020-11-23T18:28:47Z

Test build #131551 has finished for PR 30432 at commit 5383910.

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

SparkQA · 2020-11-23T18:37:00Z

Test build #131549 has finished for PR 30432 at commit 40a0096.

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

SparkQA · 2020-11-24T02:31:13Z

Test build #131595 has finished for PR 30432 at commit 5f8f4ed.

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

maryannxue · 2020-11-24T04:12:17Z

sql/core/src/main/scala/org/apache/spark/sql/execution/exchange/ShuffleExchangeExec.scala

 }

+// Describes where the shuffle operator comes from.
+object ShuffleOrigin extends Enumeration {


Can we use seal trait/abstract class, so we type won't have a trailing .Value?

SparkQA · 2020-11-24T08:05:02Z

Test build #131627 has finished for PR 30432 at commit 1ed28f2.

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

cloud-fan · 2020-11-24T08:52:59Z

retest this please

SparkQA · 2020-11-24T10:46:05Z

Test build #131636 has finished for PR 30432 at commit 1ed28f2.

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

maryannxue

LGTM

gatorsmile · 2020-11-24T16:22:19Z

LGTM

Thanks! Merged to master

This PR updates `ShuffleExchangeExec` to carry more information about how much we can change the partitioning. For `repartition(col)`, we should preserve the user-specified partitioning and don't apply the AQE local shuffle reader. Similar to `repartition(number, col)`, we should respect the user-specified partitioning. No a new test Closes apache#30432 from cloud-fan/aqe. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

Do not use local shuffle reader for repartition

7b556cf

github-actions bot added the SQL label Nov 19, 2020

maryannxue reviewed Nov 20, 2020

View reviewed changes

fix test

40a0096

add final

5383910

better naming

5f8f4ed

maryannxue reviewed Nov 24, 2020

View reviewed changes

refine

1ed28f2

maryannxue approved these changes Nov 24, 2020

View reviewed changes

cloud-fan closed this in d1b4f06 Nov 25, 2020

abellina mentioned this pull request Nov 25, 2020

Use CUDF's "UNBOUNDED" window boundaries for time-range queries. NVIDIA/spark-rapids#1169

Merged

MGHawes mentioned this pull request May 16, 2021

Mh/cherry pick spark 33494 palantir/spark#762

Closed

LorenzoMartini mentioned this pull request May 18, 2021

[SPARK-33494][SQL][AQE] Do not use local shuffle reader for repartition palantir/spark#763

Closed

LorenzoMartini mentioned this pull request May 18, 2021

Cherry Pick [SPARK-31220][SPARK-32056][SPARK-33494] palantir/spark#764

Merged

[SPARK-33494][SQL][AQE] Do not use local shuffle reader for repartition #30432

[SPARK-33494][SQL][AQE] Do not use local shuffle reader for repartition #30432

Uh oh!

Conversation

cloud-fan commented Nov 19, 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!

cloud-fan commented Nov 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Nov 19, 2020

Uh oh!

SparkQA commented Nov 19, 2020

Uh oh!

SparkQA commented Nov 19, 2020

Uh oh!

JkSelf commented Nov 20, 2020

Uh oh!

maryannxue commented Nov 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maryannxue Nov 20, 2020

Choose a reason for hiding this comment

Uh oh!

cloud-fan Nov 23, 2020

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Nov 23, 2020

Uh oh!

cloud-fan commented Nov 23, 2020

Uh oh!

JkSelf commented Nov 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cloud-fan commented Nov 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JkSelf commented Nov 23, 2020

Uh oh!

SparkQA commented Nov 23, 2020

Uh oh!

SparkQA commented Nov 23, 2020

Uh oh!

SparkQA commented Nov 23, 2020

Uh oh!

wangyum commented Nov 23, 2020

Uh oh!

SparkQA commented Nov 23, 2020

Uh oh!

SparkQA commented Nov 23, 2020

Uh oh!

SparkQA commented Nov 23, 2020

Uh oh!

SparkQA commented Nov 23, 2020

Uh oh!

SparkQA commented Nov 23, 2020

Uh oh!

SparkQA commented Nov 23, 2020

Uh oh!

SparkQA commented Nov 23, 2020

Uh oh!

SparkQA commented Nov 23, 2020

Uh oh!

SparkQA commented Nov 23, 2020

Uh oh!

SparkQA commented Nov 24, 2020

Uh oh!

maryannxue Nov 24, 2020

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 24, 2020

Uh oh!

cloud-fan commented Nov 24, 2020

Uh oh!

SparkQA commented Nov 24, 2020

Uh oh!

maryannxue left a comment

cloud-fan commented Nov 19, 2020 •

edited

Loading

maryannxue commented Nov 20, 2020 •

edited

Loading

JkSelf commented Nov 23, 2020 •

edited

Loading

cloud-fan commented Nov 23, 2020 •

edited

Loading