Sorry I don't understand why number 2 in your first email doesn't work. That's what you need to use if you want a Hadoop config. It's the only FileIO that understands Hadoop. We have no reason to allow a Hadoop configuration for the other IOs.
On Sat, Nov 7, 2026 at 5:48 AM Sebastian Daberdaku < [email protected]> wrote: > Hi Dan and Russell, > > A proposal to close the API question: replace catalogName(String) with > hadoopConf(Configuration), with Dan's FileIO lookup as the default. I agree > with Russell that catalogName does not say what it controls. > > - hadoopConf(Configuration) on DeleteOrphanFilesSparkAction is the > Hadoop configuration the listing uses, replacing the session configuration > when set. RemoveOrphanFilesProcedure passes the session configuration plus > the spark.sql.catalog.<name>.hadoop.* overrides of the catalog it was > invoked on, which is what SparkUtil.hadoopConfCatalogOverrides already > builds for SparkCatalog. The action keeps wrapping it in > SerializableConfiguration and no longer knows about Spark catalog names. > - Default, per Dan's suggestion: when hadoopConf is not set and > table.io() is HadoopConfigurable, use the configuration it holds; > otherwise the session configuration as today. For tables loaded through > SparkCatalog that is the session configuration plus the catalog's hadoop.* > overrides, so HadoopFileIO and ResolvingFileIO users get the fix without > calling anything. This reads the configuration the FileIO was built with, > so there is no name-based catalog lookup and no catalog loading side > effect, which is what the PR review rejected. It is a change from what I > said on the PR, that nothing changes without opting in; I think it is > the right default since it is the configuration the table already reads and > writes with. Say so if you would rather keep it opt-in. > > It is still one public method, on the Spark action only, not on the > DeleteOrphanFiles interface. The procedure is in a different package, so it > has to be public. Going through the generic option(...) map was discussed > in the PR review and set aside as the same thing stringly typed, and the > action already takes an arbitrary location, so a typed setter for the > configuration it lists with adds no new capability. > > The default alone does not cover #17860: as in my last reply, S3FileIO, > GCSFileIO and ADLSFileIO are not HadoopConfigurable, so the listing would > still use the session configuration and fail with AccessDenied. The > spark.sql.catalog.<name>.hadoop.* overrides are the only per-catalog place > a user can configure the Hadoop FileSystem the listing uses, so the > procedure passes them. As agreed in the review, the fix stays limited to > those explicit overrides; hadoopConfCatalogOverrides keeps its contract and > the action does no credential derivation. prefix_listing does authenticate > through table.io(), but it lists serially on the driver into a single > partition (#16932, #17387), so it is not a drop-in for large tables; making > it parallel is a separate change. > > Does this shape work for you? If so, or if there are no objections by the > end of next week (November 15th), I will push it to #17862 this week, > retarget the PR to the 4.2 module, ask yangshangqing95 to re-review since > his approval was on the catalogName shape, and follow with the 4.1, 4.0 and > 3.5 backports. The action tests move from catalogName to hadoopConf, plus > one for the default path on a HadoopConfigurable FileIO, and > spark-procedures.md gets the default rule. If you would rather see > something else, a pointer before I rework it would help. > > Thanks, > Sebastian > > On Fri, Oct 30, 2026 at 7:34 AM Sebastian Daberdaku < > [email protected]> wrote: > >> Hi Daniel and Russell, >> >> Thanks for taking another look. I checked whether reading the >> configuration off the FileIO would work, and it runs into two problems: >> >> 1. S3FileIO (as well as GCSFileIO and ADLSFileIO) does not implement >> HadoopConfigurable, so there is no Hadoop configuration to read from >> table.io(). The setup in #17860 uses io-impl = S3FileIO, so for the >> reported case the check would fall through to the session configuration >> and >> the listing would still fail with AccessDenied. >> 2. Where the FileIO is HadoopConfigurable (HadoopFileIO, >> ResolvingFileIO), the configuration it holds is the one >> SparkCatalog.buildIcebergCatalog passes in, which is already >> SparkUtil.hadoopConfCatalogOverrides(spark, name). In those cases the two >> approaches produce the same configuration. >> >> The spark.sql.catalog.<name>.hadoop.* overrides are the only place a user >> can tell the Hadoop listing how to authenticate when the catalog's FileIO >> is not Hadoop-based, which is why the PR routes through them. >> >> Best, >> >> Sebastian >> >> On Fri, Oct 30, 2026 at 12:11 AM Russell Spitzer < >> [email protected]> wrote: >> >>> Sorry "bit disconnected" >>> >>> On Thu, Oct 29, 2026 at 6:10 PM Russell Spitzer < >>> [email protected]> wrote: >>> >>>> I would check out Dan's suggestion. We should try any solutions we can >>>> that avoid expanding the API, especially when the connection here between >>>> the behavior and the API is a bad disconnected. >>>> >>>> On Thu, Oct 29, 2026 at 4:53 PM Daniel Weeks <[email protected]> wrote: >>>> >>>>> Sebastian, >>>>> >>>>> It's been a little while since I looked at this, but wouldn't it make >>>>> more sense to interrogate the FileIO to see if it's HadoopConfigurable and >>>>> grab the exact configuration used by the IO rather than go through this >>>>> catalog path? >>>>> >>>>> At that point, you'd have the exact configuration and overrides for >>>>> that specific table. >>>>> >>>>> -Dan >>>>> >>>>> On Wed, Oct 28, 2026 at 1:15 AM Sebastian Daberdaku via dev < >>>>> [email protected]> wrote: >>>>> >>>>>> Hi all, >>>>>> >>>>>> I'd like to get some eyes on a PR that fixes a credentials mismatch >>>>>> in remove_orphan_files, and agree on the approach before I port it across >>>>>> Spark versions: >>>>>> >>>>>> https://github.com/apache/iceberg/pull/17862 (closes #17860) >>>>>> >>>>>> Problem >>>>>> >>>>>> Unless prefix_listing is enabled, DeleteOrphanFilesSparkAction lists >>>>>> the table location with a Hadoop FileSystem built from the Spark session >>>>>> configuration alone, while every other file operation in the action goes >>>>>> through table.io(). When a catalog reaches its storage with >>>>>> credentials other than the cluster's (e.g. a catalog that assumes a role >>>>>> in >>>>>> another AWS account), reads and deletes succeed but the listing fails >>>>>> with >>>>>> AccessDenied or silently sees no files. >>>>>> >>>>>> Change >>>>>> >>>>>> - DeleteOrphanFilesSparkAction gets a new catalogName(String) method. >>>>>> When set, the listing uses the session Hadoop configuration plus that >>>>>> catalog's spark.sql.catalog.<name>.hadoop.* overrides >>>>>> (SparkUtil.hadoopConfCatalogOverrides). When not set, behavior is >>>>>> unchanged. >>>>>> - RemoveOrphanFilesProcedure passes the catalog it was invoked on, so >>>>>> CALL <catalog>.system.remove_orphan_files(...) picks up that catalog's >>>>>> overrides with no new procedure argument. >>>>>> - spark-procedures.md documents which configuration the listing uses. >>>>>> >>>>>> The catalog is passed explicitly rather than inferred from >>>>>> Table.name(): as @yangshangqing95 pointed out in review, a table name's >>>>>> first segment isn't guaranteed to identify the Spark catalog it came >>>>>> from, >>>>>> and resolving it via CatalogManager can initialize unrelated catalogs as >>>>>> a >>>>>> side effect. >>>>>> >>>>>> Tests cover the procedure path across the test catalogs and the >>>>>> action API with both SparkCatalog and SparkSessionCatalog, using a marker >>>>>> FileSystem registered only in the catalog's hadoop.* overrides. >>>>>> >>>>>> Questions >>>>>> >>>>>> 1. Is a catalogName(String) method on the action an acceptable public >>>>>> API addition, or would people prefer a different shape? >>>>>> 2. The PR was opened against Spark 4.1 before the 4.2 module landed. >>>>>> If the direction is agreed, I'll move the change to 4.2 and backport it >>>>>> to >>>>>> 4.1, 4.0 and 3.5. >>>>>> >>>>>> Thanks, >>>>>> Sebastian Daberdaku >>>>>> >>>>> > *This communication, including any attachments, is intended solely for the > recipient(s) addressed. * > *If you have received this email in error, please notify the sender > immediately and delete it from your system. * > *Any unauthorized use, disclosure, or distribution, of this communication > is strictly prohibited.* >
