[SPARK-34546][SQL] AlterViewAs.query should be analyzed during the analysis phase, and AlterViewAs should invalidate the cache #31652

imback82 · 2021-02-25T18:21:23Z

What changes were proposed in this pull request?

This PR proposes the following:

AlterViewAs.query is currently analyzed in the physical operator AlterViewAsCommand, but it should be analyzed during the analysis phase.
When spark.sql.legacy.storeAnalyzedPlanForView is set to true, store TermporaryViewRelation which wraps the analyzed plan, similar to [SPARK-34152][SQL] Make CreateViewStatement.child to be LogicalPlan's children so that it's resolved in analyze phase #31273.
Try to uncache the view you are altering.

Why are the changes needed?

Analyzing a plan should be done in the analysis phase if possible.

Not uncaching the view (existing behavior) seems like a bug since the cache may not be used again.

Does this PR introduce any user-facing change?

Yes, now the view can be uncached if it's already cached.

How was this patch tested?

Added new tests around uncaching.

The existing tests such as SQLViewSuite should cover the analysis changes.

SparkQA · 2021-02-25T19:26:01Z

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

SparkQA · 2021-02-25T19:55:50Z

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

SparkQA · 2021-02-25T22:50:38Z

Test build #135475 has finished for PR 31652 at commit ba766f7.

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

This reverts commit eeb3712.

SparkQA · 2021-03-07T06:08:06Z

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

SparkQA · 2021-03-07T06:35:45Z

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

imback82 · 2021-03-08T00:28:16Z

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

