RussellSpitzer commented on code in PR #15614:
URL: https://github.com/apache/iceberg/pull/15614#discussion_r3320140310
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/SparkCatalog.java:
##########
@@ -393,6 +421,14 @@ public boolean purgeTable(Identifier ident) {
}
}
+ private boolean dropTableWithPurging(Identifier ident) {
+ if (isPathIdentifier(ident)) {
Review Comment:
This is still rather confusing from an API perspective. The purge parameter
is meaningless on the path-identifier branch (HadoopTables.dropTable ignores it
and always drops the warehouse directory - see the existing comment a few lines
above). So we have a helper whose second argument silently no-ops depending on
the first argument's runtime type.
I think you have a few options:
The clearest is probably to not use this helper at all in the new call site,
since we've already verified !isPathIdentifier(ident) in the guarding if:
```java
if (restCatalogPurge) {
return icebergCatalog.dropTable(buildIdentifier(ident), true);
} else {
LOG.warn(
"Set '{}' to true to use the REST Catalog's capabilities to purge the
table.",
RESTCatalogProperties.REST_CATALOG_PURGE);
}
```
Then catalogDropTable doesn't need to change at all and keeps its current
purge=false contract.
The other option is to add a precondition to prevent accidentally calling
this with a path identifier and purge=true:
```java
Preconditions.checkArgument(
!purge || !isPathIdentifier(ident),
"Cannot purge path-identifier tables via catalogDropTable");
```
Personally I would just go with the first option there
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
