[SPARK-30326][SQL] Raise exception if analyzer exceed max iterations #26977

Eric5553 · 2019-12-22T00:32:32Z

What changes were proposed in this pull request?

Enhance RuleExecutor strategy to take different actions when exceeding max iterations. And raise exception if analyzer exceed max iterations.

Why are the changes needed?

Currently, both analyzer and optimizer just log warning message if rule execution exceed max iterations. They should have different behavior. Analyzer should raise exception to indicates the plan is not fixed after max iterations, while optimizer just log warning to keep the current plan. This is more feasible after SPARK-30138 was introduced.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Add test in AnalysisSuite

Eric5553 · 2019-12-22T06:17:47Z

@cloud-fan @dongjoon-hyun @HyukjinKwon
Would you please help review, thanks!

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

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

cloud-fan · 2019-12-23T05:58:55Z

If the analyzer hits max iteration, and the plan is resolved, shall we log warning or fail?

Eric5553 · 2019-12-23T18:04:39Z

@cloud-fan Good catch!

IMO, literally the description The max number of iterations the analyzer runs. infers we should try to iterate the plan to the maxIteration inclusively with no error or warning. But the current code will raise exception or log warning at the max iteration even the plan is fixed. This apply to both analyzer & optimizer.

I tried to fix it in 015e97218b60da907e49ad9bebb208a276a13354, and also added a corresponding test case in RuleExecutorSuite. The case will fail(raise exception) with original code.

What's your opinion? Please correct me if I'm wrong. Thanks!

Eric5553 · 2019-12-26T13:11:59Z

@cloud-fan @maropu @viirya Would you please help review the latest change? Thanks!

maropu · 2019-12-27T00:50:00Z

Analyzer should raise exception to indicates the plan is not fixed after max iterations

I'm currently not sure that this feature is useful... If a logical plan has unresolved nodes for that case, I think Spark should throw an analysis exception in CheckAnalysis. Do you mean that is not enough?

gatorsmile · 2019-12-27T06:19:29Z

@maropu I think the exception threw by CheckAnalysis is confusing to end users. The unresolved plans are not caused by the user queries. Instead, they need to increase the value of SQLConf.ANALYZER_MAX_ITERATIONS.

gatorsmile · 2019-12-27T06:20:20Z

If the analyzer hits max iteration, and the plan is resolved, shall we log warning or fail?

in this case, keep the existing behavior?

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/rules/RuleExecutor.scala

Eric5553 · 2019-12-28T15:03:55Z

If the analyzer hits max iteration, and the plan is resolved, shall we log warning or fail?

in this case, keep the existing behavior?

Sure, I've reverted the code to keep existing behavior for this case. Thanks for review! @gatorsmile

Eric5553 · 2020-01-03T06:54:20Z

@cloud-fan @maropu @viirya Would you please help take a look? Thanks!

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisTest.scala

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisSuite.scala

maropu · 2020-01-08T12:27:32Z

ok to test

maropu · 2020-01-08T12:29:07Z

Looks fine now. I'll leave this to the other more qualified reviewers. @cloud-fan @viirya

Eric5553 · 2020-01-08T12:45:12Z

@maropu Thanks for the detailed review!

SparkQA · 2020-01-08T13:10:02Z

Test build #116303 has finished for PR 26977 at commit ab82c41.

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

SparkQA · 2020-01-08T17:48:02Z

Test build #116305 has finished for PR 26977 at commit 259d12d.

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

Eric5553 · 2020-01-28T08:51:19Z

@cloud-fan @viirya @gatorsmile Would you please revisit the changes, thanks!

gatorsmile · 2020-02-08T04:38:41Z

retest this please

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/rules/RuleExecutor.scala

gatorsmile · 2020-02-08T04:43:44Z

LGTM except a comment about the message

gatorsmile · 2020-02-08T04:43:53Z

cc @cloud-fan @maryannxue

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

Test build #118055 has finished for PR 26977 at commit 259d12d.

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

SparkQA · 2020-02-08T12:26:36Z

Test build #118064 has finished for PR 26977 at commit c80773f.

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

SparkQA · 2020-02-09T07:29:53Z

Test build #118084 has finished for PR 26977 at commit 177adc8.

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

gatorsmile · 2020-02-10T01:40:19Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/rules/RuleExecutor.scala

