[SPARK-34076][SQL] SQLContext.dropTempTable fails if cache is non-empty #31136

sunchao · 2021-01-11T23:28:18Z

What changes were proposed in this pull request?

This changes CatalogImpl.dropTempView and CatalogImpl.dropGlobalTempView use analyzed logical plan instead of viewDef which is unresolved.

Why are the changes needed?

Currently, CatalogImpl.dropTempView is implemented as following:

override def dropTempView(viewName: String): Boolean = {
  sparkSession.sessionState.catalog.getTempView(viewName).exists { viewDef =>
    sparkSession.sharedState.cacheManager.uncacheQuery(
      sparkSession, viewDef, cascade = false)
    sessionCatalog.dropTempView(viewName)
  }
}

Here, the logical plan viewDef is not resolved, and when passing to uncacheQuery, it could fail at sameResult call, where canonicalized plan is compared. The error message looks like:

Invalid call to qualifier on unresolved object, tree: 'key

This can be reproduced via:

sql(s"CREATE TEMPORARY VIEW $v AS SELECT key FROM src LIMIT 10")
sql(s"CREATE TABLE $t AS SELECT * FROM src")
sql(s"CACHE TABLE $t")
dropTempTable(v)

Does this PR introduce any user-facing change?

The only user-facing change is that, previously SQLContext.dropTempTable may fail in the above scenario but will work with this fix.

How was this patch tested?

Added new unit tests.

SparkQA · 2021-01-12T00:14:17Z

Test build #133946 has finished for PR 31136 at commit 708adc4.

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

SparkQA · 2021-01-12T00:19:35Z

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

SparkQA · 2021-01-12T00:50:59Z

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

viirya

sql(s"CREATE TEMPORARY VIEW $v AS SELECT key FROM src LIMIT 10")
sql(s"CREATE TABLE $t AS SELECT * FROM src")
sql(s"CACHE TABLE $t")
dropTempTable(v)

Is the table $t related to the temp view $v? Why caching the table $t affects the temp view $v?

sunchao · 2021-01-12T16:43:41Z

This is just to make sure the cache is non-empty so when dropping the view comparison will be done. The issue occurs during the comparison.

viirya · 2021-01-12T17:40:00Z

sql/core/src/main/scala/org/apache/spark/sql/internal/CatalogImpl.scala

      sparkSession.sharedState.cacheManager.uncacheQuery(
-        sparkSession, viewDef, cascade = false)
+        sparkSession.table(viewName), cascade = false)


Shall we add an assert into uncacheQuery to make sure all passed in logical plans are analyzed? e.g.,

def uncacheQuery( spark: SparkSession, plan: LogicalPlan, cascade: Boolean, blocking: Boolean = false): Unit = { assert(plan.analyzed, "the plan input to `uncacheQuery` should be analyzed") ... }

I'm not totally sure about this. It seems common that a temporary view is dropped after its dependent view/table is already gone, as demonstrated by the test failures, so this assertion will fail in those cases.

Another approach is to ignore the possible non-fatal errors during query analysis and uncache, similar to what's done in DropTableCommand.

SparkQA · 2021-01-12T19:23:26Z

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

SparkQA · 2021-01-12T19:56:42Z

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

SparkQA · 2021-01-12T23:10:05Z

Test build #133980 has finished for PR 31136 at commit 712b079.

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

viirya · 2021-01-13T00:05:42Z

sql/core/src/main/scala/org/apache/spark/sql/internal/CatalogImpl.scala

