perf: cache Iceberg FileIO per executor instead of building one per task

[mixermt]

Which issue does this PR close?

Closes #6105.

Rationale for this change

Every native Iceberg scan or write task builds its own FileIO. On a production workload that was 40,317 FileIO constructions against one endpoint for a single query, each one rebuilding the storage factory, its parsed configuration and, for S3 with a custom access provider, the JNI bridge. Spark's Hadoop layer shares one client per executor JVM. Details in #6105.

What changes are included in this PR?

load_file_io in iceberg_common.rs now serves clones of a per-executor LRU cache and builds only on a miss; the previous body becomes build_file_io. Clones share iceberg-rust's Arc<OnceLock<Arc<dyn Storage>>>, so all tasks on an executor share one Storage.

The key is everything that shapes the client: access mode, catalog name, the full reference path, and the whole catalog property bag. The path is included because the S3 access bridge is constructed with the bucket and url.path() of the reference location and the JVM provider is called with exactly that pair, so a bridge built for one table must not serve another. memory:/// is never cached; the write path assembles manifest bytes there per task. The cache holds 64 entries and evicts the least recently used one, dropping it after the lock is released since the last clone of a FileIO releases JNI global refs. release_runtime drains it with the Tokio runtime. A read whose configured S3 access provider failed to initialise falls back to opendal's default chain, as before; that build is returned but not cached, so the next task retries the provider instead of inheriting the fallback.

docs/source/contributor-guide/s3-credential-provider-design.md points the property filter at build_file_io and gains a section on the executor FileIO cache: what the key holds and why the reference path stays in it, what is never cached, and how long a bridge and its handle live.

What is actually shared, per backend

Sharing a FileIO shares what its Storage holds. At the pinned iceberg-rust rev that differs by backend:

