Spark: Add support for CASCADE option in DROP NAMESPACE - #16096
Conversation
ab6aaa4 to
c33ef20
Compare
c33ef20 to
c4de5a8
Compare
| for (String[] ns : listNamespaces(namespace)) { | ||
| dropNamespace(ns, true); | ||
| } |
There was a problem hiding this comment.
shouldn't we drop all the namespaces last ?
There was a problem hiding this comment.
Do you mean views → tables → nested views → nested tables → all namespaces?
There was a problem hiding this comment.
@singhpk234 I'm wondering whether this is really worth doing, since it requires additional logic. Could you explain the motivation for dropping namespaces last?
| @TestTemplate | ||
| public void testDropNamespaceCascade() { | ||
| assumeThat(catalogName).as("Session catalog has flaky behavior").isNotEqualTo("spark_catalog"); | ||
| assumeThat(catalogName).as("Multi part namespace is unsupported").isNotEqualTo("testhive"); |
There was a problem hiding this comment.
can we have coverage for just non-nested namespace then for this ?
| assertThat(validationNamespaceCatalog.namespaceExists(NS)).isTrue(); | ||
| assertThat(validationNamespaceCatalog.namespaceExists(nestedNs)).isTrue(); | ||
| assertThat(validationCatalog.tableExists(TableIdentifier.of(NS, "table"))).isTrue(); | ||
| assertThat(validationCatalog.tableExists(TableIdentifier.of(nestedNs, "table"))).isTrue(); |
There was a problem hiding this comment.
lets add a view too in both the namespace ?
There was a problem hiding this comment.
Hadoop catalog doesn't support views. That is why I updated TestViews. Left a code comment.
| for (String[] ns : listNamespaces(namespace)) { | ||
| dropNamespace(ns, true); | ||
| } | ||
| for (Identifier view : listViews(namespace)) { | ||
| dropView(view); | ||
| } | ||
| for (Identifier table : listTables(namespace)) { | ||
| dropTable(table); | ||
| } |
There was a problem hiding this comment.
| for (String[] ns : listNamespaces(namespace)) { | |
| dropNamespace(ns, true); | |
| } | |
| for (Identifier view : listViews(namespace)) { | |
| dropView(view); | |
| } | |
| for (Identifier table : listTables(namespace)) { | |
| dropTable(table); | |
| } | |
| listNamespaces(namespace).forEach(n -> dropNamespace(n, true)); | |
| listViews(namespace).forEach(v -> dropView(v)); | |
| listTables(table).forEach(t -> dropTable(t)); |
There was a problem hiding this comment.
Those methods return arrays. I updated views / tables lines with Arrays.stream. Skipped namespace line because the method throws NoSuchNamespaceException which requires additional try-catch.
|
Addressed comments except for #16096 (comment) |
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
|
@singhpk234 could you take another look when you have time? |
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
7e4e7cb to
18561ff
Compare
2 participants
Summary
Add support for the CASCADE option in DROP NAMESPACE statements in Spark 4.1.
When CASCADE is specified, the operation recursively drops all nested namespaces, tables,
and views within the target namespace before dropping the namespace itself.
Implementation