@@ -879,7 +879,7 @@ class Analyzer(override val catalogManager: CatalogManager)

  private def unwrapRelationPlan(plan: LogicalPlan): LogicalPlan = {
    EliminateSubqueryAliases(plan) match {
-      case v: View if v.isDataFrameTempView => v.child
+      case v: View if v.isTempViewStoringAnalyzedPlan => v.child


I renamed because this type of view can also be created not from a dataframe - e.g., ALTER VIEW ... AS with spark.sql.legacy.storeAnalyzedPlanForView set to true.

imback82 · 2021-03-08T00:29:44Z

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 = session.sessionState.executePlan(query)
-    qe.assertAnalyzed()
-    val analyzedPlan = qe.analyzed


query will be an analyzed plan since AlterViewAs is converted to AlterViewAsCommand in Analyzer phase (ResolveSessionCatalog).

shall we remove the similar code in CreateViewCommand?

We cannot yet because of the following:

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

Lines 96 to 105 in f340857

override lazy val planToCache: LogicalPlan = {

Dataset.ofRows(sparkSession,

CreateViewCommand(

name = TableIdentifier(tempViewName),

userSpecifiedColumns = Nil,

comment = None,

properties = Map.empty,

originalText = Some(originalText),

child = query,

allowExisting = false,

Here, query is not resolved yet. I will get to this though.

imback82 · 2021-03-08T00:31:34Z

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

@@ -116,7 +116,7 @@ case class CreateViewCommand(
    if (viewType == LocalTempView) {
      val aliasedPlan = aliasPlan(sparkSession, analyzedPlan)
      if (replace && needsToUncache(catalog.getRawTempView(name.table), aliasedPlan)) {
-        logInfo(s"Try to uncache ${name.quotedString} before replacing.")
+        logDebug(s"Try to uncache ${name.quotedString} before replacing.")


I changed this to logDebug because the permanent view was using it:

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

Line 174 in 9ec8696

logDebug(s"Try to uncache ${viewIdent.quotedString} before replacing.")

If logInfo is better, I can change these to logInfo instead.

imback82 · 2021-03-08T00:32:40Z

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

+    checkCyclicViewReference(analyzedPlan, Seq(name), name)
+
+    logDebug(s"Try to uncache ${name.quotedString} before altering.")
+    CommandUtils.uncacheTableOrView(session, name.quotedString)


@cloud-fan Uncaching seems like a right thing to do when you alter a view?

yea I think so, since the cached data is stale. It's similar to CREATE OR REPLACE TEMP VIEW, seems we have an extra check needsToUncache. Shall we do it here as well?

Since the logic is very similar, I refactored this part to share code with CreateViewCommand.

SparkQA · 2021-03-08T03:07:26Z

Test build #135849 has finished for PR 31652 at commit 51c66e8.

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

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

imback82 · 2021-03-10T03:41:29Z

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

+    checkCyclicViewReference(analyzedPlan, Seq(name), name)
+
+    logDebug(s"Try to uncache ${name.quotedString} before altering.")
+    CommandUtils.uncacheTableOrView(session, name.quotedString)


Since the logic is very similar, I refactored this part to share code with CreateViewCommand.

imback82 · 2021-03-10T04:41:32Z

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

+      rawTempView: LogicalPlan,
+      aliasedPlan: LogicalPlan): Boolean = rawTempView match {
+    // If TemporaryViewRelation doesn't store the analyzed view, always uncache.
+    case TemporaryViewRelation(_, None) => true


This is unnecessary because case p => !p.sameResult(aliasedPlan) should cover, but I added this case explicitly to make the intention clear.

cloud-fan · 2021-03-10T05:38:34Z

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

+      needsToUncache(r, aliasedPlan)
+    }.getOrElse(false)
+    if (replace && uncache) {
+      logInfo(s"Try to uncache ${name.quotedString} before replacing.")


cloud-fan · 2021-03-10T05:46:16Z

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

+   * If `userSpecifiedColumns` is defined, alias the analyzed plan to the user specified columns,
+   * else return the analyzed plan directly.
+   */
+  def aliasPlan(


shall we still keep this in CreateViewCommand and let it pass the aliasedPlan to createTemporaryViewRelation? This logic is not needed for AlterViewAsCommand.

cloud-fan · 2021-03-10T05:47:58Z

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

+    // Do not need to uncache if the to-be-replaced temp view plan and the new plan are the
+    // same-result plans.
+    case TemporaryViewRelation(_, Some(p)) => !p.sameResult(aliasedPlan)
+    case p => !p.sameResult(aliasedPlan)


Do we still need it? rawTempView should always be TemporaryViewRelation right?

We need to first update CREATE TEMP VIEW USING:

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

Lines 93 to 100 in 3a299aa

val viewDefinition = Dataset.ofRows(

sparkSession, LogicalRelation(dataSource.resolveRelation())).logicalPlan

if (global) {

catalog.createGlobalTempView(tableIdent.table, viewDefinition, replace)

} else {

catalog.createTempView(tableIdent.table, viewDefinition, replace)

}

, which I will get to right after this PR.

Once the PR is done, we can update createTempView to take in TemporaryViewRelation (https://github.com/apache/spark/pull/31273/files#r580757641), then I can safely remove this line.

Let me actually create a JIRA to capture these as subtasks.

I created https://issues.apache.org/jira/browse/SPARK-34698 to track these.

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

SparkQA · 2021-03-10T10:00:43Z

Test build #135921 has finished for PR 31652 at commit 9302be9.

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

SparkQA · 2021-03-10T22:36:49Z

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

SparkQA · 2021-03-10T23:07:24Z

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

SparkQA · 2021-03-11T02:08:26Z

Test build #135946 has finished for PR 31652 at commit 7e4815a.

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

cloud-fan · 2021-03-11T05:31:39Z

thanks, merging to master!

…d take/return more concrete types ### What changes were proposed in this pull request? Now that all the temporary views are wrapped with `TemporaryViewRelation`(#31273, #31652, and #31825), this PR proposes to update `SessionCatalog`'s APIs for temporary views to take or return more concrete types. APIs that will take `TemporaryViewRelation` instead of `LogicalPlan`: ``` createTempView, createGlobalTempView, alterTempViewDefinition ``` APIs that will return `TemporaryViewRelation` instead of `LogicalPlan`: ``` getRawTempView, getRawGlobalTempView ``` APIs that will return `View` instead of `LogicalPlan`: ``` getTempView, getGlobalTempView, lookupTempView ``` ### Why are the changes needed? Internal refactoring to work with more concrete types. ### Does this PR introduce _any_ user-facing change? No, this is internal refactoring. ### How was this patch tested? Updated existing tests affected by the refactoring. Closes #31906 from imback82/use_temporary_view_relation. Authored-by: Terry Kim <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

initial commit

ba766f7

github-actions bot added the SQL label Feb 25, 2021

imback82 changed the title ~~[SPARK-34546][SQL] AlterViewAs.query should be analyzed during the analysis phase~~ [WIP][SPARK-34546][SQL] AlterViewAs.query should be analyzed during the analysis phase Feb 25, 2021

imback82 marked this pull request as draft February 25, 2021 19:12

imback82 added 4 commits March 6, 2021 11:04

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

12af3a7

refactoring

eeb3712

Revert "refactoring"

2b2d35e

This reverts commit eeb3712.

uncache

ba42775

Add tests related to uncaching

51c66e8

imback82 changed the title ~~[WIP][SPARK-34546][SQL] AlterViewAs.query should be analyzed during the analysis phase~~ [SPARK-34546][SQL] AlterViewAs.query should be analyzed during the analysis phase, and AlterView should invalidate the cache Mar 8, 2021

imback82 marked this pull request as ready for review March 8, 2021 00:25

imback82 commented Mar 8, 2021

View reviewed changes

imback82 changed the title ~~[SPARK-34546][SQL] AlterViewAs.query should be analyzed during the analysis phase, and AlterView should invalidate the cache~~ [SPARK-34546][SQL] AlterViewAs.query should be analyzed during the analysis phase, and AlterViewAs should invalidate the cache Mar 8, 2021

cloud-fan reviewed Mar 8, 2021

View reviewed changes

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

imback82 added 2 commits March 9, 2021 16:13

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

8c8b0ef

refactor

31cb500

imback82 commented Mar 10, 2021

View reviewed changes

add more test coverage

9302be9

imback82 commented Mar 10, 2021

View reviewed changes

cloud-fan reviewed Mar 10, 2021

View reviewed changes

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

imback82 added 2 commits March 10, 2021 10:27

address PR comments

e86d5e3

clean up

7e4815a

cloud-fan approved these changes Mar 11, 2021

View reviewed changes

cloud-fan closed this in 2a6e68e Mar 11, 2021

imback82 mentioned this pull request Mar 19, 2021

[SPARK-34700][SQL] SessionCatalog's temporary view related APIs should take/return more concrete types #31906

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-34546][SQL] AlterViewAs.query should be analyzed during the analysis phase, and AlterViewAs should invalidate the cache #31652

[SPARK-34546][SQL] AlterViewAs.query should be analyzed during the analysis phase, and AlterViewAs should invalidate the cache #31652

imback82 commented Feb 25, 2021 •

edited

Loading

SparkQA commented Feb 25, 2021

SparkQA commented Feb 25, 2021

SparkQA commented Feb 25, 2021

SparkQA commented Mar 7, 2021

SparkQA commented Mar 7, 2021

imback82 Mar 8, 2021

imback82 Mar 8, 2021

cloud-fan Mar 8, 2021

imback82 Mar 8, 2021

imback82 Mar 8, 2021

imback82 Mar 8, 2021

cloud-fan Mar 8, 2021

imback82 Mar 10, 2021

SparkQA commented Mar 8, 2021

imback82 Mar 10, 2021

imback82 Mar 10, 2021

cloud-fan Mar 10, 2021

cloud-fan Mar 10, 2021

cloud-fan Mar 10, 2021

imback82 Mar 10, 2021

imback82 Mar 10, 2021

SparkQA commented Mar 10, 2021

SparkQA commented Mar 10, 2021

SparkQA commented Mar 10, 2021

SparkQA commented Mar 11, 2021

cloud-fan commented Mar 11, 2021

	override lazy val planToCache: LogicalPlan = {
	Dataset.ofRows(sparkSession,
	CreateViewCommand(
	name = TableIdentifier(tempViewName),
	userSpecifiedColumns = Nil,
	comment = None,
	properties = Map.empty,
	originalText = Some(originalText),
	child = query,
	allowExisting = false,

	val viewDefinition = Dataset.ofRows(
	sparkSession, LogicalRelation(dataSource.resolveRelation())).logicalPlan

	if (global) {
	catalog.createGlobalTempView(tableIdent.table, viewDefinition, replace)
	} else {
	catalog.createTempView(tableIdent.table, viewDefinition, replace)
	}

[SPARK-34546][SQL] AlterViewAs.query should be analyzed during the analysis phase, and AlterViewAs should invalidate the cache #31652

[SPARK-34546][SQL] AlterViewAs.query should be analyzed during the analysis phase, and AlterViewAs should invalidate the cache #31652

Conversation

imback82 commented Feb 25, 2021 • 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?

SparkQA commented Feb 25, 2021

SparkQA commented Feb 25, 2021

SparkQA commented Feb 25, 2021

SparkQA commented Mar 7, 2021

SparkQA commented Mar 7, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Mar 8, 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

Choose a reason for hiding this comment

SparkQA commented Mar 10, 2021

SparkQA commented Mar 10, 2021

SparkQA commented Mar 10, 2021

SparkQA commented Mar 11, 2021

cloud-fan commented Mar 11, 2021

imback82 commented Feb 25, 2021 •

edited

Loading