Spark 3.3: drop_namespace with CASCADE support #7275

abmo-x · 2023-04-03T18:56:46Z

SparkCatalog in 3.3 passes the cascade flag, current implementation doesn't use the flag and ignores it quietly

iceberg/spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/SparkCatalog.java

Line 506 in d485cc8

public boolean dropNamespace(String[] namespace, boolean cascade)

we can use that to support drop_namespace with cascade and fix current implementation by adding new api

public boolean dropNamespace(String[] namespace, boolean cascade)

This PR adds new API to support cascade with drop_namespace in Iceberg Catalog

boolean dropNamespace(Namespace namespace, boolean cascade) throws NamespaceNotEmptyException;

drop namespace table purge behaviour by catalog for CASCADE

When namespace is dropped with CASCADE, all tables are dropped and contents are purged based on the type of
catalog.

Catalog	Table Purge	Nested Namespaces
Hive	- [x]	- [ ]
Hadoop	- [ ]	- [ x ]
JDBC	- [ ]	- [ x ]
Nessie	NotSupported	NotSupported
DynamoDb	NotSupported	NotSupported
Glue	NotSupported	NotSupported
ECS	NotSupported	NotSupported
Snowflake	NotSupported	NotSupported

Should Fix Issue: #3541

RussellSpitzer · 2023-04-04T15:57:32Z

api/src/main/java/org/apache/iceberg/catalog/SessionCatalog.java

+   *
+   * @param context session context
+   * @param namespace a {@link Namespace namespace}
+   * @param cascade – When true, deletes all objects under the namespace


This needs a stricter definition IMO. In some instances below here you delete folders, sometimes you delete files, sometimes it deletes entries. We need a strong definition here.

One big missing thing here is how this works with "purge"

I think we have two options, drop_namespace with cascade will:

drop the tables not the data (no purge)

drop the tables and delete the data (purge)

We can standardize this in Iceberg and go with the preferred option, #1 sounds better as #2 can take a long time depending on number of tables and data in them. This behavior is not consistent across all catalogs, for example hive metastore cascade deletes all the data (purge) in tables where as others don't.

should I create a vote thread for this to decide on the preference or can we decide that in this PR? I can then update the implementation.

@RussellSpitzer documented the current purge behavior in the PR description and docs.

Only Hive catalog purges the tables as that's the default behavior of hive when we pass the cascade=true parameter.

I can make it consistent across all catalogs and modify current hive catalog behavior to just drop table without purge similar to other catalogs. Let me know your thoughts

Yeah sorry I didn't get back to this but I do believe we probably need a discuss thread, or we can get feedback here but I think we need to hear from @jackye1995 and others who are maintaining other Catalog implementations.

removed implementation for other catalogs, will initiate a discussion once api change in this PR is merged

Glue by default does cascade drop if just call glue.deleteDatabase, but does not drop data in the table. So technically you can support that directly. But I am okay to publish another PR to support that, up to you.

For doing purge or not, I think the behavior of cascade is not clearly specified in Spark. It could also be argued as something that is catalog-specific, just like the behavior of Hive and Glue are different and it will be difficult to satisfy the other behavior on both sides.

This is related to the issue about different behaviors of list namespace that comes up recently. Maybe we should make a page documenting all catalogs and their different behaviors.

+1 to have a devlist discussion thread first.

I will start a devlist discussion and create a one pager with current state. Thanks @jackye1995

@jackye1995 To confirm are you OK with adding this api to Iceberg? Or do you want keep it within engine implementations?

yes I am okay with adding this to Iceberg API, otherwise there is no way to leverage catalog service level integration features. I would imagine people using REST catalog would like to just handle this behind the service.

RussellSpitzer · 2023-04-04T20:02:23Z

aws/src/main/java/org/apache/iceberg/aws/dynamodb/DynamoDbCatalog.java

