[SPARK-34701][SQL] Remove analyzing temp view again in CreateViewCommand #31933

imback82 · 2021-03-22T19:06:04Z

What changes were proposed in this pull request?

This PR proposes to remove re-analyzing the already analyzed plan for CreateViewCommand as discussed https://github.com/apache/spark/pull/31273/files#r581592786.

Why are the changes needed?

No need to analyze the plan if it's already analyzed.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Existing tests should cover this.

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/CacheTableExec.scala

SparkQA · 2021-03-22T19:27:37Z

Test build #136359 has finished for PR 31933 at commit ff7fabb.

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

SparkQA · 2021-03-22T19:47:57Z

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40943/

SparkQA · 2021-03-23T00:53:59Z

Test build #136368 has finished for PR 31933 at commit 5829e48.

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

SparkQA · 2021-03-23T01:00:35Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40952/

SparkQA · 2021-03-23T01:05:29Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40952/

imback82 · 2021-03-23T04:11:14Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala

    allowExisting: Boolean,
    replace: Boolean,
    viewType: ViewType)
  extends RunnableCommand {

  import ViewHelper._

-  override def innerChildren: Seq[QueryPlan[_]] = Seq(child)
+  override def plansToCheckAnalysis: Seq[LogicalPlan] = Seq(analyzedPlan)


We need to run checkAnalysis on the analyzed plan, otherwise, for the following:

sql("CREATE TABLE view_base_table (key int, data varchar(20)) USING PARQUET") sql("CREATE VIEW key_dependent_view AS SELECT * FROM view_base_table GROUP BY key")

, view creation works fine, whereas it should have failed with:

org.apache.spark.sql.AnalysisException expression 'spark_catalog.default.view_base_table.data' is neither present in the group by, nor is it an aggregate function. Add to group by or wrap in first() (or first_value) if you don't care which value you get.

imback82 · 2021-03-23T04:13:42Z

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

@@ -167,6 +167,9 @@ trait CheckAnalysis extends PredicateHelper with LookupCatalog {
      case _: ShowTableExtended =>
        throw new AnalysisException("SHOW TABLE EXTENDED is not supported for v2 tables.")

+      case c: Command =>
+        c.plansToCheckAnalysis.foreach(checkAnalysis)


This seems hacky? But we cannot make the analyzed plan as children for CreateViewCommaned. The reason is that the View will be optimized away (in the optimizer), and the verification that checks if a permanent view references temp views will fail.

cloud-fan · 2021-03-23T04:45:32Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/Command.scala

@@ -28,6 +28,7 @@ trait Command extends LogicalPlan {
  override def output: Seq[Attribute] = Seq.empty
  override def producedAttributes: AttributeSet = outputSet
  override def children: Seq[LogicalPlan] = Seq.empty
+  def plansToCheckAnalysis: Seq[LogicalPlan] = Seq.empty


can we reuse innerChildren instead of adding this?

sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala

SparkQA · 2021-03-23T04:58:23Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40963/

SparkQA · 2021-03-23T05:51:28Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40963/

SparkQA · 2021-03-23T07:11:52Z

Test build #136379 has finished for PR 31933 at commit 6fdd9e0.

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

SparkQA · 2021-03-23T17:24:08Z

Test build #136418 has started for PR 31933 at commit e04fbdd.

cloud-fan · 2021-03-24T07:44:52Z

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

+
+    // Check analysis on internal nodes.
+    plan match {
+      case c: Command =>


shall we move it to L682?

SparkQA · 2021-03-24T20:24:12Z

Test build #136471 has finished for PR 31933 at commit a14b3b9.

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

SparkQA · 2021-03-24T20:31:07Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41055/

SparkQA · 2021-03-24T21:45:40Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41055/

imback82 · 2021-03-25T01:19:02Z

retest this please

SparkQA · 2021-03-25T02:22:57Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41070/

SparkQA · 2021-03-25T03:14:52Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41070/

SparkQA · 2021-03-25T03:45:44Z

Test build #136486 has finished for PR 31933 at commit a14b3b9.

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

…rom_create_temp_view

SparkQA · 2021-03-25T05:41:06Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41080/

SparkQA · 2021-03-25T05:51:10Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41080/

cloud-fan · 2021-03-25T06:53:54Z

thanks, merging to master!

SparkQA · 2021-03-25T09:03:49Z

Test build #136496 has finished for PR 31933 at commit 507b00c.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class GetViewColumnByNameAndOrdinal(
case class MultiplyYMInterval(
case class LocalLimitExec(limit: Int, child: SparkPlan) extends BaseLimitExec

cloud-fan · 2021-03-26T06:43:22Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/CacheTableExec.scala

@@ -94,14 +94,21 @@ case class CacheTableAsSelectExec(
  override lazy val relationName: String = tempViewName

  override lazy val planToCache: LogicalPlan = {
+    // CacheTableAsSelectExec.query is not resolved yet (e.g., not a child of CacheTableAsSelect)
+    // in order to skip optimizing it; note that we need to pass an analyzed plan to
+    // CreateViewCommand for the cache to work correctly. Thus, the query is analyzed below.


Can we further clean this up by creating CreateViewStatement instead of CreateViewCommand below?

cloud-fan · 2021-03-26T07:24:50Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala

-    // If the plan cannot be analyzed, throw an exception and don't proceed.
-    val qe = sparkSession.sessionState.executePlan(child)
-    qe.assertAnalyzed()
-    val analyzedPlan = qe.analyzed


Actually this is a regression. CreateViewCommand can be created from CreateViewStatement in ResolveSessionCatalog. However, ResolveSessionCatalog is inside extendedResolutionRules, which means the analyzedPlan doesn't go through postHocResolutionRules.

I think we need to make CreateViewCommand.analyzedPlan a real child, so that it can go through all the analyzer rules.

Good catch. One issue with "make CreateViewCommand.analyzedPlan a real child" is that it will become an optimized plan, which will affect the caching. I can try to make certain command's children to skip optimizer. WDYT?

It will be great to have such a mechanism to skip optimizer, useful to CacheTableAsSelect as well.

OK, I will give it a shot.

@cloud-fan I created #32032 to handle this scenario. Please let me know what you think. TIA!

cloud-fan · 2021-03-26T07:27:20Z

I've reverted it due to the regression mentioned in #31933 (comment)

…dren to be removed once the command is marked as analyzed ### What changes were proposed in this pull request? This PR proposes to introduce the `AnalysisOnlyCommand` trait such that a command that extends this trait can have its children only analyzed, but not optimized. There is a corresponding analysis rule `HandleAnalysisOnlyCommand` that marks the command as analyzed after all other analysis rules are run. This can be useful if a logical plan has children where they need to be only analyzed, but not optimized - e.g., `CREATE VIEW` or `CACHE TABLE AS`. This also addresses the issue found in #31933. This PR also updates `CreateViewCommand`, `CacheTableAsSelect`, and `AlterViewAsCommand` to use the new trait / rule such that their children are only analyzed. ### Why are the changes needed? To address the issue where the plan is unnecessarily re-analyzed in `CreateViewCommand`. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Existing tests should cover the changes. Closes #32032 from imback82/skip_transform. Authored-by: Terry Kim <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

initial commit

ff7fabb

imback82 changed the title ~~[W[SPARK-34701][SQL] Remove analyzing temp view again in CreateViewCommand~~ [WIP][SPARK-34701][SQL] Remove analyzing temp view again in CreateViewCommand Mar 22, 2021

imback82 marked this pull request as draft March 22, 2021 19:06

imback82 commented Mar 22, 2021

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/CacheTableExec.scala Show resolved Hide resolved

github-actions bot added the SQL label Mar 22, 2021

fix compilation

5829e48

imback82 changed the title ~~[WIP][SPARK-34701][SQL] Remove analyzing temp view again in CreateViewCommand~~ [SPARK-34701][SQL] Remove analyzing temp view again in CreateViewCommand Mar 22, 2021

imback82 marked this pull request as ready for review March 22, 2021 23:39

imback82 added 2 commits March 22, 2021 21:07

fix test

c0e9723

update

6fdd9e0

imback82 commented Mar 23, 2021

View reviewed changes

cloud-fan reviewed Mar 23, 2021

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala Show resolved Hide resolved

address comments

e04fbdd

cloud-fan reviewed Mar 24, 2021

View reviewed changes

cloud-fan approved these changes Mar 24, 2021

View reviewed changes

address PR comment

a14b3b9

Merge remote-tracking branch 'upstream/master' into remove_analyzed_f…

507b00c

…rom_create_temp_view

cloud-fan closed this in da04f1f Mar 25, 2021

cloud-fan reviewed Mar 26, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-34701][SQL] Remove analyzing temp view again in CreateViewCommand #31933

[SPARK-34701][SQL] Remove analyzing temp view again in CreateViewCommand #31933

imback82 commented Mar 22, 2021

SparkQA commented Mar 22, 2021

SparkQA commented Mar 22, 2021

SparkQA commented Mar 23, 2021

SparkQA commented Mar 23, 2021

SparkQA commented Mar 23, 2021

imback82 Mar 23, 2021

imback82 Mar 23, 2021

cloud-fan Mar 23, 2021 •

edited

Loading

SparkQA commented Mar 23, 2021

SparkQA commented Mar 23, 2021

SparkQA commented Mar 23, 2021

SparkQA commented Mar 23, 2021

cloud-fan Mar 24, 2021

SparkQA commented Mar 24, 2021

SparkQA commented Mar 24, 2021

SparkQA commented Mar 24, 2021

imback82 commented Mar 25, 2021

SparkQA commented Mar 25, 2021

SparkQA commented Mar 25, 2021

SparkQA commented Mar 25, 2021

SparkQA commented Mar 25, 2021

SparkQA commented Mar 25, 2021

cloud-fan commented Mar 25, 2021

SparkQA commented Mar 25, 2021

cloud-fan Mar 26, 2021

cloud-fan Mar 26, 2021

imback82 Mar 26, 2021

cloud-fan Mar 26, 2021

imback82 Mar 26, 2021

imback82 Apr 2, 2021

cloud-fan commented Mar 26, 2021

[SPARK-34701][SQL] Remove analyzing temp view again in CreateViewCommand #31933

[SPARK-34701][SQL] Remove analyzing temp view again in CreateViewCommand #31933

Conversation

imback82 commented Mar 22, 2021

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?

SparkQA commented Mar 22, 2021

SparkQA commented Mar 22, 2021

SparkQA commented Mar 23, 2021

SparkQA commented Mar 23, 2021

SparkQA commented Mar 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan Mar 23, 2021 • edited Loading

Choose a reason for hiding this comment

SparkQA commented Mar 23, 2021

SparkQA commented Mar 23, 2021

SparkQA commented Mar 23, 2021

SparkQA commented Mar 23, 2021

Choose a reason for hiding this comment

SparkQA commented Mar 24, 2021

SparkQA commented Mar 24, 2021

SparkQA commented Mar 24, 2021

imback82 commented Mar 25, 2021

SparkQA commented Mar 25, 2021

SparkQA commented Mar 25, 2021

SparkQA commented Mar 25, 2021

SparkQA commented Mar 25, 2021

SparkQA commented Mar 25, 2021

cloud-fan commented Mar 25, 2021

SparkQA commented Mar 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan commented Mar 26, 2021

cloud-fan Mar 23, 2021 •

edited

Loading