+        sparkSession.sharedState.cacheManager.uncacheQuery(
+          sparkSession, plan.analyzed, cascade = false)
+      } catch {
+        case NonFatal(_) => // ignore


If there is table or view not found error, does it mean we skip uncaching the temp view?

This is a good question. I think currently this won't happen because:

when dropping a table or permanent view, we'll drop all the caches with reference on it in a cascading fashion, so when we are dropping the caches themselves, they are already invalidated.

On the other hand, we currently store temp view as analyzed logical plans so they won't be analyzed again upon retrieving, which means we won't run into the error you mentioned. Although, this also means the plans themselves could become stale and potentially generate incorrect result. [SPARK-34052][SQL] store SQL text for a temp view created using "CACHE TABLE .. AS SELECT" #31107 proposes to change this, following similar changes done by [SPARK-33142][SPARK-33647][SQL] Store SQL text for SQL temp view #30567, so the behavior of temporary view as well as cache is more aligned to that of permanent view.

HyukjinKwon · 2021-01-13T03:00:32Z

cc @cloud-fan and @MaxGekk too FYI

cloud-fan · 2021-01-13T07:51:15Z

sql/core/src/main/scala/org/apache/spark/sql/internal/CatalogImpl.scala

+        sparkSession.sharedState.cacheManager.uncacheQuery(
+          sparkSession, plan.analyzed, cascade = false)
+      } catch {
+        case NonFatal(_) => // ignore


is it safer to not add this try-catch? if there are failures, then the uncache fails and we should not swallow the exception?

This is also discussed above. Suppose we have:

CREATE TEMPORARY VIEW v1 AS SELECT * FROM v2

a Spark user can drop v2 first followed by v1. In this case the uncacheQuery will fail because v2 is already gone. Consequently, the view will not be dropped. This seems to be a quite common scenario.

Also this is following the DropTableCommand, which swallows any non-fatal exception and proceed to drop the view.

thanks for the explanation!

cloud-fan · 2021-01-13T13:22:19Z

thanks, merging to master/3.1!

### What changes were proposed in this pull request? This changes `CatalogImpl.dropTempView` and `CatalogImpl.dropGlobalTempView` use analyzed logical plan instead of `viewDef` which is unresolved. ### Why are the changes needed? Currently, `CatalogImpl.dropTempView` is implemented as following: ```scala override def dropTempView(viewName: String): Boolean = { sparkSession.sessionState.catalog.getTempView(viewName).exists { viewDef => sparkSession.sharedState.cacheManager.uncacheQuery( sparkSession, viewDef, cascade = false) sessionCatalog.dropTempView(viewName) } } ``` Here, the logical plan `viewDef` is not resolved, and when passing to `uncacheQuery`, it could fail at `sameResult` call, where canonicalized plan is compared. The error message looks like: ``` Invalid call to qualifier on unresolved object, tree: 'key ``` This can be reproduced via: ```scala sql(s"CREATE TEMPORARY VIEW $v AS SELECT key FROM src LIMIT 10") sql(s"CREATE TABLE $t AS SELECT * FROM src") sql(s"CACHE TABLE $t") dropTempTable(v) ``` ### Does this PR introduce _any_ user-facing change? The only user-facing change is that, previously `SQLContext.dropTempTable` may fail in the above scenario but will work with this fix. ### How was this patch tested? Added new unit tests. Closes #31136 from sunchao/SPARK-34076. Authored-by: Chao Sun <sunchao@apple.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 62d82b5) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

viirya · 2021-01-13T17:29:42Z

lgtm too.

github-actions bot added the SQL label Jan 11, 2021

SPARK-34076

708adc4

sunchao force-pushed the SPARK-34076 branch from 4690988 to 708adc4 Compare January 11, 2021 23:29

sunchao marked this pull request as draft January 12, 2021 01:12

viirya reviewed Jan 12, 2021

View reviewed changes

ignore table or view not found error

712b079

sunchao marked this pull request as ready for review January 12, 2021 20:33

viirya reviewed Jan 13, 2021

View reviewed changes

cloud-fan reviewed Jan 13, 2021

View reviewed changes

cloud-fan approved these changes Jan 13, 2021

View reviewed changes

cloud-fan closed this in 62d82b5 Jan 13, 2021

sunchao deleted the SPARK-34076 branch January 13, 2021 17:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-34076][SQL] SQLContext.dropTempTable fails if cache is non-empty #31136

[SPARK-34076][SQL] SQLContext.dropTempTable fails if cache is non-empty #31136

sunchao commented Jan 11, 2021

SparkQA commented Jan 12, 2021

SparkQA commented Jan 12, 2021

SparkQA commented Jan 12, 2021

viirya left a comment

sunchao commented Jan 12, 2021 •

edited

Loading

viirya Jan 12, 2021

sunchao Jan 12, 2021 •

edited

Loading

SparkQA commented Jan 12, 2021

SparkQA commented Jan 12, 2021

SparkQA commented Jan 12, 2021

viirya Jan 13, 2021

sunchao Jan 13, 2021 •

edited

Loading

HyukjinKwon commented Jan 13, 2021

cloud-fan Jan 13, 2021

sunchao Jan 13, 2021 •

edited

Loading

cloud-fan Jan 13, 2021

cloud-fan commented Jan 13, 2021

viirya commented Jan 13, 2021

[SPARK-34076][SQL] SQLContext.dropTempTable fails if cache is non-empty #31136

[SPARK-34076][SQL] SQLContext.dropTempTable fails if cache is non-empty #31136

Conversation

sunchao commented Jan 11, 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 Jan 12, 2021

SparkQA commented Jan 12, 2021

SparkQA commented Jan 12, 2021

viirya left a comment

Choose a reason for hiding this comment

sunchao commented Jan 12, 2021 • edited Loading

viirya Jan 12, 2021

Choose a reason for hiding this comment

sunchao Jan 12, 2021 • edited Loading

Choose a reason for hiding this comment

SparkQA commented Jan 12, 2021

SparkQA commented Jan 12, 2021

SparkQA commented Jan 12, 2021

viirya Jan 13, 2021

Choose a reason for hiding this comment

sunchao Jan 13, 2021 • edited Loading

Choose a reason for hiding this comment

HyukjinKwon commented Jan 13, 2021

cloud-fan Jan 13, 2021

Choose a reason for hiding this comment

sunchao Jan 13, 2021 • edited Loading

Choose a reason for hiding this comment

cloud-fan Jan 13, 2021

Choose a reason for hiding this comment

cloud-fan commented Jan 13, 2021

viirya commented Jan 13, 2021

sunchao commented Jan 12, 2021 •

edited

Loading

sunchao Jan 12, 2021 •

edited

Loading

sunchao Jan 13, 2021 •

edited

Loading

sunchao Jan 13, 2021 •

edited

Loading