[SPARK-15864] [SQL] Fix Inconsistent Behaviors when Uncaching Non-cached Tables

[gatorsmile]

What changes were proposed in this pull request?

To uncache a table, we have three different ways:

• SQL interface: UNCACHE TABLE
• DataSet API: sparkSession.catalog.uncacheTable
• DataSet API: sparkSession.table(tableName).unpersist()

When the table is not cached,

• SQL interface: UNCACHE TABLE non-cachedTable -> no error message
• Dataset API: sparkSession.catalog.uncacheTable("non-cachedTable") -> report a strange error message:
  requirement failed: Table [a: int] is not cached
• Dataset API: sparkSession.table("non-cachedTable").unpersist() -> no error message

This PR will make them consistent. No operation if the table has already been uncached.

In addition, this PR also removes uncacheQuery and renames tryUncacheQuery to uncacheQuery, and documents it that it's noop if the table has already been uncached

How was this patch tested?

Improved the existing test case for verifying the cases when the table has not been cached.
Also added test cases for verifying the cases when the table does not exist

[SparkQA]

Test build #60278 has finished for PR 13593 at commit f4b334e.

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

[gatorsmile]

cc @rxin @hvanhovell @liancheng This is another issue related to Cache and Uncache. Actually, I am not sure if we should provide a SQL interface UNCACHE TABLE IF EXISTS.

Thanks!

[liancheng]

From the perspective of better consistency, I'd prefer the current fix in the PR plus a new UNCACHE TABLE ... IF EXISTS syntax. However, this PR does introduce behavior breaking change.

@rxin What do you think?

(BTW, it's "Dataset" rather than "DataSet".)

[liancheng]

We do have a private method tryUncacheQuery in CacheManager, which exposes exactly the same semantics as UNCACHE TABLE ... IF EXISTS. But Catalog doesn't have an equivalent public method.

[rxin]

Hmmm maybe just throw an exception if the table does not exist, but no-op if the table is already uncached. I don't think we should change the behavior here.

[gatorsmile]

@rxin @liancheng I see. Since the existing Dataset API sparkSession.catalog.uncacheTable("non-cachedTable") issues an error if uncaching non-cached tables. Thus, to ensure both SQL statements and Dataset APIs have the same behavior. We still need to change one of them, right?

Will follow what @rxin said. No-op if the table is already uncached.

[gatorsmile]

@liancheng tryUncacheQuery and uncacheQuery are different. tryUncacheQuery does not unregister the accumulators, but uncacheQuery does it. This difference is also confusing.

uncacheQuery:

spark/sql/core/src/main/scala/org/apache/spark/sql/execution/CacheManager.scala

Line 112 in 468da03

cachedData(dataIndex).cachedRepresentation.uncache(blocking)

tryUncacheQuery:

spark/sql/core/src/main/scala/org/apache/spark/sql/execution/CacheManager.scala

Line 127 in 468da03

cachedData(dataIndex).cachedRepresentation.cachedColumnBuffers.unpersist(blocking)

I plan to unregister the accumulators in both APIs. Does that make sense? Thanks!

[gatorsmile]

After this change, nobody is calling sparkSession.sharedState.cacheManager.uncacheQuery. Should we remove this API?

[cloud-fan]

yea, let's remove it and rename tryUncacheQuery to uncacheQuery, and document it that it's a noop if table is already uncached

[gatorsmile]

Sure, will do it. Thanks!

[SparkQA]

Test build #60340 has finished for PR 13593 at commit 17a0885.

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

[cloud-fan]

hi @gatorsmile , do you wanna update it? now both tryUncacheQuery and uncacheQuery won't unregister accumulator

[gatorsmile]

@cloud-fan Thank you for letting me know it. Just updated the PR. Please let me know if anything needs a change. Thanks!

[SparkQA]

Test build #60467 has finished for PR 13593 at commit ef3572c.

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

[SparkQA]

Test build #60474 has finished for PR 13593 at commit 6999bd2.

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

[cloud-fan]

LGTM, can you update the PR description?

cc @rxin for final sing-off as this is kind of a behaviour change.

[rxin]

Behavior change LGTM.

@gatorsmile can you update the pr description? It is outdated.

[gatorsmile]

@cloud-fan @rxin @liancheng Thank you for your reviews!

The PR description is updated. Let me know if any change is needed. Thanks!

[cloud-fan]

Dataset API: sparkSession.catalog.uncacheTable("non-cachedTable") -> report a strange error message: requirement failed: Table [a: int] is not cached
Dataset API: sparkSession.catalog.uncacheTable("non-cachedTable") -> no error message

typo?

[gatorsmile]

@cloud-fan Sorry, please refresh the browser. Just finished the changes.

[cloud-fan]

thanks, merging to master/2.0!