add more test coverage

apache · Mar 10, 2021 · 9302be9 · 9302be9
1 parent 31cb500
commit 9302be9
Show file tree

Hide file tree

Showing 2 changed files with 42 additions and 24 deletions.
diff --git a/sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala b/sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala
@@ -572,10 +572,10 @@ object ViewHelper extends SQLConfHelper with Logging {
       userSpecifiedColumns: Seq[(String, Option[String])],
       analyzedPlan: LogicalPlan): TemporaryViewRelation = {
     val aliasedPlan = aliasPlan(session, analyzedPlan, userSpecifiedColumns)
-    val needsToUncache = getRawTempView(name.table).map { r =>
-      produceSameResult(r, aliasedPlan)
+    val uncache = getRawTempView(name.table).map { r =>
+      needsToUncache(r, aliasedPlan)
     }.getOrElse(false)
-    if (replace && needsToUncache) {
+    if (replace && uncache) {
       logInfo(s"Try to uncache ${name.quotedString} before replacing.")
       checkCyclicViewReference(analyzedPlan, Seq(name), name)
       CommandUtils.uncacheTableOrView(session, name.quotedString)
@@ -617,11 +617,15 @@ object ViewHelper extends SQLConfHelper with Logging {
   }
 
   /**
-   * Checks if the raw temp view will return the same result as the given aliased plan.
+   * Checks if need to uncache the temp view being replaced.
    */
-  private def produceSameResult(
+  private def needsToUncache(
       rawTempView: LogicalPlan,
       aliasedPlan: LogicalPlan): Boolean = rawTempView match {
+    // If TemporaryViewRelation doesn't store the analyzed view, always uncache.
+    case TemporaryViewRelation(_, None) => true
+    // 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)
   }

diff --git a/sql/core/src/test/scala/org/apache/spark/sql/CachedTableSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/CachedTableSuite.scala
@@ -1450,14 +1450,7 @@ class CachedTableSuite extends QueryTest with SQLTestUtils
     Seq(true, false).foreach { storeAnalyzed =>
       withSQLConf(SQLConf.STORE_ANALYZED_PLAN_FOR_VIEW.key -> storeAnalyzed.toString) {
         withTempView("tv") {
-          sql("CREATE TEMPORARY VIEW tv AS SELECT 1")
-          sql("CACHE TABLE tv")
-          assert(spark.catalog.isCached("tv"))
-          assert(spark.sharedState.cacheManager.lookupCachedData(sql("SELECT 1")).nonEmpty)
-
-          sql("ALTER VIEW tv as SELECT 2")
-          assert(!spark.catalog.isCached("tv"))
-          assert(spark.sharedState.cacheManager.lookupCachedData(sql("SELECT 1")).isEmpty)
+          testAlterTemporaryViewAsWithCache(TableIdentifier("tv"), storeAnalyzed)
         }
       }
     }
@@ -1467,30 +1460,51 @@ class CachedTableSuite extends QueryTest with SQLTestUtils
     Seq(true, false).foreach { storeAnalyzed =>
       withSQLConf(SQLConf.STORE_ANALYZED_PLAN_FOR_VIEW.key -> storeAnalyzed.toString) {
         withGlobalTempView("global_tv") {
-          sql("CREATE GLOBAL TEMPORARY VIEW global_tv AS SELECT 1")
-
           val db = spark.sharedState.globalTempViewManager.database
-          val gv = s"$db.global_tv"
-          sql(s"CACHE TABLE $gv")
-          assert(spark.catalog.isCached(gv))
-          assert(spark.sharedState.cacheManager.lookupCachedData(sql("SELECT 1")).nonEmpty)
-
-          sql(s"ALTER VIEW $gv as SELECT 2")
-          assert(!spark.catalog.isCached(gv))
-          assert(spark.sharedState.cacheManager.lookupCachedData(sql("SELECT 1")).isEmpty)
+          testAlterTemporaryViewAsWithCache(TableIdentifier("global_tv", Some(db)), storeAnalyzed)
         }
       }
     }
   }
 
+  private def testAlterTemporaryViewAsWithCache(
+      ident: TableIdentifier,
+      storeAnalyzed: Boolean): Unit = {
+    val (tempViewStr, viewName) = if (ident.database.nonEmpty) {
+      ("GLOBAL TEMPORARY", s"${ident.database.get}.${ident.table}")
+    } else {
+      ("TEMPORARY", ident.table)
+    }
+
+    sql(s"CREATE $tempViewStr VIEW ${ident.table} AS SELECT 1")
+
+    sql(s"CACHE TABLE $viewName")
+    assert(spark.catalog.isCached(viewName))
+    assert(spark.sharedState.cacheManager.lookupCachedData(sql("SELECT 1")).nonEmpty)
+
+    if (storeAnalyzed) {
+      // Altered temporary view will have the same plan, thus it will not be uncached.
+      // Note that this check is done only if a temporary view stores an analyzed view.
+      sql(s"ALTER VIEW $viewName as SELECT 1")
+      assert(spark.catalog.isCached(viewName))
+      assert(spark.sharedState.cacheManager.lookupCachedData(sql("SELECT 1")).nonEmpty)
+    }
+
+    sql(s"ALTER VIEW $viewName as SELECT 2")
+    assert(!spark.catalog.isCached(viewName))
+    assert(spark.sharedState.cacheManager.lookupCachedData(sql("SELECT 1")).isEmpty)
+  }
+
   test("SPARK-34546: ALTER VIEW AS should uncache if a permanent temp view is cached") {
     withView("view") {
       sql("CREATE VIEW view AS SELECT 1")
       sql("CACHE TABLE view")
       assert(spark.catalog.isCached("view"))
       assert(spark.sharedState.cacheManager.lookupCachedData(sql("SELECT 1")).nonEmpty)
 
-      sql("ALTER VIEW view as SELECT 2")
+      // ALTER VIEW AS on a permanent view should uncache even if the replacing view produces
+      // the same result.
+      sql("ALTER VIEW view as SELECT 1")
       assert(!spark.catalog.isCached("view"))
       assert(spark.sharedState.cacheManager.lookupCachedData(sql("SELECT 1")).isEmpty)
     }