[SPARK-18465] Add 'IF EXISTS' clause to 'UNCACHE' to not throw exceptions when table doesn't exist #15896

brkyvz · 2016-11-16T02:16:17Z

What changes were proposed in this pull request?

While this behavior is debatable, consider the following use case:

UNCACHE TABLE foo;
CACHE TABLE foo AS 
SELECT * FROM bar

The command above fails the first time you run it. But I want to run the command above over and over again, and I don't want to change my code just for the first run of it.
The issue is that subsequent CACHE TABLE commands do not overwrite the existing table.

Now we can do:

UNCACHE TABLE IF EXISTS foo;
CACHE TABLE foo AS 
SELECT * FROM bar

How was this patch tested?

Unit tests

SparkQA · 2016-11-16T02:19:26Z

Test build #68684 has finished for PR 15896 at commit b8a4791.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

brkyvz · 2016-11-16T02:19:37Z

cc @gatorsmile I changed a test you added. Do you have any strong feelings on this?

SparkQA · 2016-11-16T02:24:27Z

Test build #68685 has finished for PR 15896 at commit 8c11e90.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2016-11-16T04:23:56Z

After the style is fixed, LGTM pending testing.

SparkQA · 2016-11-16T16:44:42Z

Test build #68725 has finished for PR 15896 at commit 27acd46.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-11-16T18:55:59Z

Test build #68726 has finished for PR 15896 at commit 98b22d5.

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

SparkQA · 2016-11-16T21:39:45Z

Test build #68730 has finished for PR 15896 at commit 15f3e59.

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

gatorsmile · 2016-11-16T22:09:19Z

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

+    try {
+      sparkSession.sharedState.cacheManager.uncacheQuery(query = sparkSession.table(tableName))
+    } catch {
+      case _: NoSuchTableException => // do nothing


In the PR #15682, we identified an issue when we try to uncache a view that containing a table that has been dropped. We are not issuing NoSuchTableException. Instead. Instead, we issue an AnalysisException. That means, this might not cover all the senario. Do you want to cover that case in this PR?

brkyvz · 2016-11-16T22:51:27Z

@gatorsmile Talking offline with several people, I may put this PR on hold for now since it is a behavior change. I guess it would be better to go with Options 1 or 2 that I defined in the PR description.

gatorsmile · 2016-11-16T22:58:22Z

@brkyvz Why not supporting both? : )

Each has its own usage scenario.

dongjoon-hyun · 2016-11-17T23:30:55Z

Hi, @brkyvz and @gatorsmile .
Does this proceed to Option 1 or 2 for now? Or is this holding on for next month?

brkyvz · 2016-11-17T23:40:45Z

On hold on my side. Will try to get back to it

On Nov 17, 2016 3:31 PM, "Dongjoon Hyun" [email protected] wrote:

Hi, @brkyvz https://github.com/brkyvz and @gatorsmile
https://github.com/gatorsmile .
Does this proceed to Option 1 or 2 for now? Or is this holding on for next
month?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#15896 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AFACe6aqzw1YO26KRXEZU0LG-tQLaAB7ks5q_OPHgaJpZM4KzRIL
.

dongjoon-hyun · 2016-11-18T01:25:50Z

I see. Thank you for informing that, @brkyvz

andrewor14 · 2016-11-21T21:19:44Z

I personally think UNCACHE TABLE IF EXISTS is best. It preserves the old behavior but lets the user make sure a table is not cached if they really want.

brkyvz · 2016-11-22T18:16:28Z

Hey @andrewor14 ! I went with your way. @hvanhovell can you take a quick look please? I would really like this to be available in Spark 2.1 (even though it is a new API)

hvanhovell · 2016-11-22T19:09:57Z

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

