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

[Udbhav30]

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

[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]

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]

CC @HyukjinKwon

[dilipbiswal]

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.

[Udbhav30]

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]

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]

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.

[Udbhav30]

@dongjoon-hyun I have done the suggested changes

[dongjoon-hyun]

ok to test

[SparkQA]

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]

@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]

yes i will check and update

[SparkQA]

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]

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]

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

[dongjoon-hyun]

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))

[dilipbiswal]

@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 ?

[dongjoon-hyun]

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

[dilipbiswal]

Thank you @dongjoon-hyun

[SparkQA]

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]

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]

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]

retest this please

[SparkQA]

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.