[SPARK-31037][SQL] refine AQE config names by cloud-fan · Pull Request #27793 · apache/spark

cloud-fan · 2020-03-04T15:37:08Z

What changes were proposed in this pull request?

When introducing AQE to others, I feel the config names are a bit incoherent and hard to use.
This PR refines the config names:

remove the "shuffle" prefix. AQE is all about shuffle and we don't need to add the "shuffle" prefix everywhere.
targetPostShuffleInputSize is obscure, rename to advisoryPartitionSizeInBytes.
reducePostShufflePartitions doesn't match the actual optimization, rename to coalescePartitions
minNumPostShufflePartitions is obscure, rename it minPartitionNum under the coalescePartitions namespace
maxNumPostShufflePartitions is confusing with the word "max", rename it initialPartitionNum
skewedJoinOptimization is too verbose. skew join is a well-known terminology in database area, we can just say skewJoin

Why are the changes needed?

Make the config names easy to understand.

Does this PR introduce any user-facing change?

deprecate the config spark.sql.adaptive.shuffle.targetPostShuffleInputSize

How was this patch tested?

N/A

cloud-fan · 2020-03-04T15:37:21Z

cc @JkSelf @maryannxue

yaooqinn · 2020-03-04T15:40:45Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

SparkQA · 2020-03-04T15:45:46Z

Test build #119314 has finished for PR 27793 at commit 186a6af.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

yaooqinn · 2020-03-04T15:46:42Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

SparkQA · 2020-03-04T15:54:29Z

Test build #119316 has finished for PR 27793 at commit 2ebd979.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

maryannxue · 2020-03-04T15:57:16Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

enable -> "enabled"

maryannxue · 2020-03-04T15:59:00Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

"number" -> "minimum number"

SparkQA · 2020-03-04T16:27:56Z

Test build #119324 has finished for PR 27793 at commit 9ce3f04.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-03-04T16:35:22Z

Test build #119325 has finished for PR 27793 at commit 2dffcd5.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

yaooqinn · 2020-03-04T16:37:36Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

Can we make these shorter and easier to remember? Personally, maybe change spark.sql.adaptive.coalesceShufflePartitions.enabled to spark.sql.adaptive.mergePartitions, spark.sql.adaptive.coalesceShufflePartitions.initialPartitionNum to spark.sql.adaptive.mergePartitions.initialNum and spark.sql.adaptive.coalesceShufflePartitions.minPartitionNum to spark.sql.adaptive.mergePartiontions.minNum.

Shuffle is where this behavior happens and have been written in the doc field, we may not need to enforce it in the config name, and it seems that we do not have any other places under adaptive to coalesce partitions. And merge might be easier to spell than coalesce :)

I'm fine with "coalescePartitions".

shall we also rename advisoryShufflePartitionSizeInBytes to advisoryPartitionSizeInBytes?

advisoryPartitionSizeInBytes looks good to me.

Can we uniform the verb naming in reduce, coalesce, or merge both in here configuration name and the optimization rule name of ReduceNumShufflePartitions ? If we use the coalescePartitions, It is better to modify the optimization rule name from ReduceNumShufflePartitions to CoalesceShufflePartitions ?

Think we should, but we can do it in another PR. The code name is not user facing and doesn't need to made into 3.0.

JkSelf · 2020-03-05T04:47:29Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

local shuffle reader may optimize the local reader both build side and probe side?

SparkQA · 2020-03-05T05:32:37Z

Test build #119363 has finished for PR 27793 at commit 3daf7df.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-03-05T06:09:14Z

Test build #119366 has finished for PR 27793 at commit 5a63fdb.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-03-05T08:05:03Z

Test build #119368 has finished for PR 27793 at commit f2dea40.

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

gatorsmile · 2020-03-05T08:12:56Z

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

