[GitHub] [spark] imback82 commented on a change in pull request #31652: [SPARK-34546][SQL] AlterViewAs.query should be analyzed during the analysis phase, and AlterView should invalidate the cache

[GitBox <gi...@apache.org>]

imback82 commented on a change in pull request #31652:
URL: https://github.com/apache/spark/pull/31652#discussion_r589113144



##########
File path: 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

Review comment:
       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.

##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala
##########
@@ -268,30 +268,32 @@ case class AlterViewAsCommand(
     name: TableIdentifier,
     originalText: String,
     query: LogicalPlan) extends RunnableCommand {
+  require(query.resolved)
 
   import ViewHelper._
 
   override def innerChildren: Seq[QueryPlan[_]] = Seq(query)
 
   override def run(session: SparkSession): Seq[Row] = {
-    // 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

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

##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala
##########
@@ -268,30 +268,32 @@ case class AlterViewAsCommand(
     name: TableIdentifier,
     originalText: String,
     query: LogicalPlan) extends RunnableCommand {
+  require(query.resolved)
 
   import ViewHelper._
 
   override def innerChildren: Seq[QueryPlan[_]] = Seq(query)
 
   override def run(session: SparkSession): Seq[Row] = {
-    // 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
-
     if (session.sessionState.catalog.isTempView(name)) {
-      alterTemporaryView(session, analyzedPlan)
+      alterTemporaryView(session, query)
     } else {
-      alterPermanentView(session, analyzedPlan)
+      alterPermanentView(session, query)
     }
     Seq.empty[Row]
   }
 
   private def alterTemporaryView(session: SparkSession, analyzedPlan: LogicalPlan): Unit = {
+    checkCyclicViewReference(analyzedPlan, Seq(name), name)
+
+    logDebug(s"Try to uncache ${name.quotedString} before altering.")
+    CommandUtils.uncacheTableOrView(session, name.quotedString)

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

##########
File path: 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.")

Review comment:
       I changed this to `logDebug` because the permanent view was using it: https://github.com/apache/spark/blob/9ec8696f117a5c43d24aae3188f07ddd91b69d2d/sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala#L174
   
   If `logInfo` is better, I can change these to `logInfo` instead.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org