-
Notifications
You must be signed in to change notification settings - Fork 28.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-33364][SQL] Introduce the "purge" option in TableCatalog.dropTable for v2 catalog. #30267
Conversation
Kubernetes integration test starting |
Kubernetes integration test status success |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, LGTM. Thank you, @imback82 .
Test build #130664 has finished for PR 30267 at commit
|
* <p> | ||
* If the purge option is set to false, the default implementation falls back to | ||
* {@link #dropTable(Identifier)} dropTable}. Otherwise, it throws | ||
* {@link UnsupportedOperationException}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should mention that, people need to overwrite this method to support the purge option.
* If the catalog supports views and contains a view for the identifier and not a table, this | ||
* must not drop the view and must return false. | ||
* <p> | ||
* If the catalog supports the option to purge a table, this method must be overwritten. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overwritten
-> overridden
.
Kubernetes integration test starting |
Merged to master. The last two commits are only for comment changes. |
Thanks @dongjoon-hyun / @cloud-fan for the review! |
Kubernetes integration test status success |
Kubernetes integration test starting |
Kubernetes integration test status success |
Test build #130692 has finished for PR 30267 at commit
|
Test build #130689 has finished for PR 30267 at commit
|
Late +1 from me. Thanks @imback82! |
### What changes were proposed in this pull request? This is a followup of #30267 Inspired by #30886, it's better to have 2 methods `def dropTable` and `def purgeTable`, than `def dropTable(ident)` and `def dropTable(ident, purge)`. ### Why are the changes needed? 1. make the APIs orthogonal. Previously, `def dropTable(ident, purge)` calls `def dropTable(ident)` and is a superset. 2. simplifies the catalog implementation a little bit. Now the `if (purge) ... else ...` check is done at the Spark side. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? existing tests Closes #30890 from cloud-fan/purgeTable. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: HyukjinKwon <[email protected]>
This is a followup of #30267 Inspired by #30886, it's better to have 2 methods `def dropTable` and `def purgeTable`, than `def dropTable(ident)` and `def dropTable(ident, purge)`. 1. make the APIs orthogonal. Previously, `def dropTable(ident, purge)` calls `def dropTable(ident)` and is a superset. 2. simplifies the catalog implementation a little bit. Now the `if (purge) ... else ...` check is done at the Spark side. No. existing tests Closes #30890 from cloud-fan/purgeTable. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: HyukjinKwon <[email protected]> (cherry picked from commit ec1560a) Signed-off-by: HyukjinKwon <[email protected]>
What changes were proposed in this pull request?
This PR proposes to introduce the
purge
option inTableCatalog.dropTable
so that v2 catalogs can use the option if needed.Related discussion: #30079 (comment)
Why are the changes needed?
Spark DDL supports passing the purge option to
DROP TABLE
command. However, the option is not used (ignored) for v2 catalogs.Does this PR introduce any user-facing change?
This PR introduces a new API in
TableCatalog
.How was this patch tested?
Added a test.