From 9302be9b872e816f9470bd0ef88cb1f7c82ee4c2 Mon Sep 17 00:00:00 2001 From: Terry Kim Date: Tue, 9 Mar 2021 20:40:13 -0800 Subject: [PATCH] add more test coverage --- .../spark/sql/execution/command/views.scala | 14 +++-- .../apache/spark/sql/CachedTableSuite.scala | 52 ++++++++++++------- 2 files changed, 42 insertions(+), 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 index 7d33ea815407a..4d0e9647e9fa7 100644 --- 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 index 56ecfdc6d98fd..47b5ea8cd82a9 100644 --- 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,22 +1460,41 @@ 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") @@ -1490,7 +1502,9 @@ class CachedTableSuite extends QueryTest with SQLTestUtils 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) }