Backend	Storage holds	Shared across tasks by this PR
s3, s3a, aliases, gs, oss	config and the access loader; create_operator builds a new opendal Operator per file open	factory and parsed config, and for a custom S3 provider the JNI bridge and its ensureInitialized. Not the client or signer: opendal builds a new Signer per operator, and HTTP pooling is already process-wide
hdfs (#5898)	config plus an operator cache keyed by NameNode	the hdfs-native client and its NameNode session, which is the case that motivated this change
memory	the operator itself	excluded from the cache on purpose
file	nothing	harmless

So for S3-family backends this PR is necessary but not sufficient for client reuse: an operator cache inside OpenDalStorage::S3, mirroring what the HDFS backend already does, would make the shared Storage also share the signer and its access cache. That is an iceberg-rust change, tracked in #6109. Without an executor-level FileIO cache such an operator cache would die with every task, which is why this lands first.

How are these changes tested?

• Unit tests in iceberg_common.rs: the key separates access mode, path, catalog and properties and is None for memory:///; cached_file_io builds once for repeated identical loads and every time without a key; load_file_io("memory:///") leaves the cache untouched; a degraded build is returned but never cached; eviction removes the least recently used entry and a re-insert replaces without evicting.
• cargo clippy --all-targets -p datafusion-comet -- -D warnings is clean.
• JVM, with the rebuilt library: CometIcebergNativeSuite, CometIcebergWriteActionSuite and the MinIO-backed IcebergReadFromS3Suite, which runs REST catalog vending with wrong access immediately before correct access against the same bucket and catalog.
• The manual CometS3CredentialBridgeSuite (MinIO, listed in dev/ci/check-suites.py), which drives a custom CometS3CredentialProvider on both the Parquet and Iceberg paths: 6 tests pass on the merged head, including the two-catalog isolation test and the location-scoped tests from fix: refresh S3 policy locations when a location's credential fails #6223.
• Not measured here: the end-to-end gain on the production workload.

AI Disclosure

Drafted, implemented and tested with AI assistance (Claude Code); reviewed before submission.

🤖 Generated with Claude Code

[mixermt]

Review of the first revision found three problems, all addressed in the latest push:

1. The description overclaimed for S3. At the pinned iceberg-rust rev OpenDalStorage::S3 holds only config and the access loader; create_operator builds a new opendal Operator, and with it a new Signer, on every file open, while HTTP pooling is already process-wide. A shared FileIO therefore reuses the factory, the parsed config and the JNI bridge, not the S3 client. Client reuse holds for the HDFS backend in feat(iceberg): support HDFS storage via iceberg-rust hdfs-native backend [NOT FOR MERGE: pending apache/iceberg-rust#3111] #5898, whose Storage caches operators per NameNode. The description now states this per backend, and an operator cache for the S3 family is noted as an iceberg-rust follow-up in Native Iceberg scan and write build a new FileIO, and a new storage client, for every task #6105.
2. The bound cleared the whole map, including the entry a running query was using, and dropped every FileIO while holding the lock. Replaced with LRU eviction of one entry, dropped after the lock is released.
3. The test never exercised a cache hit. Added tests that a repeated identical load builds once, that a load without a key builds every time, that memory:/// never enters the cache, and that eviction removes the least recently used entry.

[mixermt]

Second review round, three findings, all addressed in the latest push:

1. A read-side provider init failure became sticky. build_s3_credential_loader falls back to opendal's default chain on a read, and that FileIO was cached like a success, so later tasks never retried ensureInitialized. storage_factory_for and build_file_io now report whether the build is cacheable; a fallback is returned but not inserted, so the next task retries. Covered by cached_file_io_does_not_cache_a_degraded_build.
2. The S3 operator cache follow-up lived only in Native Iceberg scan and write build a new FileIO, and a new storage client, for every task #6105, which this PR closes. Split into Iceberg S3-family storage rebuilds the opendal Operator and signer on every file open #6109 and linked from the description.
3. Two comments still described per-task lifetimes. The FILE_IO_CACHE rustdoc now says what is shared at this pin, and the missing-expiry latch comment in credential_bridge.rs reflects that bridges live as long as their cache entry.

[andygrove]

Thanks for the careful per-backend breakdown in the description. It makes the scope of this change much easier to reason about.

HDFS support (#5898) isn't on main yet, so today the gain comes only from the S3 factory and bridge construction. CometS3CredentialDispatcher.ensureInitialized already caches the provider JVM-side via KEY_TO_HANDLE.computeIfAbsent, so the per-task bridge cost is two new_string calls, two global refs and a map lookup. The provider isn't re-initialised. Do you have measurements showing the per-task FileIO build cost matters for S3 on its own? If not, would it make sense to land this together with #5898 or #6109, where the cache is clearly needed?

[andygrove]

The key carries the whole catalog property bag, which can include vended s3.access-key-id, s3.secret-access-key and s3.session-token. Could we drop Debug here, or implement a redacting one, so a future {:?} can't leak credentials into logs?

[mixermt]

Replaced the derive with a manual Debug that prints access_mode, catalog_name and reference_path and omits properties. cache_key_debug_does_not_leak_properties asserts the exact output from a key built with vended secrets.

[sunchao]

Summary

• Prior state and problem: Each native Iceberg task rebuilt its FileIO, storage configuration, and optional JNI credential bridge.
• Design approach: Add a bounded, 64-entry executor cache keyed by access mode, catalog, full reference path, and all catalog properties.
• Correctness / compatibility analysis: No introduced P1/P2 issues found within this review. Checked configuration isolation, credential refresh behavior, ownership, and cleanup against the pinned Iceberg implementation and Spark 3.4.3, 3.5.9, 4.0.4, 4.1.3, and 4.2.0 sources.
• Key design decisions: Exclude memory storage and degraded credential-provider builds. Drop evicted values outside the mutex. The implementation stays local to the shared Iceberg helper.
• Implementation sketch: load_file_io checks the cache, calls build_file_io on misses, and returns clones sharing storage. release_runtime drains the cache.
• Behavioral changes worth calling out: S3 factory/configuration/bridge reuse does not reuse its per-file operator or signer. Cache lookup adds property copying, sorting, hashing, and locking. End-to-end performance was not measured.
• Suggested improvements: None meeting the requested P1/P2 threshold.

Reviewed full SHA 36ffdc1f57043043b0e6cc16e7b33096693a97b8 against base 9a4d5f28368d4fbb54ac23c14c5d52d5637ce26d, covering all three commits and the complete four-file PR diff using their merge base. The PR remains non-draft.

Routed skills: review-comet-pr and review-comet-ffi-pr. Read existing human reviews, issue comments, inline comments, and threads. No existing P1/P2 blocker was substantiated: closeAll() already documents test/shutdown-only use, HDFS is unsupported at this pin, and no cache-key logging was found.

Exact-head CI: Comet CI and CodeQL report action_required, with no jobs executed. Only labeling passed.

Validation: All 78 native Iceberg tests passed with --no-default-features. Five disposable cache-lifecycle/concurrency tests also passed using the exact cache implementation with Arc probes. JVM/MinIO, Spark SQL, upstream Iceberg suites, and default-feature builds were not validated here. No project code or GitHub state was changed.

[andygrove]

sunchao's right that closeAll() only runs from tests and the shutdown hook, so I'm fine with cached bridges keeping their handle. I'll take the Kerberos question to #5898, since HDFS only arrives there. The Debug derive on FileIoCacheKey still worries me, though. The key holds the whole catalog property bag, including vended s3.secret-access-key and s3.session-token, so one {:?} in a future log line leaks them. Could it get a Debug impl that prints only the catalog name, access mode and reference path? The tests' assert_ne! calls need Debug, which is why dropping the derive outright would mean rewriting them.

[mixermt]

Latest push replaces the derived Debug on FileIoCacheKey with a manual impl that prints only access mode, catalog name and reference path, with a test asserting the exact output from a key built with vended secrets. The bridge struct doc no longer describes a per-scan lifetime.

On measurements: none yet, and I would rather not invent one. ensureInitialized caches the provider, so the cache does not remove a second initialize. What each task repeated was marshalling the whole catalog bag into a Java map before the JVM cache is reached, four new_string calls plus two per property, two thread attaches, two global refs and a fresh FileIO. I would rather land this on its own: #5898 is blocked upstream, #6109 has to land in iceberg-rust, and its operator cache only outlives a task if this cache is already in place.

[andygrove]

Thanks for the Debug fix. That's exactly what I was after. And thanks for the answer on measurements. #6109's operator cache can't outlive a task without this, and #5898 is blocked upstream, so I'm happy for this to land on its own.

Two things before it goes in.

Could this PR also update docs/source/contributor-guide/s3-credential-provider-design.md? Line 109 says the storage-prefix filter is applied in iceberg_scan.rs::load_file_io, and it now lives in iceberg_common.rs::build_file_io. It would also help to add a short paragraph there about the new executor FileIO cache, since that page is where bridge lifetimes are explained for the Parquet path. The things worth capturing are what the key holds and why the reference path has to be in it, that memory:/// and degraded read builds are never cached, and that a bridge and its dispatcher handle now live as long as their cache entry. The path matters most. Dropping it from the key looks like a free hit-rate win, and it would hand one table's bridge to another.

Thanks for running IcebergReadFromS3Suite. That suite covers vended static credentials without a provider, though. The case where this cache saves real work today is a custom CometS3CredentialProvider on the Iceberg path, and the suite that drives it is CometS3CredentialBridgeSuite, including the two-catalog isolation test. It's a manual suite, so CI won't run it. Could you run it against this branch and add the result to the testing section?

[mixermt]

Docs updated: the filter reference now points at iceberg_common.rs::build_file_io, and a new section describes the executor FileIO cache: what the key holds and why the reference path stays in it, that memory:/// and degraded read builds are never cached, and that a bridge and its dispatcher handle live as long as their entry.

CometS3CredentialBridgeSuite on the merged head: 6 tests pass, including the two-catalog isolation test and the location-scoped tests from #6223. Added to the testing section.

[andygrove]

Thanks @mixermt