+  public boolean dropNamespace(Namespace namespace, boolean cascade)
+      throws NamespaceNotEmptyException {
+    if (cascade) {
+      listTables(namespace).forEach(this::dropTable);


I think i'm missing something here since I'm not sure how dropTable get's called here with a single arg. The issue like I mentioned above is this would be a purely metadata delete for Dynamo I think in this case. Is that the expected behavior we want?

dropTable is overloaded and has two methods, 1 with just single arg TableIdentifier and another which takes the boolean purge arg, we are using the former one here where purge is false.

addressed the purge comment here (#7275 (comment)))

core/src/main/java/org/apache/iceberg/catalog/BaseSessionCatalog.java

RussellSpitzer · 2023-04-04T20:47:48Z

core/src/main/java/org/apache/iceberg/hadoop/HadoopCatalog.java

+      throws NamespaceNotEmptyException {
+    if (cascade) {
+      // recursively delete all nested namespaces
+      listNamespaces(namespace).forEach(n -> dropNamespace(n, true));


All of the other catalogs do not drop nested namespaces, do we need to cover this in other cases?

From my testing and existing tests, not all catalogs support nested namespaces only the ones I covered here do. Example Hive doesn't support nested namespaces.

If I missed any, I can add those as well.

RussellSpitzer · 2023-04-04T20:48:39Z

core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java

+      throws NamespaceNotEmptyException {
+    if (cascade) {
+      // recursively delete all nested namespaces
+      listNamespaces(namespace).forEach(n -> dropNamespace(n, true));


JDBC Supports nested namespaces?

I believe so based on

iceberg/core/src/test/java/org/apache/iceberg/jdbc/TestJdbcCatalog.java

Line 597 in d485cc8

public void testDropNamespace() {

abmo-x · 2023-04-19T04:39:11Z

@rdblue @kbendick
can you review when you get a chance, Thanks!

abmo-x · 2023-05-05T21:00:21Z

@RussellSpitzer
Makes sense, I removed implementation for various catalogs and left them as unsupported.
Only implemented for Hive, Hadoop and JDBC.

RussellSpitzer · 2023-05-05T21:29:27Z

Tests are failing because of AssertHelpers usage, think that needs a minor change to AssertJ

abmo-x · 2023-05-09T18:10:44Z

@RussellSpitzer fixed build, Thanks

RussellSpitzer · 2023-05-09T19:20:16Z

snowflake/src/main/java/org/apache/iceberg/snowflake/SnowflakeCatalog.java

  }

+  @Override
+  public boolean dropNamespace(Namespace namespace, boolean cascade) {


shouldn't be needed

I may be missing something, its needed to implement the interface though?

Class 'SnowflakeCatalog' must either be declared abstract or implement abstract method 'dropNamespace(Namespace, boolean)' in 'SupportsNamespaces'

Oh above you would add this with a "default" throw unsupported operation exception to the api. That way you don't have to add exception throwing code to all the catalogs that don't implement it

This also prevents the change from breaking other catalogs not in the Iceberg repo

ah! I see what you mean, makes sense. Thanks

@RussellSpitzer made it default.

RussellSpitzer · 2023-05-09T20:42:00Z

api/src/main/java/org/apache/iceberg/catalog/SessionCatalog.java

+   * @return true if the namespace was dropped, false otherwise.
+   * @throws NamespaceNotEmptyException If the namespace is not empty
+   */
+  boolean dropNamespace(SessionContext context, Namespace namespace, boolean cascade);


Do we need the default here as well? I think we do for any implementers of SessionCatalog

RussellSpitzer · 2023-05-09T21:42:27Z

docs/spark-ddl.md

+To drop a namespace and all its contents including tables, run:
+
+```sql
+DROP TABLE prod.db.sample CASCADE


Should this be table? or database?

RussellSpitzer · 2023-05-09T21:42:44Z

spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/SparkCatalog.java

    if (asNamespaceCatalog != null) {
      try {
-        return asNamespaceCatalog.dropNamespace(Namespace.of(namespace));
+        return asNamespaceCatalog.dropNamespace(Namespace.of(namespace), cascade);


Needs tests for the Spark DDL

vinitamaloo-asu · 2023-10-31T23:36:11Z

@abmo-x any when do you plan to merge this PR?

supsupsap · 2024-04-02T11:18:20Z

@abmo-x do you plan to merge this PR?

github-actions · 2024-08-28T00:14:18Z

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.

github-actions · 2024-09-05T00:13:58Z

This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

Abid Mohammed added 10 commits April 2, 2023 07:21

SPARK 3.3 : add cascade support in drop_namespace

b41e4bf

added all tests drop namespace cascade

f2a7ace

more tests for drop namespace with cascade

8441c74

more tests for drop namespace with cascade

00671d2

cleanup

a4049d6

spotless

01de116

add test for cascade false

d13837a

spotless

da335fd

cleanup rest adapter

2c31a4f

fix cyclomatic complexity

db87d5a

github-actions bot added API AWS core DELL hive NESSIE SNOWFLAKE spark labels Apr 3, 2023

RussellSpitzer reviewed Apr 4, 2023

View reviewed changes

core/src/main/java/org/apache/iceberg/catalog/BaseSessionCatalog.java Show resolved Hide resolved

RussellSpitzer reviewed Apr 4, 2023

View reviewed changes

Abid Mohammed added 2 commits April 5, 2023 16:14

add purge behaviour for cascade drop namespace

aa63ee3

fix tests

28b289b

github-actions bot added the docs label Apr 18, 2023

abmo-x requested a review from RussellSpitzer April 18, 2023 22:45

Abid Mohammed added 2 commits April 18, 2023 15:52

fix tests

c5dd8a5

fix tests

5ad1e94

Abid Mohammed added 4 commits May 5, 2023 11:00

Revert: cascade support for other catalog implementations

c75ea5c

resolve conflicts

4c05fcb

revapi

59bb5e3

update docs

e485616

use cataloghandlers

4a04148

Abid Mohammed added 4 commits May 8, 2023 17:42

retrigger checks

95d6676

fix assertj error

f5219db

fix assertj error

ac28327

revert nessie cascade changes

187d378

RussellSpitzer reviewed May 9, 2023

View reviewed changes

make dropNamespace with cascade default impl

e43de0e

github-actions bot removed SNOWFLAKE DELL NESSIE AWS labels May 9, 2023

spotless

3ba10f9

RussellSpitzer reviewed May 9, 2023

View reviewed changes

make dropNamespace with cascade default impl in session catalog

3e852ac

RussellSpitzer reviewed May 9, 2023

View reviewed changes

amogh-jahagirdar mentioned this pull request Sep 10, 2023

CASCADE WITH Drop Namespace Gives exception #8529

Closed

github-actions bot added the stale label Aug 28, 2024

github-actions bot closed this Sep 5, 2024

Spark 3.3: drop_namespace with CASCADE support #7275

Spark 3.3: drop_namespace with CASCADE support #7275

Uh oh!

Conversation

abmo-x commented Apr 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

drop namespace table purge behaviour by catalog for CASCADE

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abmo-x Apr 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abmo-x commented Apr 19, 2023

Uh oh!

abmo-x commented May 5, 2023

Uh oh!

RussellSpitzer commented May 5, 2023

Uh oh!

abmo-x commented May 9, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vinitamaloo-asu commented Oct 31, 2023

Uh oh!

supsupsap commented Apr 2, 2024

Uh oh!

github-actions bot commented Aug 28, 2024

Uh oh!

github-actions bot commented Sep 5, 2024

Uh oh!

Reviewers

abmo-x commented Apr 3, 2023 •

edited

Loading

abmo-x Apr 5, 2023 •

edited

Loading