[SPARK-17751] [SQL] Remove spark.sql.eagerAnalysis and Output the Plan if Existed in AnalysisException by gatorsmile · Pull Request #15316 · apache/spark

gatorsmile · 2016-09-30T22:00:44Z

What changes were proposed in this pull request?

Dataset always does eager analysis now. Thus, spark.sql.eagerAnalysis is not used any more. Thus, we need to remove it.

This PR also outputs the plan. Without the fix, the analysis error is like

cannot resolve '`k1`' given input columns: [k, v]; line 1 pos 12

After the fix, the analysis error becomes:

org.apache.spark.sql.AnalysisException: cannot resolve '`k1`' given input columns: [k, v]; line 1 pos 12;
'Project [unresolvedalias(CASE WHEN ('k1 = 2) THEN 22 WHEN ('k1 = 4) THEN 44 ELSE 0 END, None), v#6]
+- SubqueryAlias t
   +- Project [_1#2 AS k#5, _2#3 AS v#6]
      +- LocalRelation [_1#2, _2#3]

How was this patch tested?

N/A

SparkQA · 2016-09-30T23:55:07Z

Test build #66189 has finished for PR 15316 at commit c71a03e.

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

hvanhovell · 2016-10-01T00:41:24Z

Huh, when did we remove eager analysis?

gatorsmile · 2016-10-01T04:36:36Z

@hvanhovell It was removed when we migrated DataFrame to Dataset. For details, see the PR: #11443

hvanhovell · 2016-10-01T04:46:36Z

Hmmmm. I did kinda like this feature. @liancheng why did we remove this?

rxin · 2016-10-01T04:56:43Z

Shouldn't we bring it back? This is an important feature when there is a bug.

gatorsmile · 2016-10-01T04:59:46Z

To @hvanhovell @rxin

@liancheng listed the reason in the original PR.

Dataset always do eager analysis now
We used to support disabling DataFrame eager analysis to help reporting partially analyzed malformed logical plan on analysis failure. However, Dataset encoders requires eager analysis during Dataset construction. To preserve the error reporting feature, AnalysisException now takes an extra Option[LogicalPlan] argument to hold the partially analyzed plan, so that we can check the plan tree when reporting test failures. This plan is passed by QueryExecution.assertAnalyzed.

However, there is a bug in AnslysisException output. It does not output the plan. I just fixed it. See the updated PR description.

gatorsmile · 2016-10-01T05:06:37Z

Do we need a test for this fix?

SparkQA · 2016-10-01T06:32:08Z

Test build #66203 has finished for PR 15316 at commit ca45217.

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

SparkQA · 2016-10-01T18:10:25Z

Test build #66222 has finished for PR 15316 at commit ab893a6.

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

SparkQA · 2016-10-01T18:12:55Z

Test build #66221 has finished for PR 15316 at commit 8fb4f40.

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

hvanhovell · 2016-10-10T05:39:36Z

  }

  override def getMessage: String = {
+    val planAnnotation = plan.map(p => s";\n$p").getOrElse("")


Why do we need a separate method here?

When hitting an AnalysisException, we called getMessage in SQLQueryTestSuite.scala. That means, we also output the logical plan in the .sql.out when hitting an AnalysisException. The logical plan contains expression IDs. Thus, when we doing the text compare between the query results with the contents in .sql.out, it might fail due to the mismatch of expression IDs. Thus, I added an extra function here. Maybe I need to add a comment to say this is for testing only.

Ah, that makes sense.

SparkQA · 2016-10-10T19:14:36Z

Test build #66668 has finished for PR 15316 at commit 2c5de3c.

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

gatorsmile · 2016-10-10T21:41:17Z

retest this please

SparkQA · 2016-10-11T00:00:10Z

Test build #66687 has finished for PR 15316 at commit 2c5de3c.

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

gatorsmile · 2016-10-17T07:23:02Z

cc @hvanhovell @rxin Any more comment about this PR? I assume Spark 2.0.2 needs it.

Recently, when we analyzing the JIRA https://issues.apache.org/jira/browse/SPARK-17709, we are unable to see the plan due the analyzer failure. The users have to manually rebuild it with this fix. Then, we can see the failed analyzed plan.

gatorsmile · 2016-10-17T07:23:38Z

Also cc @cloud-fan and @liancheng

cloud-fan · 2016-10-17T07:29:38Z

LGTM, cc @hvanhovell @rxin for final sign-off

rxin · 2016-10-17T07:40:30Z

LGTM

SparkQA · 2016-10-17T09:27:17Z

Test build #67058 has finished for PR 15316 at commit f082643.

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

hvanhovell · 2016-10-17T18:32:23Z

LGTM. Merging to master. Thanks!

hvanhovell · 2016-10-17T18:46:20Z

@gatorsmile I cannot merge this 2.0. Can you open a backport for this?

gatorsmile · 2016-10-17T19:39:55Z

Sure, will do it soon. Thanks!

…utput the Plan if Existed in AnalysisException ### What changes were proposed in this pull request? This PR is to backport the fix #15316 to 2.0. Dataset always does eager analysis now. Thus, `spark.sql.eagerAnalysis` is not used any more. Thus, we need to remove it. This PR also outputs the plan. Without the fix, the analysis error is like ``` cannot resolve '`k1`' given input columns: [k, v]; line 1 pos 12 ``` After the fix, the analysis error becomes: ``` org.apache.spark.sql.AnalysisException: cannot resolve '`k1`' given input columns: [k, v]; line 1 pos 12; 'Project [unresolvedalias(CASE WHEN ('k1 = 2) THEN 22 WHEN ('k1 = 4) THEN 44 ELSE 0 END, None), v#6] +- SubqueryAlias t +- Project [_1#2 AS k#5, _2#3 AS v#6] +- LocalRelation [_1#2, _2#3] ``` ### How was this patch tested? N/A Author: gatorsmile <gatorsmile@gmail.com> Closes #15529 from gatorsmile/eagerAnalysis20.

… if Existed in AnalysisException ### What changes were proposed in this pull request? Dataset always does eager analysis now. Thus, `spark.sql.eagerAnalysis` is not used any more. Thus, we need to remove it. This PR also outputs the plan. Without the fix, the analysis error is like ``` cannot resolve '`k1`' given input columns: [k, v]; line 1 pos 12 ``` After the fix, the analysis error becomes: ``` org.apache.spark.sql.AnalysisException: cannot resolve '`k1`' given input columns: [k, v]; line 1 pos 12; 'Project [unresolvedalias(CASE WHEN ('k1 = 2) THEN 22 WHEN ('k1 = 4) THEN 44 ELSE 0 END, None), v#6] +- SubqueryAlias t +- Project [_1#2 AS k#5, _2#3 AS v#6] +- LocalRelation [_1#2, _2#3] ``` ### How was this patch tested? N/A Author: gatorsmile <gatorsmile@gmail.com> Closes apache#15316 from gatorsmile/eagerAnalysis.

clean

c71a03e

fix msg

ca45217

gatorsmile changed the title ~~[SPARK-17751] [SQL] Remove spark.sql.eagerAnalysis~~ [SPARK-17751] [SQL] Remove spark.sql.eagerAnalysis and Output the Plan if Existed in AnalysisException. Oct 1, 2016

gatorsmile changed the title ~~[SPARK-17751] [SQL] Remove spark.sql.eagerAnalysis and Output the Plan if Existed in AnalysisException.~~ [SPARK-17751] [SQL] Remove spark.sql.eagerAnalysis and Output the Plan if Existed in AnalysisException Oct 1, 2016

gatorsmile added 2 commits October 1, 2016 09:09

fix msg

8fb4f40

clean

ab893a6

hvanhovell reviewed Oct 10, 2016

View reviewed changes

gatorsmile added 2 commits October 10, 2016 10:05

Merge remote-tracking branch 'upstream/master' into eagerAnalysis

39dc9ce

add comments

2c5de3c

gatorsmile added 2 commits October 17, 2016 00:00

Merge remote-tracking branch 'upstream/master' into eagerAnalysis

6f51363

fix

f082643

asfgit closed this in d88a1ba Oct 17, 2016

gatorsmile mentioned this pull request Oct 18, 2016

[SPARK-17751] [SQL] [Backport-2.0] Remove spark.sql.eagerAnalysis and Output the Plan if Existed in AnalysisException #15529

Closed

Conversation

gatorsmile commented Sep 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Sep 30, 2016

Uh oh!

hvanhovell commented Oct 1, 2016

Uh oh!

gatorsmile commented Oct 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hvanhovell commented Oct 1, 2016

Uh oh!

rxin commented Oct 1, 2016

Uh oh!

gatorsmile commented Oct 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gatorsmile commented Oct 1, 2016

Uh oh!

SparkQA commented Oct 1, 2016

Uh oh!

SparkQA commented Oct 1, 2016

Uh oh!

SparkQA commented Oct 1, 2016

Uh oh!

hvanhovell Oct 10, 2016

Choose a reason for hiding this comment

Uh oh!

gatorsmile Oct 10, 2016

Choose a reason for hiding this comment

Uh oh!

hvanhovell Oct 10, 2016

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 10, 2016

Uh oh!

gatorsmile commented Oct 10, 2016

Uh oh!

SparkQA commented Oct 11, 2016

Uh oh!

gatorsmile commented Oct 17, 2016

Uh oh!

gatorsmile commented Oct 17, 2016

Uh oh!

cloud-fan commented Oct 17, 2016

Uh oh!

rxin commented Oct 17, 2016

Uh oh!

SparkQA commented Oct 17, 2016

Uh oh!

hvanhovell commented Oct 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hvanhovell commented Oct 17, 2016

Uh oh!

gatorsmile commented Oct 17, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gatorsmile commented Sep 30, 2016 •

edited

Loading

gatorsmile commented Oct 1, 2016 •

edited

Loading

gatorsmile commented Oct 1, 2016 •

edited

Loading

hvanhovell commented Oct 17, 2016 •

edited

Loading