[SPARK-32528][SQL][TEST] The analyze method should make sure the plan is analyzed #29349

cloud-fan · 2020-08-04T14:49:52Z

What changes were proposed in this pull request?

This PR updates the analyze method to make sure the plan can be resolved. It also fixes some miswritten optimizer tests.

Why are the changes needed?

It's error-prone if the analyze method can return an unresolved plan.

Does this PR introduce any user-facing change?

no

How was this patch tested?

test only

SparkQA · 2020-08-04T17:50:37Z

Test build #127055 has finished for PR 29349 at commit 90c75fb.

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

HyukjinKwon · 2020-08-05T04:01:00Z

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

 class AnalysisSuite extends AnalysisTest with Matchers {
  import org.apache.spark.sql.catalyst.analysis.TestRelations._

+  test("fail for unresolved plan") {


Shall we add a JIRA prefix?

This is very general and not a regression test, maybe not needed?

maropu · 2020-08-06T05:18:07Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/dsl/package.scala

-        EliminateSubqueryAliases(analysis.SimpleAnalyzer.execute(logicalPlan))
+      def analyze: LogicalPlan = {
+        val analyzed = analysis.SimpleAnalyzer.execute(logicalPlan)
+        analysis.SimpleAnalyzer.checkAnalysis(analyzed)


Oh, this change can find many miswritten tests..., nice.

cloud-fan · 2020-08-06T12:01:36Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/FilterPushdownSuite.scala

    comparePlans(Optimize.execute(originalQuery.analyze), correctAnswer)
  }

-  test("join condition pushdown: deterministic and non-deterministic") {


This was added in #14815

It's not valid anymore, as we don't allow nondeterministic join condition now.

SparkQA · 2020-08-06T16:52:33Z

Test build #127142 has finished for PR 29349 at commit 5a71545.

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

cloud-fan · 2020-08-07T05:26:19Z

It's ready for review. @viirya @maropu @dongjoon-hyun

viirya · 2020-08-07T05:57:32Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/FilterPushdownSuite.scala

-    comparePlans(Optimize.execute(originalQuery.analyze), correctAnswer.analyze,
-      checkAnalysis = false)


We set checkAnalysis as false so didn't find it is not resolved?

Yes. It's true that sometimes we need to construct a special unresolved plan to test some special branches of the optimizer rule. But it's not the case here.

viirya · 2020-08-07T06:08:40Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/dsl/package.scala


-      def analyze: LogicalPlan =
-        EliminateSubqueryAliases(analysis.SimpleAnalyzer.execute(logicalPlan))
+      def analyze: LogicalPlan = {


This looks nice. This seems only used in test.

cloud-fan · 2020-08-07T06:08:53Z

...test/scala/org/apache/spark/sql/catalyst/optimizer/BinaryComparisonSimplificationSuite.scala

-    val originalQuery = nonNullableRelation
-        .where(EqualTo(fieldA1, fieldA2))
-        .analyze
+    val originalQuery = nonNullableRelation.where(EqualTo(fieldA1, fieldA2))


Here we construct the resolved plan directly, because we need to reply on case insensitive and .analyze use case sensitive analyzer.

I go to look at this test and its original PR. fieldA1 and fieldA2 are not different in letter case, but the name in GetStructField. That is why above comment is GetStructField with different names are semantically equal.

cloud-fan · 2020-08-07T06:09:17Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/FilterPushdownSuite.scala

-    val originalQuery = EventTimeWatermark('b, interval, testRelation)
-      .where('a === 5 && 'b === 10 && 'c === 5)
+    val originalQuery = EventTimeWatermark('b, interval, relation)
+      .where('a === 5 && 'b === new java.sql.Timestamp(0) && 'c === 5)


watermark column must be timestamp.

cloud-fan · 2020-08-07T06:10:03Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/JoinReorderSuite.scala

      t1.join(t2, Inner, Some(nameToAttr("t1.k-1-2") === nameToAttr("t2.k-1-5")))
        .hint("broadcast")
-        .join(t4, Inner, Some(nameToAttr("t4.v-1-10") === nameToAttr("t3.v-1-100")))
+        .join(t4, Inner, Some(nameToAttr("t1.k-1-2") === nameToAttr("t4.k-1-2")))


the join condition was wrong written. t3 is not accessible here.

cloud-fan · 2020-08-07T06:10:23Z

...src/test/scala/org/apache/spark/sql/catalyst/optimizer/PullupCorrelatedPredicatesSuite.scala

    val scalarSubquery =
      testRelation
-        .where(ScalarSubquery(subPlan))
+        .where(ScalarSubquery(subPlan) === 1)


where takes a boolean expression

cloud-fan · 2020-08-07T06:11:09Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/SimplifyCastsSuite.scala

    val input = LocalRelation('a.array(ArrayType(IntegerType, true)))
-    val plan = input.select('a.cast(ArrayType(IntegerType, false)).as("casted")).analyze
+    val attr = input.output.head
+    val plan = input.select(attr.cast(ArrayType(IntegerType, false)).as("casted"))


here we are testing something that can't pass analysis, just to make sure the optimizer rule is robust.

cloud-fan · 2020-08-07T07:35:37Z

thanks for review, merging to master!

HyukjinKwon · 2020-08-07T11:05:09Z

+1 Looks good!

dongjoon-hyun · 2020-08-07T15:23:18Z

+1, late LGTM.
Can we have this nice patch in branch-3.0 too, @cloud-fan ?

… is analyzed This PR updates the `analyze` method to make sure the plan can be resolved. It also fixes some miswritten optimizer tests. It's error-prone if the `analyze` method can return an unresolved plan. no test only Closes apache#29349 from cloud-fan/test. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

… plan is analyzed ### What changes were proposed in this pull request? backport #29349 to 3.0. This PR updates the `analyze` method to make sure the plan can be resolved. It also fixes some miswritten optimizer tests. ### Why are the changes needed? It's error-prone if the `analyze` method can return an unresolved plan. ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? test only Closes #29400 from cloud-fan/backport. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

The analyze method make sure the plan is analyzed

90c75fb

probot-autolabeler bot added the SQL label Aug 4, 2020

HyukjinKwon reviewed Aug 5, 2020

View reviewed changes

maropu reviewed Aug 6, 2020

View reviewed changes

fix tests

5a71545

cloud-fan commented Aug 6, 2020

View reviewed changes

viirya reviewed Aug 7, 2020

View reviewed changes

viirya approved these changes Aug 7, 2020

View reviewed changes

cloud-fan commented Aug 7, 2020

View reviewed changes

maropu approved these changes Aug 7, 2020

View reviewed changes

cloud-fan closed this in d5682c1 Aug 7, 2020

cloud-fan mentioned this pull request Aug 10, 2020

[SPARK-32528][SQL][TEST][3.0] The analyze method should make sure the plan is analyzed #29400

Closed

		comparePlans(Optimize.execute(originalQuery.analyze), correctAnswer.analyze,
		checkAnalysis = false)

[SPARK-32528][SQL][TEST] The analyze method should make sure the plan is analyzed #29349

[SPARK-32528][SQL][TEST] The analyze method should make sure the plan is analyzed #29349

Uh oh!

Conversation

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

SparkQA commented Aug 4, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan Aug 6, 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!

SparkQA commented Aug 6, 2020

Uh oh!

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

cloud-fan commented Aug 7, 2020

Uh oh!

HyukjinKwon commented Aug 7, 2020

Uh oh!

dongjoon-hyun commented Aug 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cloud-fan commented Aug 4, 2020 •

edited

Loading

cloud-fan Aug 6, 2020 •

edited

Loading