[hotfix] Fix drop database not working in terminal#3363
Conversation
huyuanfeng2018
left a comment
There was a problem hiding this comment.
Should we add a test to guard against future regression?
Added test, PTAL |
huyuanfeng2018
left a comment
There was a problem hiding this comment.
Thanks for the PR!
| throw new NoSuchNamespaceException(namespace); | ||
| } | ||
| List<TableIDWithFormat> tables = unifiedCatalog.listTables(database); | ||
| if (!tables.isEmpty() && !cascade) { |
There was a problem hiding this comment.
Why not drop all tables first and then drop the database?
I think should this logic be implemented directly by the parent (spark-common).
We already have concrete logic in SparkUnifiedCatalogBase.dropNamespace.
so，remove Override is enough.
WDYT?
There was a problem hiding this comment.
I agreed that we may reuse some implementation in the parent class. However I think it is dangerous to delete all tables by default in Spark 3.2 implementation. So we may:
- Add a method in the base class to support dropping namespace cascade or not.
- Drop namespace by default with false for Spark 3.2
|
Thanks for your review @zhoujinsong @huyuanfeng2018 |
huyuanfeng2018
left a comment
There was a problem hiding this comment.
just a nit comment
|
Thanks for the contribution! @MarigWeizhi |
* Implement dropNamespace * test namespace for SparkUnifiedCatalog * Implement cascade or non-cascading drop namespace in base * Restore private * fix * Remove redundant method --------- Co-authored-by: big face cat <731030576@qq.com>
Why are the changes needed?
Close #xxx.
Brief change log
dropNamespaceto support cascade or non-cascade inSparkUnifiedCatalogBasefor spark 3.3How was this patch tested?
Add some test cases that check the changes thoroughly including negative and positive cases if possible
Add screenshots for manual tests if appropriate
Run test locally before making a pull request
Documentation