yangshangqing95 commented on code in PR #17862:
URL: https://github.com/apache/iceberg/pull/17862#discussion_r3895643720
##########
spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/actions/DeleteOrphanFilesSparkAction.java:
##########
@@ -222,6 +223,24 @@ public DeleteOrphanFilesSparkAction
usePrefixListing(boolean newUsePrefixListing
return this;
}
+ /**
+ * Configures the listing to use the Hadoop configuration of the given
catalog.
+ *
+ * <p>Listing the table location goes through the Hadoop {@link
org.apache.hadoop.fs.FileSystem}
+ * API, which is configured from the session and so reaches storage as a
different principal than
+ * the catalog does. Setting the catalog makes the listing follow the
catalog's own configuration
+ * instead.
+ *
+ * @param newCatalogName the name of the catalog that holds the table
+ * @return this for method chaining
+ */
+ public DeleteOrphanFilesSparkAction catalogName(String newCatalogName) {
Review Comment:
Thanks, I agree that routing this through option(...) wouldn't really be
better, and the existing arbitrary location option makes my original concern
about exposing catalogName(...) less compelling.
I'm less comfortable with inferring the catalog from table.name(), though. I
don't think Table.name() provides a contract that it is a Spark qualified
identifier or that its first segment identifies the Spark catalog that produced
the table. Table.name() is ultimately just a string (and defaults to
toString()), while custom Iceberg catalog implementations are free to return
tables with different naming conventions.
I also don't think Spark3Util.catalogAndIdentifier is quite the same
precedent: it resolves a multipart identifier parsed by Spark and then asks
CatalogManager whether the first identifier part is a catalog. Here we'd be
using a table's display/full name to infer provenance.
There is also a possible false-positive case: a table supplied directly to
SparkActions.deleteOrphanFiles(table) could have a name beginning with a
registered Spark catalog name without having been loaded from that catalog. In
that case we'd silently apply that catalog's Hadoop configuration to an
unrelated table. isCatalogRegistered verifies that the name exists, but not
that the table came from it.
Given that this is a delete action, I'd prefer explicit catalog context over
inferring credentials/configuration from the table name. With your point about
the existing location option, I'm okay with keeping the typed
catalogName(String) method rather than going through the generic option map.
--
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]
