[SPARK-24669][SQL] Invalidate tables in case of DROP DATABASE CASCADE #23905

Udbhav30 · 2019-02-27T07:56:53Z

What changes were proposed in this pull request?

Before dropping database refresh the tables of that database, so as to refresh all cached entries associated with those tables.
We follow the same when dropping a table.

How was this patch tested?

UT is added

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

dilipbiswal

On the call to drop the database, we can get an error. For example, its not allowed to drop the default database. In that case, even though we are not going to go ahead with the drop database action, we will end up refreshing all the tables inside it ? Is that expected ?

Udbhav30 · 2019-02-27T09:39:47Z

On the call to drop the database, we can get an error. For example, its not allowed to drop the default database. In that case, even though we are not going to go ahead with the drop database action, we will end up refreshing all the tables inside it ? Is that expected ?

modified

Udbhav30 · 2019-02-28T05:38:53Z

CC @HyukjinKwon

dilipbiswal · 2019-02-28T06:24:42Z

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

+      catalog.listTables(databaseName).foreach { t =>
+        catalog.refreshTable(t)
+      }
+    }
    sparkSession.sessionState.catalog.dropDatabase(databaseName, ifExists, cascade)


Can this logic move under catalog.dropDatabase function ? The reason is, we have the following code in dropDatabase.

val dbName = formatDatabaseName(db) if (dbName == DEFAULT_DATABASE) { throw new AnalysisException(s"Can not drop default database") }

If we keep it here, then we should have identical check. In this case formatDatabase().. handles case sensitivity in the database name. Also we should use DEFAULT_DATABASE as opposed to hard coding it here.

2ndly, can any of the calls 1) listTables 2)refreshTable throw exception ? In this case, do we want to fail the drop operation or allow the drop with a warning ? I don't know the answer. Just list the questions here so others can help answer.

cc @cloud-fan @gatorsmile for their input.

yes i have moved that code to catalog.dropDatabase functions, i also think that make more sense

felixcheung

I'd a bit worry about this approach. shouldn't this be done up to the user and not automatically?

Udbhav30 · 2019-02-28T07:22:22Z

I'd a bit worry about this approach. shouldn't this be done up to the user and not automatically?

Can we do it on the behalf of user as we are following this approach while using dropTable and refresh the table so these changes will also make it inline with dropTable behaviour.

Udbhav30 · 2019-03-01T05:09:19Z

cc @gatorsmile @dongjoon-hyun can you please review this.

dongjoon-hyun

Thank you for your first contribution, @Udbhav30 .

I left a few comment. Also, could you move this test case to DDLSuite.scala? If you put it there, InMemoryCatalogedDDLSuite and HiveCatalogedDDLSuite will test it.

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala

sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala

Udbhav30 · 2019-03-01T07:40:32Z

@dongjoon-hyun I have done the suggested changes

dongjoon-hyun · 2019-03-01T16:45:28Z

ok to test

SparkQA · 2019-03-01T18:39:13Z

Test build #102917 has finished for PR 23905 at commit 188ae92.

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

dongjoon-hyun · 2019-03-01T18:45:00Z

@Udbhav30 . Two test case failures look relevant to this PR. Could you check them by running locally? Please update this PR after you pass them locally. Thanks.

Udbhav30 · 2019-03-04T05:30:08Z

yes i will check and update

SparkQA · 2019-03-04T14:33:18Z

Test build #102995 has finished for PR 23905 at commit 2c8c2c5.

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

SparkQA · 2019-03-04T20:19:07Z

Test build #103001 has finished for PR 23905 at commit 3eb82de.

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

Udbhav30 · 2019-03-05T04:38:19Z

@dongjoon-hyun i have handled all suggestions please let me know if anymore suggestions are there
thanks

sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala

dongjoon-hyun · 2019-03-05T22:10:21Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala

@@ -218,6 +218,11 @@ class SessionCatalog(
    if (dbName == DEFAULT_DATABASE) {
      throw new AnalysisException(s"Can not drop default database")
    }
+    if (cascade && databaseExists(dbName)) {
+      listTables(dbName).foreach { t =>
+        refreshTable(t)


The goal of this PR is invalidating the tables. So, let's use more narrow API here instead of refreshTable.

- refreshTable(t) + invalidateCachedTable(QualifiedTableName(dbName, t.table))

sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala

dilipbiswal · 2019-03-05T22:18:38Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala

@@ -218,6 +218,11 @@ class SessionCatalog(
    if (dbName == DEFAULT_DATABASE) {
      throw new AnalysisException(s"Can not drop default database")
    }
+    if (cascade && databaseExists(dbName)) {
+      listTables(dbName).foreach { t =>


@dongjoon-hyun Do we need to worry about any recurring exceptions from this call like following ?
http://discuss.itversity.com/t/unable-to-perform-listtables-on-spark-catalog-class/15888
Should we fail the drop database or warn and proceed ?

The best behavior is compatible with the old Spark behavior. So, that will be warn and proceed.

Thank you @dongjoon-hyun

SparkQA · 2019-03-06T08:05:02Z

Test build #103082 has finished for PR 23905 at commit eab40d6.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-03-06T08:05:02Z

Test build #103083 has finished for PR 23905 at commit e653040.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-03-06T08:05:03Z

Test build #103081 has finished for PR 23905 at commit c4e3214.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2019-03-06T12:42:54Z

retest this please

SparkQA · 2019-03-06T16:44:03Z

Test build #103093 has finished for PR 23905 at commit e653040.

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

dongjoon-hyun

Thank you for your first contribution, @Udbhav30 . And thank you, @SongYadong , @dilipbiswal , @felixcheung , @cloud-fan .

+1, LGTM, too. Merged to master/2.4/2.3.

For the MetaException, we can ignore for now since it seems that Spark don't handle that except partitioning handling cases. We can revisit that if needed later.

## What changes were proposed in this pull request? Before dropping database refresh the tables of that database, so as to refresh all cached entries associated with those tables. We follow the same when dropping a table. UT is added Closes #23905 from Udbhav30/SPARK-24669. Authored-by: Udbhav30 <u.agrawal30@gmail.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com> (cherry picked from commit 9bddf71) Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

## What changes were proposed in this pull request? Before dropping database refresh the tables of that database, so as to refresh all cached entries associated with those tables. We follow the same when dropping a table. UT is added Closes apache#23905 from Udbhav30/SPARK-24669. Authored-by: Udbhav30 <u.agrawal30@gmail.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com> (cherry picked from commit 9bddf71) Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

SongYadong reviewed Feb 27, 2019

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala Outdated Show resolved Hide resolved

dilipbiswal reviewed Feb 27, 2019

View reviewed changes

Udbhav30 force-pushed the SPARK-24669 branch from e6598ba to adfb0a6 Compare February 27, 2019 09:38

dilipbiswal reviewed Feb 28, 2019

View reviewed changes

Udbhav30 force-pushed the SPARK-24669 branch from adfb0a6 to f483c62 Compare February 28, 2019 07:00

felixcheung reviewed Feb 28, 2019

View reviewed changes

dongjoon-hyun requested changes Mar 1, 2019

View reviewed changes

Udbhav30 force-pushed the SPARK-24669 branch 2 times, most recently from 6accf35 to 188ae92 Compare March 1, 2019 07:38

Udbhav30 force-pushed the SPARK-24669 branch from 188ae92 to 2c8c2c5 Compare March 4, 2019 11:03

Udbhav30 force-pushed the SPARK-24669 branch from 2c8c2c5 to 3eb82de Compare March 4, 2019 15:59