[SPARK-36217][SQL] Rename CustomShuffleReader and OptimizeLocalShuffleReader in AQE #33429

HyukjinKwon · 2021-07-20T04:10:39Z

What changes were proposed in this pull request?

This PR proposes to rename:

Rename *Reader/*reader to *Read/*read for rules and execution plan (user-facing doc/config name remain untouched)
- *ShuffleReaderExec ->*ShuffleReadExec
- isLocalReader -> isLocalRead
- ...
Rename CustomShuffle* prefix to AQEShuffle*
Rename OptimizeLocalShuffleReader rule to OptimizeShuffleWithLocalRead

Why are the changes needed?

There are multiple problems in the current naming:

CustomShuffle* -> AQEShuffle*
it sounds like it is a pluggable API. However, this is actually only used by AQE.
OptimizeLocalShuffleReader -> OptimizeShuffleWithLocalRead
it is the name of a rule but it can be misread as a reader, which is counterintuative
*ReaderExec -> *ReadExec
Reader execution reads a bit odd. It should better be read execution (like ScanExec, ProjectExec and FilterExec). I can't find the reason to name it with something that performs an action. See also the generated plans:

Before:
```
...
* HashAggregate (12)
   +- CustomShuffleReader (11)
      +- ShuffleQueryStage (10)
         +- Exchange (9)
...
```
After:
```
...
* HashAggregate (12)
   +- AQEShuffleRead (11)
      +- ShuffleQueryStage (10)
         +- Exchange (9)
..
```

Does this PR introduce any user-facing change?

No, internal refactoring.

How was this patch tested?

Existing unittests should cover the changes.

HyukjinKwon · 2021-07-20T04:39:06Z

cc @cloud-fan and @maryannxue can you take a look please?

HyukjinKwon · 2021-07-20T04:43:46Z

cc @ulysses-you too FYI

sql/core/src/main/scala/org/apache/spark/sql/execution/adaptive/CoalesceShufflePartitions.scala

dongjoon-hyun

+1, LGTM (from my side). I agree with the reason of renaming mentioned by @HyukjinKwon .

HyukjinKwon · 2021-07-20T05:36:48Z

let me rebase. seems like it couldn't detect my GitHub actions job.

SparkQA · 2021-07-20T05:49:39Z

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

SparkQA · 2021-07-20T06:01:50Z

Test build #141300 has finished for PR 33429 at commit 2840b47.

This patch fails to build.
This patch merges cleanly.
This patch adds the following public classes (experimental):
trait AQEShuffleReadRule extends Rule[SparkPlan]
case class CoalesceShufflePartitions(session: SparkSession) extends AQEShuffleReadRule

HyukjinKwon · 2021-07-20T06:05:35Z

retest this please

SparkQA · 2021-07-20T06:27:02Z

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

cloud-fan · 2021-07-20T06:55:53Z

sql/core/src/test/scala/org/apache/spark/sql/execution/adaptive/AdaptiveQueryExecSuite.scala

@@ -139,21 +139,21 @@ class AdaptiveQueryExecSuite
    }
  }

-  private def checkNumLocalShuffleReaders(
-      plan: SparkPlan, numShufflesWithoutLocalReader: Int = 0): Unit = {
+  private def checkNumLocalShuffleReads(


"num readers" seems more natural?

or "num shuffles with local read", but it's longer

I think it's fine. The number of local shuffle reads that refers the number of (local) AQEShuffleReadExec, the number of AQE read plans.

SparkQA · 2021-07-20T07:00:08Z

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

SparkQA · 2021-07-20T07:36:50Z

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

SparkQA · 2021-07-20T08:10:37Z

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

HyukjinKwon · 2021-07-20T08:15:16Z

BTW, looks like GA changed something .. all tests fail apparently because of OOM ..

SparkQA · 2021-07-20T08:34:14Z

Test build #141297 has finished for PR 33429 at commit f2b0bab.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
trait AQEShuffleReadRule extends Rule[SparkPlan]
case class CoalesceShufflePartitions(session: SparkSession) extends AQEShuffleReadRule

SparkQA · 2021-07-20T08:53:16Z

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

SparkQA · 2021-07-20T09:00:18Z

Test build #141304 has finished for PR 33429 at commit 2840b47.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
trait AQEShuffleReadRule extends Rule[SparkPlan]
case class CoalesceShufflePartitions(session: SparkSession) extends AQEShuffleReadRule

HyukjinKwon · 2021-07-20T13:07:25Z

(rebased to resolve conflicts)

SparkQA · 2021-07-20T14:55:56Z

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

SparkQA · 2021-07-20T15:34:09Z

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

SparkQA · 2021-07-20T16:25:05Z

Test build #141335 has finished for PR 33429 at commit d64ed95.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
trait AQEShuffleReadRule extends Rule[SparkPlan]
case class CoalesceShufflePartitions(session: SparkSession) extends AQEShuffleReadRule

HyukjinKwon · 2021-07-21T00:46:30Z

I'll merge in few days if there are no more comments.

maryannxue · 2021-07-22T16:38:22Z

I am fine with "Rename OptimizeLocalShuffleReader rule to OptimizeShuffleWithLocalRead"

But can still use "reader" for the physical node name, i.e., AQEShuffleReaderExec ?

HyukjinKwon · 2021-07-23T03:15:56Z

hmm I feel like that's inconsistent with the current SparkPlan naming rules. For example, there's no -ers. One example might be V2TableWriteExec. As far as I can tell, there's almost no plan named with actors. ScanExec, ProtectExec, ExchangeExec, AggregateExec, etc. I believe using Read is more consistent with other namimg.

SparkQA · 2021-07-23T11:56:35Z

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

SparkQA · 2021-07-23T12:33:20Z

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

SparkQA · 2021-07-23T13:59:12Z

Test build #141555 has finished for PR 33429 at commit e0b8fa6.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
trait AQEShuffleReadRule extends Rule[SparkPlan]
case class CoalesceShufflePartitions(session: SparkSession) extends AQEShuffleReadRule

HyukjinKwon · 2021-07-25T02:50:08Z

Will merge tomorrow if there's no more comment @maryannxue

SparkQA · 2021-07-26T12:24:24Z

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

SparkQA · 2021-07-26T13:20:56Z

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

SparkQA · 2021-07-26T13:53:16Z

Test build #141636 has finished for PR 33429 at commit 19d6dee.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
trait AQEShuffleReadRule extends Rule[SparkPlan]
case class CoalesceShufflePartitions(session: SparkSession) extends AQEShuffleReadRule

cloud-fan · 2021-07-26T14:41:52Z

thanks, merging to master/3.2! (otherwise backporting AQE changes will be very hard)

…eReader in AQE ### What changes were proposed in this pull request? This PR proposes to rename: - Rename `*Reader`/`*reader` to `*Read`/`*read` for rules and execution plan (user-facing doc/config name remain untouched) - `*ShuffleReaderExec` ->`*ShuffleReadExec` - `isLocalReader` -> `isLocalRead` - ... - Rename `CustomShuffle*` prefix to `AQEShuffle*` - Rename `OptimizeLocalShuffleReader` rule to `OptimizeShuffleWithLocalRead` ### Why are the changes needed? There are multiple problems in the current naming: - `CustomShuffle*` -> `AQEShuffle*` it sounds like it is a pluggable API. However, this is actually only used by AQE. - `OptimizeLocalShuffleReader` -> `OptimizeShuffleWithLocalRead` it is the name of a rule but it can be misread as a reader, which is counterintuative - `*ReaderExec` -> `*ReadExec` Reader execution reads a bit odd. It should better be read execution (like `ScanExec`, `ProjectExec` and `FilterExec`). I can't find the reason to name it with something that performs an action. See also the generated plans: Before: ``` ... * HashAggregate (12) +- CustomShuffleReader (11) +- ShuffleQueryStage (10) +- Exchange (9) ... ``` After: ``` ... * HashAggregate (12) +- AQEShuffleRead (11) +- ShuffleQueryStage (10) +- Exchange (9) .. ``` ### Does this PR introduce _any_ user-facing change? No, internal refactoring. ### How was this patch tested? Existing unittests should cover the changes. Closes #33429 from HyukjinKwon/SPARK-36217. Authored-by: Hyukjin Kwon <gurwls223@apache.org> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 6e3d404) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

HyukjinKwon · 2021-07-26T15:45:49Z

Thanks Wenchen!

github-actions bot added CORE SQL labels Jul 20, 2021

HyukjinKwon marked this pull request as draft July 20, 2021 04:10

HyukjinKwon changed the title ~~[SPARK-36217][SQL] Rename CustomShuffleReader and OptimizeLocalShuffleReader~~ [WIP][SPARK-36217][SQL] Rename CustomShuffleReader and OptimizeLocalShuffleReader Jul 20, 2021

HyukjinKwon changed the title ~~[WIP][SPARK-36217][SQL] Rename CustomShuffleReader and OptimizeLocalShuffleReader~~ [WIP][SPARK-36217][SQL] Rename CustomShuffleReader and OptimizeLocalShuffleReader in AQE Jul 20, 2021

HyukjinKwon force-pushed the SPARK-36217 branch from 8099cd2 to f2b0bab Compare July 20, 2021 04:31

HyukjinKwon changed the title ~~[WIP][SPARK-36217][SQL] Rename CustomShuffleReader and OptimizeLocalShuffleReader in AQE~~ [SPARK-36217][SQL] Rename CustomShuffleReader and OptimizeLocalShuffleReader in AQE Jul 20, 2021

HyukjinKwon marked this pull request as ready for review July 20, 2021 04:38

dongjoon-hyun reviewed Jul 20, 2021

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/adaptive/CoalesceShufflePartitions.scala Outdated Show resolved Hide resolved

dongjoon-hyun approved these changes Jul 20, 2021

View reviewed changes

github-actions bot added the BUILD label Jul 20, 2021

HyukjinKwon force-pushed the SPARK-36217 branch from 3e00716 to 06a5e7e Compare July 20, 2021 05:29

HyukjinKwon force-pushed the SPARK-36217 branch from 06a5e7e to 2840b47 Compare July 20, 2021 05:37

cloud-fan reviewed Jul 20, 2021

View reviewed changes

HyukjinKwon force-pushed the SPARK-36217 branch from 2840b47 to d64ed95 Compare July 20, 2021 13:07

dongjoon-hyun approved these changes Jul 21, 2021

View reviewed changes

HyukjinKwon force-pushed the SPARK-36217 branch from d64ed95 to e0b8fa6 Compare July 23, 2021 10:27

Rename CustomShuffleReader and OptimizeLocalShuffleReader

19d6dee

HyukjinKwon force-pushed the SPARK-36217 branch from e0b8fa6 to 19d6dee Compare July 26, 2021 11:25

cloud-fan closed this in 6e3d404 Jul 26, 2021

HyukjinKwon deleted the SPARK-36217 branch January 4, 2022 00:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-36217][SQL] Rename CustomShuffleReader and OptimizeLocalShuffleReader in AQE #33429

[SPARK-36217][SQL] Rename CustomShuffleReader and OptimizeLocalShuffleReader in AQE #33429

HyukjinKwon commented Jul 20, 2021 •

edited

HyukjinKwon commented Jul 20, 2021

HyukjinKwon commented Jul 20, 2021

dongjoon-hyun left a comment

HyukjinKwon commented Jul 20, 2021

SparkQA commented Jul 20, 2021

SparkQA commented Jul 20, 2021

HyukjinKwon commented Jul 20, 2021

SparkQA commented Jul 20, 2021

cloud-fan Jul 20, 2021

cloud-fan Jul 20, 2021

HyukjinKwon Jul 20, 2021

SparkQA commented Jul 20, 2021

SparkQA commented Jul 20, 2021

SparkQA commented Jul 20, 2021

HyukjinKwon commented Jul 20, 2021

SparkQA commented Jul 20, 2021

SparkQA commented Jul 20, 2021

SparkQA commented Jul 20, 2021

HyukjinKwon commented Jul 20, 2021

SparkQA commented Jul 20, 2021

SparkQA commented Jul 20, 2021

SparkQA commented Jul 20, 2021

HyukjinKwon commented Jul 21, 2021

maryannxue commented Jul 22, 2021

HyukjinKwon commented Jul 23, 2021

SparkQA commented Jul 23, 2021

SparkQA commented Jul 23, 2021

SparkQA commented Jul 23, 2021

HyukjinKwon commented Jul 25, 2021

SparkQA commented Jul 26, 2021

SparkQA commented Jul 26, 2021

SparkQA commented Jul 26, 2021

cloud-fan commented Jul 26, 2021

HyukjinKwon commented Jul 26, 2021

[SPARK-36217][SQL] Rename CustomShuffleReader and OptimizeLocalShuffleReader in AQE #33429

[SPARK-36217][SQL] Rename CustomShuffleReader and OptimizeLocalShuffleReader in AQE #33429

Conversation

HyukjinKwon commented Jul 20, 2021 • edited

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?

HyukjinKwon commented Jul 20, 2021

HyukjinKwon commented Jul 20, 2021

dongjoon-hyun left a comment

Choose a reason for hiding this comment

HyukjinKwon commented Jul 20, 2021

SparkQA commented Jul 20, 2021

SparkQA commented Jul 20, 2021

HyukjinKwon commented Jul 20, 2021

SparkQA commented Jul 20, 2021

cloud-fan Jul 20, 2021

Choose a reason for hiding this comment

cloud-fan Jul 20, 2021

Choose a reason for hiding this comment

HyukjinKwon Jul 20, 2021

Choose a reason for hiding this comment

SparkQA commented Jul 20, 2021

SparkQA commented Jul 20, 2021

SparkQA commented Jul 20, 2021

HyukjinKwon commented Jul 20, 2021

SparkQA commented Jul 20, 2021

SparkQA commented Jul 20, 2021

SparkQA commented Jul 20, 2021

HyukjinKwon commented Jul 20, 2021

SparkQA commented Jul 20, 2021

SparkQA commented Jul 20, 2021

SparkQA commented Jul 20, 2021

HyukjinKwon commented Jul 21, 2021

maryannxue commented Jul 22, 2021

HyukjinKwon commented Jul 23, 2021

SparkQA commented Jul 23, 2021

SparkQA commented Jul 23, 2021

SparkQA commented Jul 23, 2021

HyukjinKwon commented Jul 25, 2021

SparkQA commented Jul 26, 2021

SparkQA commented Jul 26, 2021

SparkQA commented Jul 26, 2021

cloud-fan commented Jul 26, 2021

HyukjinKwon commented Jul 26, 2021

HyukjinKwon commented Jul 20, 2021 •

edited