-            val message = s"Max iterations (${iteration - 1}) reached for batch ${batch.name}"
-            if (Utils.isTesting) {
+            val message = s"Max iterations (${iteration - 1}) reached for batch ${batch.name}, " +
+              s"increasing the value of '${SQLConf.ANALYZER_MAX_ITERATIONS.key}'."


Actually, this change is not right. The optimizer will also issue this message.

Opps, sorry.
I updated the logic in cdc4879. Extend abstract class Strategy to support setting different hint setting key in Analyzer or Optimizer.
Would you please take time to review? Thanks!

gatorsmile · 2020-02-10T04:24:50Z

add to whitelist

cloud-fan · 2020-02-10T04:31:35Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/rules/RuleExecutor.scala

+            val message = batch.strategy.maxIterationsSetting match {
+              case setting: String if setting != null =>
+                s"Max iterations (${iteration - 1}) reached for batch ${batch.name}, " +
+                  s"increasing the value of '${setting}'."


nit: please set '${setting}' to a larget value

cloud-fan · 2020-02-10T04:33:20Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/rules/RuleExecutor.scala

@@ -155,8 +168,14 @@ abstract class RuleExecutor[TreeType <: TreeNode[_]] extends Logging {
        if (iteration > batch.strategy.maxIterations) {
          // Only log if this is a rule that is supposed to run more than once.
          if (iteration != 2) {
-            val message = s"Max iterations (${iteration - 1}) reached for batch ${batch.name}"
-            if (Utils.isTesting) {
+            val message = batch.strategy.maxIterationsSetting match {


nit:

val endingMsg = if (batch.strategy.maxIterationsSetting == null) { "." } else { ", please set '${setting}' to a larget value" } val message = s"Max iterations (${iteration - 1}) reached for batch ${batch.name}$endingMsg"

cloud-fan · 2020-02-10T04:33:55Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisSuite.scala

+    val conf = new SQLConf().copy(SQLConf.ANALYZER_MAX_ITERATIONS -> maxIterations)
+    val testAnalyzer = new Analyzer(
+      new SessionCatalog(new InMemoryCatalog, FunctionRegistry.builtin, conf),
+      new SQLConf().copy(SQLConf.ANALYZER_MAX_ITERATIONS -> maxIterations))


nit: just pass conf

Eric5553 · 2020-02-10T05:11:41Z

@cloud-fan Thanks for the review! bff75bc was to address the comments.

SparkQA · 2020-02-10T06:44:42Z

Test build #118108 has finished for PR 26977 at commit cdc4879.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class FixedPoint(

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

Test build #118112 has finished for PR 26977 at commit cdc4879.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class FixedPoint(

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

Test build #118130 has finished for PR 26977 at commit bff75bc.

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

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

Test build #118116 has finished for PR 26977 at commit bff75bc.

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

Eric5553 · 2020-02-10T08:08:07Z

retest this please

SparkQA · 2020-02-10T13:43:06Z

Test build #118141 has finished for PR 26977 at commit bff75bc.

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

cloud-fan · 2020-02-10T15:41:48Z

This needs to be in 3.0. In Spark 2.4 the analyzer max iteration was controlled by SQLConf.OPTIMIZER_MAX_ITERATIONS. In 3.0, we add a new config for it, and it's possible that existing queries set SQLConf.OPTIMIZER_MAX_ITERATIONS to a large value and they fail analysis after upgrading to Spark 3.0.

With this PR, they can get a clear error message to set the new config.

Thanks, merging to master/3.0!

Eric5553 · 2020-02-10T15:50:12Z

Thank you all !!

### What changes were proposed in this pull request? Enhance RuleExecutor strategy to take different actions when exceeding max iterations. And raise exception if analyzer exceed max iterations. ### Why are the changes needed? Currently, both analyzer and optimizer just log warning message if rule execution exceed max iterations. They should have different behavior. Analyzer should raise exception to indicates the plan is not fixed after max iterations, while optimizer just log warning to keep the current plan. This is more feasible after SPARK-30138 was introduced. ### Does this PR introduce any user-facing change? No ### How was this patch tested? Add test in AnalysisSuite Closes #26977 from Eric5553/EnhanceMaxIterations. Authored-by: Eric Wu <492960551@qq.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit b2011a2) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

### What changes were proposed in this pull request? Enhance RuleExecutor strategy to take different actions when exceeding max iterations. And raise exception if analyzer exceed max iterations. ### Why are the changes needed? Currently, both analyzer and optimizer just log warning message if rule execution exceed max iterations. They should have different behavior. Analyzer should raise exception to indicates the plan is not fixed after max iterations, while optimizer just log warning to keep the current plan. This is more feasible after SPARK-30138 was introduced. ### Does this PR introduce any user-facing change? No ### How was this patch tested? Add test in AnalysisSuite Closes apache#26977 from Eric5553/EnhanceMaxIterations. Authored-by: Eric Wu <492960551@qq.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

Eric5553 force-pushed the EnhanceMaxIterations branch from fb95d12 to cfe5a5e Compare December 22, 2019 01:01

maropu reviewed Dec 22, 2019

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/AnalyzerRuleStrategySuite.scala Outdated Show resolved Hide resolved

viirya reviewed Dec 22, 2019

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala Outdated Show resolved Hide resolved

maropu reviewed Dec 28, 2019

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/rules/RuleExecutor.scala Outdated Show resolved Hide resolved

Eric5553 requested review from viirya and maropu December 31, 2019 01:47

maropu reviewed Jan 8, 2020

View reviewed changes

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisTest.scala Outdated Show resolved Hide resolved

maropu reviewed Jan 8, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala Show resolved Hide resolved

maropu reviewed Jan 8, 2020

View reviewed changes

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisSuite.scala Show resolved Hide resolved

dongjoon-hyun added the SQL label Feb 5, 2020

gatorsmile reviewed Feb 8, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/rules/RuleExecutor.scala Show resolved Hide resolved

throw error when analyze exceed maxIterations

276332a

Eric5553 added 2 commits February 8, 2020 14:59

move AnylasisSuite helper method

b192f4f

inline the Analyzer builder

c6097e5

Add resolution message in exception

c80773f

Eric5553 force-pushed the EnhanceMaxIterations branch from 259d12d to c80773f Compare February 8, 2020 08:23

refine the message

177adc8

gatorsmile reviewed Feb 10, 2020

View reviewed changes

Refine the message for both Analyzer & Optimizer

cdc4879

cloud-fan reviewed Feb 10, 2020

View reviewed changes

Address comments

bff75bc

cloud-fan approved these changes Feb 10, 2020

View reviewed changes

cloud-fan closed this in b2011a2 Feb 10, 2020

Eric5553 deleted the EnhanceMaxIterations branch March 13, 2020 06:50

[SPARK-30326][SQL] Raise exception if analyzer exceed max iterations #26977

[SPARK-30326][SQL] Raise exception if analyzer exceed max iterations #26977

Conversation

Eric5553 commented Dec 22, 2019 • edited Loading

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?

Eric5553 commented Dec 22, 2019

cloud-fan commented Dec 23, 2019 • edited Loading

Eric5553 commented Dec 23, 2019

Eric5553 commented Dec 26, 2019

maropu commented Dec 27, 2019

gatorsmile commented Dec 27, 2019

gatorsmile commented Dec 27, 2019

Eric5553 commented Dec 28, 2019

Eric5553 commented Jan 3, 2020

maropu commented Jan 8, 2020

maropu commented Jan 8, 2020

Eric5553 commented Jan 8, 2020

SparkQA commented Jan 8, 2020

SparkQA commented Jan 8, 2020

Eric5553 commented Jan 28, 2020

gatorsmile commented Feb 8, 2020

gatorsmile commented Feb 8, 2020

gatorsmile commented Feb 8, 2020

SparkQA commented Feb 8, 2020

SparkQA commented Feb 8, 2020

SparkQA commented Feb 9, 2020

gatorsmile Feb 10, 2020

Choose a reason for hiding this comment

Eric5553 Feb 10, 2020

Choose a reason for hiding this comment

gatorsmile commented Feb 10, 2020

cloud-fan Feb 10, 2020

Choose a reason for hiding this comment

cloud-fan Feb 10, 2020

Choose a reason for hiding this comment

cloud-fan Feb 10, 2020

Choose a reason for hiding this comment

Eric5553 commented Feb 10, 2020

SparkQA commented Feb 10, 2020

SparkQA commented Feb 10, 2020

SparkQA commented Feb 10, 2020

SparkQA commented Feb 10, 2020

Eric5553 commented Feb 10, 2020

SparkQA commented Feb 10, 2020

cloud-fan commented Feb 10, 2020

Eric5553 commented Feb 10, 2020

Eric5553 commented Dec 22, 2019 •

edited

Loading

cloud-fan commented Dec 23, 2019 •

edited

Loading