[SPARK-30719][SQL] do not log warning if AQE is intentionally skipped and add a config to force apply #27452

cloud-fan · 2020-02-04T12:04:02Z

What changes were proposed in this pull request?

Update InsertAdaptiveSparkPlan to not log warning if AQE is skipped intentionally.

This PR also add a config to not skip AQE.

Why are the changes needed?

It's not a warning at all if we intentionally skip AQE.

Does this PR introduce any user-facing change?

no

How was this patch tested?

run AdaptiveQueryExecSuite locally and verify that there is no warning logs.

cloud-fan · 2020-02-04T12:04:19Z

cc @maryannxue @JkSelf

maryannxue · 2020-02-04T15:33:44Z

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

+  val ADAPTIVE_EXECUTION_FORCE_APPLY = buildConf("spark.sql.adaptive.forceApply")
+    .internal()
+    .doc("Adaptive query execution is skipped when the query does not have exchanges or " +
+      "subqueries. By setting this config to true, Spark will be forced to apply adaptive " +


nit: Spark will force apply adaptive query execution for all supported queries.

maryannxue · 2020-02-04T15:36:11Z

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

+  //     only know if the query may contain exchange or not by checking
+  //     `SparkPlan.requiredChildDistribution`.
+  //   - The query contains sub-query.
+  private def shouldApplyAQE(plan: SparkPlan, isSubquery: Boolean): Boolean = {


How about we just move the logic of mayContainExchange and containSubquery here and get rid of those methods?

maryannxue · 2020-02-04T15:39:59Z

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

+  //     `SparkPlan.requiredChildDistribution`.
+  //   - The query contains sub-query.
+  private def shouldApplyAQE(plan: SparkPlan, isSubquery: Boolean): Boolean = {
+    conf.getConf(SQLConf.ADAPTIVE_EXECUTION_FORCE_APPLY) || isSubquery ||


nit: we could move conf.adaptiveExecutionEnabled here too.

SparkQA · 2020-02-04T16:12:20Z

Test build #117833 has finished for PR 27452 at commit fb89132.

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

maryannxue

LGTM

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

Test build #117895 has finished for PR 27452 at commit 215435d.

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

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

Test build #117884 has finished for PR 27452 at commit 841273e.

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

JkSelf · 2020-02-05T08:10:44Z

LGTM.

JkSelf · 2020-02-05T08:11:32Z

retest this please

SparkQA · 2020-02-05T11:24:10Z

Test build #117917 has finished for PR 27452 at commit 215435d.

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

cloud-fan · 2020-02-05T11:53:07Z

retest this please

SparkQA · 2020-02-05T16:51:46Z

Test build #117931 has finished for PR 27452 at commit 215435d.

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

dongjoon-hyun · 2020-02-05T19:22:29Z

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

    .booleanConf
    .createWithDefault(false)

+  val ADAPTIVE_EXECUTION_FORCE_APPLY = buildConf("spark.sql.adaptive.forceApply")


spark.sql.adaptive.forceApply.enabled?
cc @gatorsmile

No... that sounds weird.

@maryannxue . .enabled is a general guideline for boolean flag from @gatorsmile .

No... that sounds weird.

it's usually xxx.featureName.enabled, but forceApply is a verb. For example, spark.sql.join.preferSortMergeJoin

Oh. Got it. I misunderstood the rule at that part until now. My bad. Thank you, @cloud-fan and @maryannxue .

do we have a policy that config name must be xxx.featureName.enabled? At least for internal configs we follow PR author's personal preference AFAIK.

We should then stop renaming the configurations to add .enabled (e.g., #27346, #27210, #26694). reuse, ignore, and fail can be a verb either.

@gatorsmile do we need to add ".enabled" post-fix to all boolean configs?

After this PR, it would be great if we have a documented policy for this, @gatorsmile and @cloud-fan .

Thank you for the references, @HyukjinKwon .

dongjoon-hyun · 2020-02-05T19:25:32Z

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

+    .internal()
+    .doc("Adaptive query execution is skipped when the query does not have exchanges or " +
+      "subqueries. By setting this config to true, Spark will force apply adaptive query " +
+      "execution for all supported queries.")


To be clear, shall we mention like By setting both spark.sql.adaptive.enabled and this config to true,?

By setting this config (together with spark.sql.adaptive.enabled) to true

That sounds good.

dongjoon-hyun

@cloud-fan .
Could you revise the PR title? Currently, it looks like a warning suppression, but we are adding a new configuration spark.sql.adaptive.forceApply technically.

SparkQA · 2020-02-06T07:43:08Z

Test build #117975 has finished for PR 27452 at commit a796d19.

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

cloud-fan · 2020-02-06T08:15:14Z

retest this please

SparkQA · 2020-02-06T12:29:47Z

Test build #117982 has finished for PR 27452 at commit a796d19.

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

gatorsmile · 2020-02-06T16:29:20Z

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

+  //   - The query may need to add exchanges. It's an overkill to run `EnsureRequirements` here, so
+  //     we just check `SparkPlan.requiredChildDistribution` and see if it's possible that the
+  //     the query needs to add exchanges later.
+  //   - The query contains sub-query.


This is much clear than the original code.

gatorsmile

@JkSelf Could you write a test suite to test whether we are correctly logging a warning?

… and add a config to force apply ### What changes were proposed in this pull request? Update `InsertAdaptiveSparkPlan` to not log warning if AQE is skipped intentionally. This PR also add a config to not skip AQE. ### Why are the changes needed? It's not a warning at all if we intentionally skip AQE. ### Does this PR introduce any user-facing change? no ### How was this patch tested? run `AdaptiveQueryExecSuite` locally and verify that there is no warning logs. Closes #27452 from cloud-fan/aqe. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Xiao Li <[email protected]> (cherry picked from commit 8ce5862) Signed-off-by: Xiao Li <[email protected]>

gatorsmile · 2020-02-06T17:16:55Z

Thanks! Merged to master/3.0

JkSelf · 2020-02-10T04:40:47Z

@gatorsmile Add unit test in #27515.

… intentionally skip AQE ### What changes were proposed in this pull request? This is a follow up in [#27452](#27452). Add a unit test to verify whether the log warning is print when intentionally skip AQE. ### Why are the changes needed? Add unit test ### Does this PR introduce any user-facing change? No ### How was this patch tested? adding unit test Closes #27515 from JkSelf/aqeLoggingWarningTest. Authored-by: jiake <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

… intentionally skip AQE ### What changes were proposed in this pull request? This is a follow up in [#27452](#27452). Add a unit test to verify whether the log warning is print when intentionally skip AQE. ### Why are the changes needed? Add unit test ### Does this PR introduce any user-facing change? No ### How was this patch tested? adding unit test Closes #27515 from JkSelf/aqeLoggingWarningTest. Authored-by: jiake <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 5a24060) Signed-off-by: Wenchen Fan <[email protected]>

… and add a config to force apply ### What changes were proposed in this pull request? Update `InsertAdaptiveSparkPlan` to not log warning if AQE is skipped intentionally. This PR also add a config to not skip AQE. ### Why are the changes needed? It's not a warning at all if we intentionally skip AQE. ### Does this PR introduce any user-facing change? no ### How was this patch tested? run `AdaptiveQueryExecSuite` locally and verify that there is no warning logs. Closes apache#27452 from cloud-fan/aqe. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Xiao Li <[email protected]>

… intentionally skip AQE ### What changes were proposed in this pull request? This is a follow up in [apache#27452](apache#27452). Add a unit test to verify whether the log warning is print when intentionally skip AQE. ### Why are the changes needed? Add unit test ### Does this PR introduce any user-facing change? No ### How was this patch tested? adding unit test Closes apache#27515 from JkSelf/aqeLoggingWarningTest. Authored-by: jiake <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

do not log warning if AQE is intentionally skipped

fb89132

maryannxue reviewed Feb 4, 2020

View reviewed changes

address comments

215435d

cloud-fan force-pushed the aqe branch from 841273e to 215435d Compare February 5, 2020 05:40

maryannxue approved these changes Feb 5, 2020

View reviewed changes

dongjoon-hyun added the SQL label Feb 5, 2020

dongjoon-hyun reviewed Feb 5, 2020

View reviewed changes

dongjoon-hyun requested changes Feb 5, 2020

View reviewed changes

cloud-fan changed the title ~~[SPARK-30719][SQL] do not log warning if AQE is intentionally skipped~~ [SPARK-30719][SQL] do not log warning if AQE is intentionally skipped and add a config to force apply Feb 6, 2020

address comments

a796d19

gatorsmile reviewed Feb 6, 2020

View reviewed changes

gatorsmile approved these changes Feb 6, 2020

View reviewed changes

gatorsmile closed this in 8ce5862 Feb 6, 2020

JkSelf mentioned this pull request Feb 10, 2020

[SPARK-30719][SQL][TEST] Add unit test to verify the log warning print when intentionally skip AQE #27515

Closed

[SPARK-30719][SQL] do not log warning if AQE is intentionally skipped and add a config to force apply #27452

[SPARK-30719][SQL] do not log warning if AQE is intentionally skipped and add a config to force apply #27452

Uh oh!

Conversation

cloud-fan commented Feb 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 Feb 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!

SparkQA commented Feb 4, 2020

Uh oh!

maryannxue left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Feb 5, 2020

Uh oh!

SparkQA commented Feb 5, 2020

Uh oh!

JkSelf commented Feb 5, 2020

Uh oh!

JkSelf commented Feb 5, 2020

Uh oh!

SparkQA commented Feb 5, 2020

Uh oh!

cloud-fan commented Feb 5, 2020

Uh oh!

SparkQA commented Feb 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!

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!

dongjoon-hyun Feb 5, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Feb 6, 2020

Uh oh!

cloud-fan commented Feb 6, 2020

Uh oh!

SparkQA commented Feb 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatorsmile left a comment

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Feb 4, 2020 •

edited

Loading

dongjoon-hyun Feb 5, 2020 •

edited

Loading