+      sparkSession.catalog.uncacheTable(tableId)
+    } catch {
+      case _: NoSuchTableException if ifExists => // don't throw
+        logInfo(s"Asked to uncache table $tableId which doesn't exist.")


Minor: Lets not log it, logs typically get swallowed by the environment anyway. See for example: https://github.com/apache/spark/blob/master/sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala#L206

hvanhovell

One small comment, otherwise LGTM

brkyvz · 2016-11-22T19:20:45Z

@hvanhovell Done! Thanks for the quick review!

hvanhovell · 2016-11-22T19:20:52Z

LGTM pending jenkins

gatorsmile · 2016-11-22T19:30:13Z

When the target is a view, it becomes a little bit more complex. If this PR does not handle it, we can do it in a separate PR.

UNCACHE TABLE IF EXISTS viewName;

hvanhovell · 2016-11-22T19:33:32Z

@gatorsmile could you elaborate a little? What am I missing?

SparkQA · 2016-11-22T20:53:13Z

Test build #69013 has finished for PR 15896 at commit 0307eb8.

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

SparkQA · 2016-11-22T20:56:45Z

Test build #69014 has finished for PR 15896 at commit 266b902.

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

hvanhovell · 2016-11-22T21:03:26Z

Merging to master/2.1. Thanks!

…ions when table doesn't exist ## What changes were proposed in this pull request? While this behavior is debatable, consider the following use case: ```sql UNCACHE TABLE foo; CACHE TABLE foo AS SELECT * FROM bar ``` The command above fails the first time you run it. But I want to run the command above over and over again, and I don't want to change my code just for the first run of it. The issue is that subsequent `CACHE TABLE` commands do not overwrite the existing table. Now we can do: ```sql UNCACHE TABLE IF EXISTS foo; CACHE TABLE foo AS SELECT * FROM bar ``` ## How was this patch tested? Unit tests Author: Burak Yavuz <[email protected]> Closes #15896 from brkyvz/uncache. (cherry picked from commit bdc8153) Signed-off-by: Herman van Hovell <[email protected]>

SparkQA · 2016-11-22T21:19:57Z

Test build #69017 has finished for PR 15896 at commit 4d62ce4.

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

SparkQA · 2016-11-22T22:20:33Z

Test build #69021 has finished for PR 15896 at commit 5432a83.

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

gatorsmile · 2016-11-22T22:59:53Z

      spark.range(1, 10).toDF("id1").write.format("json").saveAsTable("jt1")
      spark.range(1, 10).toDF("id2").write.format("json").saveAsTable("jt2")
      sql("CREATE VIEW testView AS SELECT * FROM jt1 JOIN jt2 ON id1 == id2")
      // Cache is empty at the beginning
      assert(spark.sharedState.cacheManager.isEmpty)
      sql("CACHE TABLE testView")
      assert(spark.catalog.isCached("testView"))
      // Cache is not empty
      assert(!spark.sharedState.cacheManager.isEmpty)
      // drop a table referenced by a cached view
      sql("DROP TABLE jt1")

-- So far everything is fine

      // Failed to unache the view
      val e = intercept[AnalysisException] {
        sql("UNCACHE TABLE testView")
      }.getMessage
      assert(e.contains("Table or view not found: `default`.`jt1`"))

      // We are unable to drop it from the cache
      assert(!spark.sharedState.cacheManager.isEmpty)

@hvanhovell Above is the example.

hvanhovell · 2016-11-22T23:04:35Z

Thanks! Isn't this an existing (separate) problem, that we should fix?

gatorsmile · 2016-11-22T23:06:41Z

Yeah! That is just a relavant problem. Found it when I reviewed another PR: #15682

hvanhovell · 2016-11-22T23:11:54Z

I agree that it is relevant, and that we should fix it for 2.1.

…ions when table doesn't exist ## What changes were proposed in this pull request? While this behavior is debatable, consider the following use case: ```sql UNCACHE TABLE foo; CACHE TABLE foo AS SELECT * FROM bar ``` The command above fails the first time you run it. But I want to run the command above over and over again, and I don't want to change my code just for the first run of it. The issue is that subsequent `CACHE TABLE` commands do not overwrite the existing table. Now we can do: ```sql UNCACHE TABLE IF EXISTS foo; CACHE TABLE foo AS SELECT * FROM bar ``` ## How was this patch tested? Unit tests Author: Burak Yavuz <[email protected]> Closes apache#15896 from brkyvz/uncache.

brkyvz added 3 commits November 15, 2016 18:08

Added test

b8a4791

make test better

f62bc2b

made change

8c11e90

Update CatalogImpl.scala

27acd46

Update CatalogSuite.scala

98b22d5

Update tests.py

15f3e59

dongjoon-hyun mentioned this pull request Nov 16, 2016

[SPARK-18169][SQL] Suppress warnings when dropping views on a dropped table #15682

Closed

gatorsmile reviewed Nov 16, 2016

View reviewed changes

brkyvz added 3 commits November 22, 2016 13:10

added if exists clause to uncache table

0c85fc0

Merge branch 'uncache' of github.com:brkyvz/spark into uncache

0307eb8

re-add pyspark test

266b902

brkyvz changed the title ~~[SPARK-18465] Uncache table shouldn't throw an exception when table doesn't exist~~ [SPARK-18465] Add 'IF EXISTS' clause to 'UNCACHE' to not throw exceptions when table doesn't exist Nov 22, 2016

remove unnecessary import

4d62ce4

hvanhovell reviewed Nov 22, 2016

View reviewed changes

hvanhovell requested changes Nov 22, 2016

View reviewed changes

don't log

5432a83

asfgit closed this in bdc8153 Nov 22, 2016

brkyvz deleted the uncache branch February 3, 2019 20:58

[SPARK-18465] Add 'IF EXISTS' clause to 'UNCACHE' to not throw exceptions when table doesn't exist #15896

[SPARK-18465] Add 'IF EXISTS' clause to 'UNCACHE' to not throw exceptions when table doesn't exist #15896

Uh oh!

Conversation

brkyvz commented Nov 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Nov 16, 2016

Uh oh!

brkyvz commented Nov 16, 2016

Uh oh!

SparkQA commented Nov 16, 2016

Uh oh!

gatorsmile commented Nov 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Nov 16, 2016

Uh oh!

SparkQA commented Nov 16, 2016

Uh oh!

SparkQA commented Nov 16, 2016

Uh oh!

gatorsmile Nov 16, 2016

Choose a reason for hiding this comment

Uh oh!

brkyvz commented Nov 16, 2016

Uh oh!

gatorsmile commented Nov 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongjoon-hyun commented Nov 17, 2016

Uh oh!

brkyvz commented Nov 17, 2016

Uh oh!

dongjoon-hyun commented Nov 18, 2016

Uh oh!

andrewor14 commented Nov 21, 2016

Uh oh!

brkyvz commented Nov 22, 2016

Uh oh!

hvanhovell Nov 22, 2016

Choose a reason for hiding this comment

Uh oh!

hvanhovell left a comment

Choose a reason for hiding this comment

Uh oh!

brkyvz commented Nov 22, 2016

Uh oh!

hvanhovell commented Nov 22, 2016

Uh oh!

gatorsmile commented Nov 22, 2016

Uh oh!

hvanhovell commented Nov 22, 2016

Uh oh!

SparkQA commented Nov 22, 2016

Uh oh!

SparkQA commented Nov 22, 2016

Uh oh!

hvanhovell commented Nov 22, 2016

Uh oh!

SparkQA commented Nov 22, 2016

Uh oh!

SparkQA commented Nov 22, 2016

Uh oh!

gatorsmile commented Nov 22, 2016

Uh oh!

hvanhovell commented Nov 22, 2016

Uh oh!

gatorsmile commented Nov 22, 2016

Uh oh!

hvanhovell commented Nov 22, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

brkyvz commented Nov 16, 2016 •

edited

Loading

gatorsmile commented Nov 16, 2016 •

edited

Loading

gatorsmile commented Nov 16, 2016 •

edited

Loading