@@ -67,8 +67,8 @@ case class OptimizeSkewedJoin(conf: SQLConf) extends Rule[SparkPlan] {
   * SHUFFLE_TARGET_POSTSHUFFLE_INPUT_SIZE.


?change it to ADVISORY_PARTITION_SIZE_IN_BYTES?

gatorsmile · 2020-03-05T08:14:54Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

+  val SHUFFLE_TARGET_POSTSHUFFLE_INPUT_SIZE =
+    buildConf("spark.sql.adaptive.shuffle.targetPostShuffleInputSize")
+      .internal()
+      .doc("(Deprecated since Spark 3.0)")


Also tell users what is the new conf that replaces it?

The doc here is not user-facing. End users will see a message to suggest the new config by https://github.com/apache/spark/pull/27793/files#diff-9a6b543db706f1a90f790783d6930a13R2494

gatorsmile · 2020-03-05T08:18:36Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

-        "the number of post-shuffle partitions based on map output statistics.")
+  val ADVISORY_PARTITION_SIZE_IN_BYTES =
+    buildConf("spark.sql.adaptive.advisoryPartitionSizeInBytes")
+      .doc("The advisory size in bytes of the shuffle partition during adaptive optimization. " +


The advisory size in bytes of the shuffle partition during adaptive optimization (when '${ADAPTIVE_EXECUTION_ENABLED.key}' is true).

gatorsmile

LGTM except a few minor comments.

SparkQA · 2020-03-05T16:21:47Z

Test build #119381 has finished for PR 27793 at commit 8d18db8.

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

cloud-fan · 2020-03-05T16:46:52Z

thanks for review, merging to master/3.0!

When introducing AQE to others, I feel the config names are a bit incoherent and hard to use. This PR refines the config names: 1. remove the "shuffle" prefix. AQE is all about shuffle and we don't need to add the "shuffle" prefix everywhere. 2. `targetPostShuffleInputSize` is obscure, rename to `advisoryShufflePartitionSizeInBytes`. 3. `reducePostShufflePartitions` doesn't match the actual optimization, rename to `coalesceShufflePartitions` 4. `minNumPostShufflePartitions` is obscure, rename it `minPartitionNum` under the `coalesceShufflePartitions` namespace 5. `maxNumPostShufflePartitions` is confusing with the word "max", rename it `initialPartitionNum` 6. `skewedJoinOptimization` is too verbose. skew join is a well-known terminology in database area, we can just say `skewJoin` Make the config names easy to understand. deprecate the config `spark.sql.adaptive.shuffle.targetPostShuffleInputSize` N/A Closes #27793 from cloud-fan/aqe. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

### What changes were proposed in this pull request? When introducing AQE to others, I feel the config names are a bit incoherent and hard to use. This PR refines the config names: 1. remove the "shuffle" prefix. AQE is all about shuffle and we don't need to add the "shuffle" prefix everywhere. 2. `targetPostShuffleInputSize` is obscure, rename to `advisoryShufflePartitionSizeInBytes`. 3. `reducePostShufflePartitions` doesn't match the actual optimization, rename to `coalesceShufflePartitions` 4. `minNumPostShufflePartitions` is obscure, rename it `minPartitionNum` under the `coalesceShufflePartitions` namespace 5. `maxNumPostShufflePartitions` is confusing with the word "max", rename it `initialPartitionNum` 6. `skewedJoinOptimization` is too verbose. skew join is a well-known terminology in database area, we can just say `skewJoin` ### Why are the changes needed? Make the config names easy to understand. ### Does this PR introduce any user-facing change? deprecate the config `spark.sql.adaptive.shuffle.targetPostShuffleInputSize` ### How was this patch tested? N/A Closes apache#27793 from cloud-fan/aqe. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

cloud-fan force-pushed the aqe branch from b3014c5 to 186a6af Compare March 4, 2020 15:40

yaooqinn reviewed Mar 4, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala Outdated

Copy link

Member

yaooqinn Mar 4, 2020

Choose a reason for hiding this comment

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

3.0.0

cloud-fan force-pushed the aqe branch from 186a6af to 2ebd979 Compare March 4, 2020 15:45

yaooqinn reviewed Mar 4, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala Outdated

Copy link

Member

yaooqinn Mar 4, 2020

Choose a reason for hiding this comment

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

enabled

maryannxue reviewed Mar 4, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala Outdated

Copy link

Contributor

maryannxue Mar 4, 2020

Choose a reason for hiding this comment

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

enable -> "enabled"

maryannxue reviewed Mar 4, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala Outdated

Copy link

Contributor

maryannxue Mar 4, 2020

Choose a reason for hiding this comment

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

"number" -> "minimum number"

cloud-fan force-pushed the aqe branch from 2ebd979 to 9ce3f04 Compare March 4, 2020 16:20

cloud-fan force-pushed the aqe branch from 9ce3f04 to 2dffcd5 Compare March 4, 2020 16:29

yaooqinn reviewed Mar 4, 2020

View reviewed changes

JkSelf reviewed Mar 5, 2020

View reviewed changes

cloud-fan force-pushed the aqe branch from 2dffcd5 to 3daf7df Compare March 5, 2020 05:13

cloud-fan force-pushed the aqe branch from 3daf7df to 5a63fdb Compare March 5, 2020 05:59

refine AQE config names

f2dea40

cloud-fan force-pushed the aqe branch from 5a63fdb to f2dea40 Compare March 5, 2020 06:52

gatorsmile reviewed Mar 5, 2020

View reviewed changes

gatorsmile approved these changes Mar 5, 2020

View reviewed changes

dongjoon-hyun added the SQL label Mar 5, 2020

address comments

8d18db8

cloud-fan closed this in ba86524 Mar 5, 2020

wjxiz1992 mentioned this pull request Sep 17, 2025

Add a new rule to recommend AQE post shuffle partition num NVIDIA/spark-rapids-tools#1908

Merged

		@@ -67,8 +67,8 @@ case class OptimizeSkewedJoin(conf: SQLConf) extends Rule[SparkPlan] {
		* SHUFFLE_TARGET_POSTSHUFFLE_INPUT_SIZE.

Conversation

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

Uh oh!

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 Mar 4, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 4, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 4, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 4, 2020

Uh oh!

SparkQA commented Mar 4, 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!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 5, 2020

Uh oh!

SparkQA commented Mar 5, 2020

Uh oh!

SparkQA commented Mar 5, 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!

gatorsmile left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 5, 2020

Uh oh!

cloud-fan commented Mar 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Comments

cloud-fan commented Mar 4, 2020 •

edited

Loading