You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "abmo-x (via GitHub)" <gi...@apache.org> on 2023/04/03 18:56:48 UTC
[GitHub] [iceberg] abmo-x opened a new pull request, #7275: Spark 3.3: drop_namespace with CASCADE support
abmo-x opened a new pull request, #7275:
URL: https://github.com/apache/iceberg/pull/7275
SparkCatalog in 3.3 passes the `cascade` flag, we can use that to support drop_namespace with cascade:
```
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;
https://github.com/apache/iceberg/issues/3541
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] abmo-x commented on a diff in pull request #7275: Spark 3.3: drop_namespace with CASCADE support
Posted by "abmo-x (via GitHub)" <gi...@apache.org>.
abmo-x commented on code in PR #7275:
URL: https://github.com/apache/iceberg/pull/7275#discussion_r1157911479
##########
core/src/main/java/org/apache/iceberg/catalog/BaseSessionCatalog.java:
##########
@@ -143,6 +143,16 @@ public boolean dropNamespace(Namespace namespace) throws NamespaceNotEmptyExcept
return BaseSessionCatalog.this.dropNamespace(context, namespace);
}
+ @Override
+ public boolean dropNamespace(Namespace namespace, boolean cascade)
+ throws NamespaceNotEmptyException {
+ if (cascade) {
+ return BaseSessionCatalog.this.dropNamespace(context, namespace, true);
Review Comment:
I don't think that's the case. this is calling base method with 3 args similar to all implementations in this class.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] abmo-x commented on pull request #7275: Spark 3.3: drop_namespace with CASCADE support
Posted by "abmo-x (via GitHub)" <gi...@apache.org>.
abmo-x commented on PR #7275:
URL: https://github.com/apache/iceberg/pull/7275#issuecomment-1514120201
@rdblue @kbendick
can you review when you get a chance, Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] abmo-x commented on a diff in pull request #7275: Spark 3.3: drop_namespace with CASCADE support
Posted by "abmo-x (via GitHub)" <gi...@apache.org>.
abmo-x commented on code in PR #7275:
URL: https://github.com/apache/iceberg/pull/7275#discussion_r1189094796
##########
snowflake/src/main/java/org/apache/iceberg/snowflake/SnowflakeCatalog.java:
##########
@@ -214,6 +214,12 @@ public boolean dropNamespace(Namespace namespace) {
"SnowflakeCatalog does not currently support dropNamespace");
}
+ @Override
+ public boolean dropNamespace(Namespace namespace, boolean cascade) {
Review Comment:
ah! I see what you mean, makes sense. Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a diff in pull request #7275: Spark 3.3: drop_namespace with CASCADE support
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #7275:
URL: https://github.com/apache/iceberg/pull/7275#discussion_r1174045218
##########
core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java:
##########
@@ -363,6 +363,16 @@ public <T extends RESTResponse> T handleRequest(
return null;
}
+ private void dropNamespace(Map<String, String> vars) {
+ Namespace namespace = namespaceFromPathVars(vars);
+ boolean cascade = PropertyUtil.propertyAsBoolean(vars, "cascade", false);
+ if (cascade) {
Review Comment:
Couldn't we always just call the three arg version here? since we can just call CatalogHandlers.dropNamespace(asNamesapceCatalog, namespace, cascade)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #7275: Spark 3.3: drop_namespace with CASCADE support
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #7275:
URL: https://github.com/apache/iceberg/pull/7275#discussion_r1189078954
##########
api/src/main/java/org/apache/iceberg/catalog/SessionCatalog.java:
##########
@@ -309,6 +309,17 @@ default List<Namespace> listNamespaces(SessionContext context) {
*/
boolean dropNamespace(SessionContext context, Namespace namespace);
+ /**
+ * Drop a namespace. If the namespace exists and was dropped, this will return true.
+ *
+ * @param context session context
+ * @param namespace a {@link Namespace namespace}
+ * @param cascade – When true, deletes all objects under the namespace
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] abmo-x commented on pull request #7275: Spark 3.3: drop_namespace with CASCADE support
Posted by "abmo-x (via GitHub)" <gi...@apache.org>.
abmo-x commented on PR #7275:
URL: https://github.com/apache/iceberg/pull/7275#issuecomment-1540640591
@RussellSpitzer fixed build, Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] abmo-x commented on a diff in pull request #7275: Spark 3.3: drop_namespace with CASCADE support
Posted by "abmo-x (via GitHub)" <gi...@apache.org>.
abmo-x commented on code in PR #7275:
URL: https://github.com/apache/iceberg/pull/7275#discussion_r1186498121
##########
api/src/main/java/org/apache/iceberg/catalog/SessionCatalog.java:
##########
@@ -309,6 +309,17 @@ default List<Namespace> listNamespaces(SessionContext context) {
*/
boolean dropNamespace(SessionContext context, Namespace namespace);
+ /**
+ * Drop a namespace. If the namespace exists and was dropped, this will return true.
+ *
+ * @param context session context
+ * @param namespace a {@link Namespace namespace}
+ * @param cascade – When true, deletes all objects under the namespace
Review Comment:
removed implementation for other catalogs, will initiate a discussion once api change in this PR is merged
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a diff in pull request #7275: Spark 3.3: drop_namespace with CASCADE support
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #7275:
URL: https://github.com/apache/iceberg/pull/7275#discussion_r1189085426
##########
snowflake/src/main/java/org/apache/iceberg/snowflake/SnowflakeCatalog.java:
##########
@@ -214,6 +214,12 @@ public boolean dropNamespace(Namespace namespace) {
"SnowflakeCatalog does not currently support dropNamespace");
}
+ @Override
+ public boolean dropNamespace(Namespace namespace, boolean cascade) {
Review Comment:
This also prevents the change from breaking other catalogs not in the Iceberg repo
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a diff in pull request #7275: Spark 3.3: drop_namespace with CASCADE support
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #7275:
URL: https://github.com/apache/iceberg/pull/7275#discussion_r1174039674
##########
api/src/main/java/org/apache/iceberg/catalog/SessionCatalog.java:
##########
@@ -309,6 +309,17 @@ default List<Namespace> listNamespaces(SessionContext context) {
*/
boolean dropNamespace(SessionContext context, Namespace namespace);
+ /**
+ * Drop a namespace. If the namespace exists and was dropped, this will return true.
+ *
+ * @param context session context
+ * @param namespace a {@link Namespace namespace}
+ * @param cascade – When true, deletes all objects under the namespace
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] abmo-x commented on a diff in pull request #7275: Spark 3.3: drop_namespace with CASCADE support
Posted by "abmo-x (via GitHub)" <gi...@apache.org>.
abmo-x commented on code in PR #7275:
URL: https://github.com/apache/iceberg/pull/7275#discussion_r1157908325
##########
core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java:
##########
@@ -419,6 +419,17 @@ public boolean dropNamespace(Namespace namespace) throws NamespaceNotEmptyExcept
return deletedRows > 0;
}
+ @Override
+ public boolean dropNamespace(Namespace namespace, boolean cascade)
+ throws NamespaceNotEmptyException {
+ if (cascade) {
+ // recursively delete all nested namespaces
+ listNamespaces(namespace).forEach(n -> dropNamespace(n, true));
Review Comment:
I believe so based on https://github.com/apache/iceberg/blob/d485cc83ec58bd587fbfaaacebcf624c7f4c44d3/core/src/test/java/org/apache/iceberg/jdbc/TestJdbcCatalog.java#L597
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] abmo-x commented on a diff in pull request #7275: Spark 3.3: drop_namespace with CASCADE support
Posted by "abmo-x (via GitHub)" <gi...@apache.org>.
abmo-x commented on code in PR #7275:
URL: https://github.com/apache/iceberg/pull/7275#discussion_r1157909282
##########
aws/src/main/java/org/apache/iceberg/aws/dynamodb/DynamoDbCatalog.java:
##########
@@ -284,6 +284,15 @@ public boolean dropNamespace(Namespace namespace) throws NamespaceNotEmptyExcept
}
}
+ @Override
+ public boolean dropNamespace(Namespace namespace, boolean cascade)
+ throws NamespaceNotEmptyException {
+ if (cascade) {
+ listTables(namespace).forEach(this::dropTable);
Review Comment:
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 (TBD)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a diff in pull request #7275: Spark 3.3: drop_namespace with CASCADE support
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #7275:
URL: https://github.com/apache/iceberg/pull/7275#discussion_r1157747661
##########
core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java:
##########
@@ -419,6 +419,17 @@ public boolean dropNamespace(Namespace namespace) throws NamespaceNotEmptyExcept
return deletedRows > 0;
}
+ @Override
+ public boolean dropNamespace(Namespace namespace, boolean cascade)
+ throws NamespaceNotEmptyException {
+ if (cascade) {
+ // recursively delete all nested namespaces
+ listNamespaces(namespace).forEach(n -> dropNamespace(n, true));
Review Comment:
JDBC Supports nested namespaces?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a diff in pull request #7275: Spark 3.3: drop_namespace with CASCADE support
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #7275:
URL: https://github.com/apache/iceberg/pull/7275#discussion_r1189085128
##########
snowflake/src/main/java/org/apache/iceberg/snowflake/SnowflakeCatalog.java:
##########
@@ -214,6 +214,12 @@ public boolean dropNamespace(Namespace namespace) {
"SnowflakeCatalog does not currently support dropNamespace");
}
+ @Override
+ public boolean dropNamespace(Namespace namespace, boolean cascade) {
Review Comment:
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 is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a diff in pull request #7275: Spark 3.3: drop_namespace with CASCADE support
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #7275:
URL: https://github.com/apache/iceberg/pull/7275#discussion_r1189112162
##########
api/src/main/java/org/apache/iceberg/catalog/SessionCatalog.java:
##########
@@ -309,6 +309,17 @@ default List<Namespace> listNamespaces(SessionContext context) {
*/
boolean dropNamespace(SessionContext context, Namespace namespace);
+ /**
+ * Drop a namespace. If the namespace exists and was dropped, this will return true.
+ *
+ * @param context session context
+ * @param namespace a {@link Namespace namespace}
+ * @param cascade – When true, deletes all objects under the namespace
+ * @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);
Review Comment:
Do we need the default here as well? I think we do for any implementers of SessionCatalog
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a diff in pull request #7275: Spark 3.3: drop_namespace with CASCADE support
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #7275:
URL: https://github.com/apache/iceberg/pull/7275#discussion_r1157463759
##########
api/src/main/java/org/apache/iceberg/catalog/SessionCatalog.java:
##########
@@ -309,6 +309,17 @@ default List<Namespace> listNamespaces(SessionContext context) {
*/
boolean dropNamespace(SessionContext context, Namespace namespace);
+ /**
+ * Drop a namespace. If the namespace exists and was dropped, this will return true.
+ *
+ * @param context session context
+ * @param namespace a {@link Namespace namespace}
+ * @param cascade – When true, deletes all objects under the namespace
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] abmo-x commented on a diff in pull request #7275: Spark 3.3: drop_namespace with CASCADE support
Posted by "abmo-x (via GitHub)" <gi...@apache.org>.
abmo-x commented on code in PR #7275:
URL: https://github.com/apache/iceberg/pull/7275#discussion_r1157911479
##########
core/src/main/java/org/apache/iceberg/catalog/BaseSessionCatalog.java:
##########
@@ -143,6 +143,16 @@ public boolean dropNamespace(Namespace namespace) throws NamespaceNotEmptyExcept
return BaseSessionCatalog.this.dropNamespace(context, namespace);
}
+ @Override
+ public boolean dropNamespace(Namespace namespace, boolean cascade)
+ throws NamespaceNotEmptyException {
+ if (cascade) {
+ return BaseSessionCatalog.this.dropNamespace(context, namespace, true);
Review Comment:
I don't think that's the case, from my understanding this is calling base method with 3 args similar to all implementations in this class.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a diff in pull request #7275: Spark 3.3: drop_namespace with CASCADE support
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #7275:
URL: https://github.com/apache/iceberg/pull/7275#discussion_r1189036365
##########
api/src/main/java/org/apache/iceberg/catalog/SessionCatalog.java:
##########
@@ -309,6 +309,17 @@ default List<Namespace> listNamespaces(SessionContext context) {
*/
boolean dropNamespace(SessionContext context, Namespace namespace);
+ /**
+ * Drop a namespace. If the namespace exists and was dropped, this will return true.
+ *
+ * @param context session context
+ * @param namespace a {@link Namespace namespace}
+ * @param cascade – When true, deletes all objects under the namespace
Review Comment:
@jackye1995 To confirm are you OK with adding this api to Iceberg? Or do you want keep it within engine implementations?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] abmo-x commented on a diff in pull request #7275: Spark 3.3: drop_namespace with CASCADE support
Posted by "abmo-x (via GitHub)" <gi...@apache.org>.
abmo-x commented on code in PR #7275:
URL: https://github.com/apache/iceberg/pull/7275#discussion_r1186499075
##########
core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java:
##########
@@ -363,6 +363,16 @@ public <T extends RESTResponse> T handleRequest(
return null;
}
+ private void dropNamespace(Map<String, String> vars) {
+ Namespace namespace = namespaceFromPathVars(vars);
+ boolean cascade = PropertyUtil.propertyAsBoolean(vars, "cascade", false);
+ if (cascade) {
Review Comment:
yeah, that works. Fixed, Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] abmo-x commented on pull request #7275: Spark 3.3: drop_namespace with CASCADE support
Posted by "abmo-x (via GitHub)" <gi...@apache.org>.
abmo-x commented on PR #7275:
URL: https://github.com/apache/iceberg/pull/7275#issuecomment-1536774971
@RussellSpitzer
Makes sense, I removed implementation for various catalogs and left them as unsupported.
Only implemented for Hive, Hadoop and JDBC.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #7275: Spark 3.3: drop_namespace with CASCADE support
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #7275:
URL: https://github.com/apache/iceberg/pull/7275#discussion_r1188102874
##########
api/src/main/java/org/apache/iceberg/catalog/SessionCatalog.java:
##########
@@ -309,6 +309,17 @@ default List<Namespace> listNamespaces(SessionContext context) {
*/
boolean dropNamespace(SessionContext context, Namespace namespace);
+ /**
+ * Drop a namespace. If the namespace exists and was dropped, this will return true.
+ *
+ * @param context session context
+ * @param namespace a {@link Namespace namespace}
+ * @param cascade – When true, deletes all objects under the namespace
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a diff in pull request #7275: Spark 3.3: drop_namespace with CASCADE support
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #7275:
URL: https://github.com/apache/iceberg/pull/7275#discussion_r1157696961
##########
api/src/main/java/org/apache/iceberg/catalog/SessionCatalog.java:
##########
@@ -309,6 +309,17 @@ default List<Namespace> listNamespaces(SessionContext context) {
*/
boolean dropNamespace(SessionContext context, Namespace namespace);
+ /**
+ * Drop a namespace. If the namespace exists and was dropped, this will return true.
+ *
+ * @param context session context
+ * @param namespace a {@link Namespace namespace}
+ * @param cascade – When true, deletes all objects under the namespace
Review Comment:
One big missing thing here is how this works with "purge"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] abmo-x commented on a diff in pull request #7275: Spark 3.3: drop_namespace with CASCADE support
Posted by "abmo-x (via GitHub)" <gi...@apache.org>.
abmo-x commented on code in PR #7275:
URL: https://github.com/apache/iceberg/pull/7275#discussion_r1157912588
##########
api/src/main/java/org/apache/iceberg/catalog/SessionCatalog.java:
##########
@@ -309,6 +309,17 @@ default List<Namespace> listNamespaces(SessionContext context) {
*/
boolean dropNamespace(SessionContext context, Namespace namespace);
+ /**
+ * Drop a namespace. If the namespace exists and was dropped, this will return true.
+ *
+ * @param context session context
+ * @param namespace a {@link Namespace namespace}
+ * @param cascade – When true, deletes all objects under the namespace
Review Comment:
I think we have two options, drop_namespace with cascade will:
1) drop the tables not the data (no purge)
2) 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] abmo-x commented on a diff in pull request #7275: Spark 3.3: drop_namespace with CASCADE support
Posted by "abmo-x (via GitHub)" <gi...@apache.org>.
abmo-x commented on code in PR #7275:
URL: https://github.com/apache/iceberg/pull/7275#discussion_r1189069028
##########
snowflake/src/main/java/org/apache/iceberg/snowflake/SnowflakeCatalog.java:
##########
@@ -214,6 +214,12 @@ public boolean dropNamespace(Namespace namespace) {
"SnowflakeCatalog does not currently support dropNamespace");
}
+ @Override
+ public boolean dropNamespace(Namespace namespace, boolean cascade) {
Review Comment:
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'
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark 3.3: drop_namespace with CASCADE support [iceberg]
Posted by "vinitamaloo-asu (via GitHub)" <gi...@apache.org>.
vinitamaloo-asu commented on PR #7275:
URL: https://github.com/apache/iceberg/pull/7275#issuecomment-1788177314
@abmo-x any when do you plan to merge this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark 3.3: drop_namespace with CASCADE support [iceberg]
Posted by "supsupsap (via GitHub)" <gi...@apache.org>.
supsupsap commented on PR #7275:
URL: https://github.com/apache/iceberg/pull/7275#issuecomment-2031741342
@abmo-x do you plan to merge this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] abmo-x commented on a diff in pull request #7275: Spark 3.3: drop_namespace with CASCADE support
Posted by "abmo-x (via GitHub)" <gi...@apache.org>.
abmo-x commented on code in PR #7275:
URL: https://github.com/apache/iceberg/pull/7275#discussion_r1189127318
##########
api/src/main/java/org/apache/iceberg/catalog/SessionCatalog.java:
##########
@@ -309,6 +309,17 @@ default List<Namespace> listNamespaces(SessionContext context) {
*/
boolean dropNamespace(SessionContext context, Namespace namespace);
+ /**
+ * Drop a namespace. If the namespace exists and was dropped, this will return true.
+ *
+ * @param context session context
+ * @param namespace a {@link Namespace namespace}
+ * @param cascade – When true, deletes all objects under the namespace
+ * @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);
Review Comment:
Added.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a diff in pull request #7275: Spark 3.3: drop_namespace with CASCADE support
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #7275:
URL: https://github.com/apache/iceberg/pull/7275#discussion_r1189160664
##########
docs/spark-ddl.md:
##########
@@ -140,6 +140,47 @@ AS SELECT ...
The schema and partition spec will be replaced if changed. To avoid modifying the table's schema and partitioning, use `INSERT OVERWRITE` instead of `REPLACE TABLE`.
The new table properties in the `REPLACE TABLE` command will be merged with any existing table properties. The existing table properties will be updated if changed else they are preserved.
+## `DROP NAMESPACE`
+
+### `DROP EMPTY NAMESPACE`
+
+To drop an _empty_ namespace, run:
+
+```sql
+DROP database prod.db.sample
+```
+If the namespace is not empty, this will fail with _NamespaceNotEmptyException_.
+
+
+### `DROP NON_EMPTY NAMESPACE`
+
+To drop a namespace and all its contents including tables, run:
+
+```sql
+DROP TABLE prod.db.sample CASCADE
Review Comment:
Should this be table? or database?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a diff in pull request #7275: Spark 3.3: drop_namespace with CASCADE support
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #7275:
URL: https://github.com/apache/iceberg/pull/7275#discussion_r1189041017
##########
snowflake/src/main/java/org/apache/iceberg/snowflake/SnowflakeCatalog.java:
##########
@@ -214,6 +214,12 @@ public boolean dropNamespace(Namespace namespace) {
"SnowflakeCatalog does not currently support dropNamespace");
}
+ @Override
+ public boolean dropNamespace(Namespace namespace, boolean cascade) {
Review Comment:
shouldn't be needed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a diff in pull request #7275: Spark 3.3: drop_namespace with CASCADE support
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #7275:
URL: https://github.com/apache/iceberg/pull/7275#discussion_r1157704756
##########
aws/src/main/java/org/apache/iceberg/aws/dynamodb/DynamoDbCatalog.java:
##########
@@ -284,6 +284,15 @@ public boolean dropNamespace(Namespace namespace) throws NamespaceNotEmptyExcept
}
}
+ @Override
+ public boolean dropNamespace(Namespace namespace, boolean cascade)
+ throws NamespaceNotEmptyException {
+ if (cascade) {
+ listTables(namespace).forEach(this::dropTable);
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] abmo-x commented on a diff in pull request #7275: Spark 3.3: drop_namespace with CASCADE support
Posted by "abmo-x (via GitHub)" <gi...@apache.org>.
abmo-x commented on code in PR #7275:
URL: https://github.com/apache/iceberg/pull/7275#discussion_r1159116353
##########
api/src/main/java/org/apache/iceberg/catalog/SessionCatalog.java:
##########
@@ -309,6 +309,17 @@ default List<Namespace> listNamespaces(SessionContext context) {
*/
boolean dropNamespace(SessionContext context, Namespace namespace);
+ /**
+ * Drop a namespace. If the namespace exists and was dropped, this will return true.
+ *
+ * @param context session context
+ * @param namespace a {@link Namespace namespace}
+ * @param cascade – When true, deletes all objects under the namespace
Review Comment:
@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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on pull request #7275: Spark 3.3: drop_namespace with CASCADE support
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #7275:
URL: https://github.com/apache/iceberg/pull/7275#issuecomment-1536800759
Tests are failing because of AssertHelpers usage, think that needs a minor change to AssertJ
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] abmo-x commented on a diff in pull request #7275: Spark 3.3: drop_namespace with CASCADE support
Posted by "abmo-x (via GitHub)" <gi...@apache.org>.
abmo-x commented on code in PR #7275:
URL: https://github.com/apache/iceberg/pull/7275#discussion_r1188967051
##########
api/src/main/java/org/apache/iceberg/catalog/SessionCatalog.java:
##########
@@ -309,6 +309,17 @@ default List<Namespace> listNamespaces(SessionContext context) {
*/
boolean dropNamespace(SessionContext context, Namespace namespace);
+ /**
+ * Drop a namespace. If the namespace exists and was dropped, this will return true.
+ *
+ * @param context session context
+ * @param namespace a {@link Namespace namespace}
+ * @param cascade – When true, deletes all objects under the namespace
Review Comment:
I will start a devlist discussion and create a one pager with current state. Thanks @jackye1995
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] abmo-x commented on a diff in pull request #7275: Spark 3.3: drop_namespace with CASCADE support
Posted by "abmo-x (via GitHub)" <gi...@apache.org>.
abmo-x commented on code in PR #7275:
URL: https://github.com/apache/iceberg/pull/7275#discussion_r1189099303
##########
snowflake/src/main/java/org/apache/iceberg/snowflake/SnowflakeCatalog.java:
##########
@@ -214,6 +214,12 @@ public boolean dropNamespace(Namespace namespace) {
"SnowflakeCatalog does not currently support dropNamespace");
}
+ @Override
+ public boolean dropNamespace(Namespace namespace, boolean cascade) {
Review Comment:
@RussellSpitzer made it default.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a diff in pull request #7275: Spark 3.3: drop_namespace with CASCADE support
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #7275:
URL: https://github.com/apache/iceberg/pull/7275#discussion_r1189160910
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/SparkCatalog.java:
##########
@@ -507,7 +507,7 @@ public boolean dropNamespace(String[] namespace, boolean cascade)
throws NoSuchNamespaceException {
if (asNamespaceCatalog != null) {
try {
- return asNamespaceCatalog.dropNamespace(Namespace.of(namespace));
+ return asNamespaceCatalog.dropNamespace(Namespace.of(namespace), cascade);
Review Comment:
Needs tests for the Spark DDL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a diff in pull request #7275: Spark 3.3: drop_namespace with CASCADE support
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #7275:
URL: https://github.com/apache/iceberg/pull/7275#discussion_r1174040485
##########
core/src/main/java/org/apache/iceberg/catalog/BaseSessionCatalog.java:
##########
@@ -143,6 +143,16 @@ public boolean dropNamespace(Namespace namespace) throws NamespaceNotEmptyExcept
return BaseSessionCatalog.this.dropNamespace(context, namespace);
}
+ @Override
+ public boolean dropNamespace(Namespace namespace, boolean cascade)
+ throws NamespaceNotEmptyException {
+ if (cascade) {
+ return BaseSessionCatalog.this.dropNamespace(context, namespace, true);
Review Comment:
ah so this is calling the three arg version of dropNamespace. Got it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a diff in pull request #7275: Spark 3.3: drop_namespace with CASCADE support
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #7275:
URL: https://github.com/apache/iceberg/pull/7275#discussion_r1157745881
##########
core/src/main/java/org/apache/iceberg/catalog/BaseSessionCatalog.java:
##########
@@ -143,6 +143,16 @@ public boolean dropNamespace(Namespace namespace) throws NamespaceNotEmptyExcept
return BaseSessionCatalog.this.dropNamespace(context, namespace);
}
+ @Override
+ public boolean dropNamespace(Namespace namespace, boolean cascade)
+ throws NamespaceNotEmptyException {
+ if (cascade) {
+ return BaseSessionCatalog.this.dropNamespace(context, namespace, true);
Review Comment:
This is just an infinite regression right?
##########
core/src/main/java/org/apache/iceberg/catalog/BaseSessionCatalog.java:
##########
@@ -143,6 +143,16 @@ public boolean dropNamespace(Namespace namespace) throws NamespaceNotEmptyExcept
return BaseSessionCatalog.this.dropNamespace(context, namespace);
}
+ @Override
+ public boolean dropNamespace(Namespace namespace, boolean cascade)
+ throws NamespaceNotEmptyException {
+ if (cascade) {
+ return BaseSessionCatalog.this.dropNamespace(context, namespace, true);
Review Comment:
This is just an infinite recursion right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a diff in pull request #7275: Spark 3.3: drop_namespace with CASCADE support
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #7275:
URL: https://github.com/apache/iceberg/pull/7275#discussion_r1157746889
##########
core/src/main/java/org/apache/iceberg/hadoop/HadoopCatalog.java:
##########
@@ -349,6 +349,17 @@ public boolean dropNamespace(Namespace namespace) {
}
}
+ @Override
+ public boolean dropNamespace(Namespace namespace, boolean cascade)
+ throws NamespaceNotEmptyException {
+ if (cascade) {
+ // recursively delete all nested namespaces
+ listNamespaces(namespace).forEach(n -> dropNamespace(n, true));
Review Comment:
All of the other catalogs do not drop nested namespaces, do we need to cover this in other cases?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] abmo-x commented on a diff in pull request #7275: Spark 3.3: drop_namespace with CASCADE support
Posted by "abmo-x (via GitHub)" <gi...@apache.org>.
abmo-x commented on code in PR #7275:
URL: https://github.com/apache/iceberg/pull/7275#discussion_r1157906856
##########
core/src/main/java/org/apache/iceberg/hadoop/HadoopCatalog.java:
##########
@@ -349,6 +349,17 @@ public boolean dropNamespace(Namespace namespace) {
}
}
+ @Override
+ public boolean dropNamespace(Namespace namespace, boolean cascade)
+ throws NamespaceNotEmptyException {
+ if (cascade) {
+ // recursively delete all nested namespaces
+ listNamespaces(namespace).forEach(n -> dropNamespace(n, true));
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] abmo-x commented on a diff in pull request #7275: Spark 3.3: drop_namespace with CASCADE support
Posted by "abmo-x (via GitHub)" <gi...@apache.org>.
abmo-x commented on code in PR #7275:
URL: https://github.com/apache/iceberg/pull/7275#discussion_r1157909282
##########
aws/src/main/java/org/apache/iceberg/aws/dynamodb/DynamoDbCatalog.java:
##########
@@ -284,6 +284,15 @@ public boolean dropNamespace(Namespace namespace) throws NamespaceNotEmptyExcept
}
}
+ @Override
+ public boolean dropNamespace(Namespace namespace, boolean cascade)
+ throws NamespaceNotEmptyException {
+ if (cascade) {
+ listTables(namespace).forEach(this::dropTable);
Review Comment:
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 (https://github.com/apache/iceberg/pull/7275#discussion_r1157912588))
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org