feat: per-location credentials for the S3 credential SPI

[snmvaughan]

Which issue does this PR close?

Closes #6207, which proposes the new @Public API for agreement, as the versioning policy asks.

Rationale for this change

On the Parquet path, object_store::CredentialProvider::get_credential() receives no request path, so one S3 store presents one credential, and Comet caches one store per (scheme://bucket, config_hash, backend). A CometS3CredentialProvider therefore gets one credential per bucket, requested with the path of the first file Comet reads. Vendors whose policies differ by location within a bucket (one STS session for warehouse/sales, another for warehouse/finance) get 403s on every location but the first.

What changes are included in this PR?

• CometS3LocationScopedCredentialProvider (new @Public opt-in extension of CometS3CredentialProvider) with one method, List<String> getPolicyLocations(String bucket), returning every location in the bucket that has its own policy. Each request uses the credential of the longest location covering its path, matched one segment at a time after percent-decoding; the bucket root is an implicit location. Comet requests a location's credential with the location itself as the path. Locations apply to native Parquet reads; the Iceberg path is unchanged.
• CometS3CredentialDispatcher.getPolicyLocations returns null for providers that do not implement the interface, without calling them, and otherwise a String[] copy, so provider list code runs inside the checked JNI call. A null list or location throws.
• LocationScopedObjectStore (native): registered and cached once per bucket like any S3 store. It routes each request to its location's store and builds each location's store on first use from a template whose region was resolved up front, so nothing calls block_on inside an async read. On a 403 it fetches the locations again, once for all the reads routed from the same snapshot whether the fetch succeeds or fails, and retries each read once if its path now routes elsewhere.
• Base providers are unaffected: create_store builds the same store as before with the same calls in the same order; the only addition is one dispatcher call on a cache miss that does not reach the provider.
• Removes RetryOn403ObjectStore, the scope-hint JNI entry point, and the multi-entry object store cache from the earlier revision.
• Docs: user guide section, design notes, and the versioning policy's public API list.

How are these changes tested?

• Rust unit tests (parquet::objectstore::location_scoped):
  • Routing: longest covering location, segment boundaries (sales vs sales_eu), percent-decoding (Spark's %3A escapes), duplicate and root locations, and invalid locations (empty and .. segments, URIs, control characters, invalid UTF-8).
  • Reads through one store: three locations in rotating order (A/B/A), with lazy store construction.
  • 403 handling: unchanged locations; a location added after the snapshot; a retry that is also denied; concurrent 403s on get_opts and get_ranges sharing one refresh; reads on separate threads waiting for a refresh already in progress; concurrent reads sharing one failed refresh; a later read asking again; a failed store build reported as itself.
  • The multi-threaded test fails when the refresh mutex is removed, and the failed-refresh test fails when failures are not shared.
• parquet::objectstore::s3: building a store from a template inside the Tokio runtime with no endpoint or region configured.
• JUnit (CometS3LocationScopedCredentialProviderTest): base providers return null without being called; empty, null, null-element, non-String, lazy-list and provider exceptions; unknown handles.
• CometPublicApiSuite pins the new @Public type.
• CometS3CredentialBridgeSuite (MinIO, manual suite): a dedicated bucket with a per-bucket provider override reads files from four locations, including the bucket root, in one partition and asserts the credential path each read requested; a second test asserts that scans after a warm-up scan reuse the bucket's locations without asking again. The existing base-provider test now also asserts the base provider still receives a file path. The suite needs Docker and has not been run on this revision yet.

cargo fmt, cargo clippy --all-targets --workspace -- -D warnings, cargo test -p datafusion-comet --lib (438 passed), the JUnit tests and CometPublicApiSuite above, spotless, scalastyle, and Prettier pass locally.

[sunchao]

Correctness

Reviewed 8a86d804 against base 5705a58ac.

The existing bucket-level cache can bind all reads to the first file's credential scope. This change adds an opt-in Java scope hint, stores several scope entries per existing configuration/backend key, chooses the longest covering prefix, and rebuilds a store after a 403. Keeping configuration and backend identity in the outer key preserves the existing separation of explicitly configured credentials and endpoints.

Four P2 findings are reproduced at the assigned head: recovery redirects the original scope to the replacement store, documented prefixes do not match native URL paths, an overlapping 403 can skip its retry, and region discovery can re-enter Tokio during async recovery. The first issue also affects a single partition containing files from several scopes because the planner prepares one store using its first file.

I compared the maintained Spark 3.5 and 4.0 Parquet reader paths at 5947fd6e and 03f28fc4. They initialize readers for the individual file split and its Hadoop context. Comet must continue to read each authorized file and surface genuine authorization failures. This PR changes credential routing and errors, not SQL type conversion, null handling, ANSI behavior, or arithmetic boundaries. Maintained 3.4 and 4.1 source branches were unavailable, so those versions remain unqualified.

Validation

The offline Rust fixture copied the production cache-matching and retry methods verbatim, used synthetic stores, and linked cached once_cell 1.21.4 and Tokio 1.53.1. It reproduced all four conditions, with controls showing that the original A store and the fresh concurrent-request store still authorize the failed paths. The nested-runtime probe uses a ready synthetic region future. No AWS, credentials, JNI execution, full Comet build, Spark suite, or Minio suite was involved. The author's test claims are separate from this evidence.

At the final cutoff, only the label check had succeeded. Comet CI and CodeQL were awaiting approval with zero product jobs executed. The new same-scope integration test also expects no additional policy callback, while production explicitly calls the callback before every cache lookup. Its URI-shaped hints and that assertion should be reconciled with the intended contract when adding end-to-end regression coverage.

Performance

The prefix mismatch prevents cache reuse for conforming nonempty hints. The component fixture measured zero hits in 1,000 same-scope lookups, versus 1,000 hits for the leading-slash format used by the Rust tests. This is a functional hit-count comparison, not an end-to-end latency measurement.

The exact lookup helper's median time for 20,000 lookups per sample, across three samples, was about 2 ns with one entry, 126 ns with 64 entries, and 2,378 ns with 1,024 entries for a hit. Misses had similar costs. These local synthetic timings show the linear scan cost only. They exclude JNI, credential vending, network I/O, and store construction. Scope hints and bridge/global-reference construction also run before cache hits. A focused native/JVM same-scope hit and concurrent-miss benchmark would establish whether that repeated work is justified.

Design

The opt-in sub-interface is a useful compatibility boundary, and longest-prefix selection can let disjoint scopes coexist. The store's mutable backing identity currently undermines that model: an entry retains its original scope while its wrapper starts using credentials for another scope. A path-aware selector over stable scoped stores would keep the routing rule in one place and allow one retry per operation without a process-lifetime retry latch.

Empty hints and policy-fetch errors become catchall entries. They need the same multi-scope recovery guarantees as nonempty hints. The implementation also wraps base-interface providers, despite the user guide saying their retry path stays inactive. The documentation should describe the final behavior consistently. The retained vector is process-wide and grows with distinct scope lists, so the old bucket-count-only size argument no longer describes its bounds. I found no newly demonstrated cross-user or cross-catalog credential disclosure and do not make that claim.

Abstraction & complexity

The cache helper, SPI bridge, and retry decorator each have a clear purpose, but scope selection and replacement ownership are split across the cache and wrapper. Consolidating path-based selection would remove the contradictory lifetime rules highlighted above. The appended rebuilt entry is also a raw store, so recovery behavior differs depending on how an entry was created. Please exercise the final routing design through actual scoped read sequences and concurrent failures, in addition to the current fake-store unit tests.

[sunchao]

Correctness

[P2] Could the rebuilt store be selected by request path without permanently replacing the backing store for every operation on this wrapper? The planner constructs one store from the first file and uses it for all files in a Parquet partition. With an A-only initial store and a B-only rebuilt store, reading A1, B1, then A2 returns success, success, then 403. The original store still authorizes A2, but current() now routes it to B and already_rebuilt() suppresses recovery. Appending a new scope entry does not repair the original cache entry or scans already holding this wrapper. I reproduced that sequence using these exact methods and synthetic scoped stores. Please retain path-specific routing and bound retries per operation, with A/B/A and three-disjoint-scope coverage.

[andygrove]

Agreed, and I think this constrains where the fix can live. A path-aware selector inside the cache wouldn't be consulted at read time: prepare_object_store_with_configs registers one store per ObjectStoreUrl, and that URL carries scheme, config hash, backend and host but no scope. DefaultObjectStoreRegistry::register_store is a plain insert, so two NativeScan operators on the same bucket under different scopes overwrite each other, and FileScanConfig::open resolves from that single URL at execute time. That's also why the one-file-per-partition case you mention can't be fixed in the cache -- a file group holds many paths but only one URL.

Would it work to make the registered store itself the selector? One wrapper per (bucket, config_hash, backend) holding the scoped stores and dispatching on location inside get_opts and get_ranges would put routing in one place, give one retry per operation with no process-lifetime latch, and handle a partition that spans scopes. The alternative is splitting file groups by scope and registering a URL per scope, which looks considerably more invasive.

Either way, the comment above register_object_store currently documents the invariant that Vec<ScopeEntry> breaks, so that needs updating too.

[sunchao]

Correctness

[P2] Could both sides be canonicalized to the bucket-relative representation required by CometS3ScopedCredentialProvider before this comparison? requested_path is taken directly from url.path(), so it starts with /, while the Java contract explicitly requires prefixes without a leading slash. A conforming hint such as warehouse/table therefore never covers /warehouse/table/file. With only that scoped entry present, the exact lookup helper produced zero hits in 1,000 requests, causing a new store and HTTP pool to be constructed on each preparation. The absolute S3 URI used by the new Minio test also never matches. The comparison should enforce the documented path-component boundary too, so table cannot match table_extra. Please exercise the Java contract's prefix format through the native cache.

[sunchao]

Correctness

[P2] Could this decision distinguish a 403 from the original store from a 403 returned by the rebuilt store? Two requests can capture the original store before either completes. If the first finishes, rebuilds successfully, and retries, the second then observes already_rebuilt() == true and returns its original 403 without trying the fresh store. A deterministic interleaving of these exact methods reproduced one successful request and one failed request, even though a later call for the failed path succeeds on the rebuilt store. OnceCell prevents duplicate construction but does not make this guard safe. Please let each operation retry once against the published replacement and cover the same race in get_ranges.

[sunchao]

Correctness

[P2] Could recovery reuse the resolved region or perform the rebuild without synchronously entering Tokio? This closure is invoked inside the async get_opts/get_ranges operation. For an ordinary AWS bucket with neither fs.s3a.endpoint nor fs.s3a.endpoint.region configured, create_store_with_bridge calls get_runtime().block_on(resolve_bucket_region(bucket)). Tokio rejects that nested runtime entry with a panic, including when the region cache would return immediately. I verified the latter with the locked Tokio 1.53.1 and a ready cached-region future, without making an AWS request. Thus the first recoverable 403 can abort the scan instead of retrying. Please cover recovery without an explicit endpoint or region.

[andygrove]

Thanks for working on this. sunchao's four findings all look right to me, so I won't repeat them. A few additional things below.

One process note first: Preflight is currently failing on the markdown formatting check, and that gates everything downstream. PR Build, the Spark SQL matrix and the Iceberg suites are all skipped, so nothing has actually been built or tested on this change yet. npx prettier "**/*.md" --write fixes both of the changed docs.

[andygrove]

This dedup check means the append that the whole recovery story depends on usually doesn't happen. In the overreporting case the vendor returns the same over-broad prefix on the rebuild as it did on the initial fetch, so fresh_prefixes equals the existing entry's prefixes, the check fires, and nothing gets pushed. The wrapper is left silently rebound to the new store with no entry recording it, which is the state sunchao describes on the retry wrapper. Should the dedup key on something that tells the two sessions apart rather than on the prefix list?

[andygrove]

Every other JNI call in this file goes through jni_static_call!, which runs check_exception for you. This helper calls call_method_unchecked directly, so if a vendor hands back a lazily evaluated List that throws from size() or get(i), the exception stays pending and the next JNI call on that thread misbehaves. Could it use the checked path instead?

Separately, line 448 casts each element straight through JString::from_raw with no type check, while the doc comment above says non-string elements are skipped. Worth making the code match the comment, since a vendor returning a List<Object> would currently be undefined behavior rather than a skipped element.

[andygrove]

docs/source/about/versioning_policy.md enumerates the public API as well, and it still lists only the four pre-existing types. The comment at the top of this suite asks for the doc and this list to change together, so could CometS3ScopedCredentialProvider be added there too? It would also help to say what adding a method to the sub-interface later would mean for compatibility, since the existing text covers that question for CometS3CredentialProvider.

[andygrove]

Two things on this paragraph. The registry isn't keyed as (bucket, config_hash, backend, scope_prefixes) -- the design doc has a whole section explaining why that shape was rejected, and the code keeps the three-tuple with the prefixes hanging off the entry. And the rebuild doesn't widen to a catchall; it re-fires the SPI and uses whatever prefixes come back.

Together with the point sunchao raised about base-interface providers being wrapped despite this page saying their retry path stays inactive, that's three claims on this page that need reconciling with the final behavior. The backward compatibility promise rests on the third one, so I'd like that one to be accurate in particular.

[andygrove]

Beyond the URI-shaped hint and the callback assertion sunchao flagged, there's a third thing working against this test. The native object_store cache is a process-wide static and MinioCometS3CredentialProvider defaults to an empty hint, so the earlier tests in this suite have already seeded a catchall entry under the same bucket key. find_matching_scope matches that at effective length 0, so the scoped entry never gets exercised even once the prefix format is fixed.

Would using a bucket the other tests don't touch be enough here, or is there a way to reset the native cache between tests?

[sunchao]

Follow-up at 8a86d804 against 5705a58ac: the reviewed source pair is unchanged, and the four P2 findings in the previous review remain.

One qualification to the registry-overwrite discussion: the current Parquet path captures the selected store in its reader factory, and each reader clones that retained store. Locked DataFusion 55.1.0 uses this supplied factory instead of its default factory. Replacing the registry entry therefore does not by itself retarget an already-created Comet Parquet reader. The earlier A/B/A finding remains because the retained retry wrapper changes its own backing store.

I corroborated the existing JNI exception comment: an exception from a lazy list's size() or get() remains pending on an already-attached JVM thread while the caller falls back to an empty scope. The JNI local frame releases references but does not clear that exception. The prefix-list deduplication, API documentation, user documentation, and cache-isolation comments are also supported by the source. The scoped tests need an isolated cache key and assertions about the selected store or scoped access, since policy callback counts alone do not establish routing.

Preflight now fails Prettier on both changed S3 credential documentation files. All product build, Spark, and benchmark jobs were skipped. CodeQL succeeded only for Actions. The logged CI checkout has the same tree as the reviewed head.

This follow-up used source and locked-dependency checks. I did not rerun the earlier synthetic Rust probes or execute JNI, Spark, or Minio tests. Maintained Spark 3.4 and 4.1 source branches remain unavailable.

[snmvaughan]

Thanks both. The findings were right, and most of them came from one design choice: the store used for a request was decided by a mutable, process-lifetime latch inside the retry wrapper, and scopes were discovered one session at a time from 403s. I've replaced that design rather than patching it, in cb1bdec.

New design. The opt-in interface is now CometS3LocationScopedCredentialProvider.getPolicyLocations(String bucket): the provider lists every location in the bucket that has its own policy, instead of reporting what one session covers. As andygrove suggested, the registered store is the selector: LocationScopedObjectStore is cached and registered once per (bucket, config_hash, backend), holds a store per location, and routes each request by location to the longest covering location. The bucket root is an implicit location.

How each finding is addressed:

• A/B/A and partitions spanning scopes (sunchao): routing is per request with no mutable backing store. There's a test that reads three locations in rotating order through one store.
• Prefix format and segment boundary (sunchao): locations and request paths are both canonicalized with Path::from_url_path and matched segment by segment, so table doesn't cover table_extra. Tests cover the contract's format, slash variants and %-escapes.
• Concurrent 403s (sunchao): a 403 re-fetches the locations, then retries once if the route changed. The retry budget is per request, with no latch. Every refresh attempt starts a new snapshot generation, so requests routed from the same snapshot share one attempt, including a failed one. Tests cover two in-flight get_opts and get_ranges requests failing together, reads on separate threads waiting for a refresh in progress, and a shared failed refresh.
• Nested runtime (sunchao): the region is resolved when the bucket's store is created. Location stores are built from that template and never block_on, which a test checks inside the runtime.
• Dedup (andygrove): gone. Nothing is appended on recovery; recovery only re-reads the provider's list.
• JNI (andygrove): the dispatcher returns a String[] copy. List code runs inside the checked jni_static_call!, non-String elements fail with ArrayStoreException in Java, and native frees each element's local ref.
• Registry invariant (andygrove, sunchao): one store per key again, so the register_object_store comment holds as written.
• Base providers (sunchao, andygrove): unaffected. The dispatcher returns null without calling them, and create_store builds the same store as before. The user guide now says exactly that.
• Versioning policy (andygrove): the new type is listed, along with what adding a method or changing how paths match locations would mean. The policy also asks for the addition to be agreed in an issue first; I'll open one and link it from the PR.
• Docs (andygrove): the user guide and design notes are rewritten against the new behavior, and Prettier passes.
• Integration tests (andygrove, sunchao): a dedicated bucket with a per-bucket provider override gives the scoped tests their own cache key. They assert which credential path each read requested, with four locations read in one partition, rather than counting callbacks; the reuse test runs a warm-up scan first so it does not depend on test order. The suite needs Docker, and I haven't run it on this revision yet.

The PR description is updated. Could a maintainer approve the workflow runs for this revision? Comet CI and CodeQL are waiting for approval. Happy to walk through any of it.

[andygrove]

Each location's bridge goes through CometS3CredentialBridge::new again, so it only lands on the bucket's registration because this &HashMap::new() matches what create_store passes. #6023 changes create_store to forward the fs.s3a.* map. In a trial merge of the two, s3.rs only conflicts in create_store, so this line survives unchanged. After that, ensureInitialized would miss the existing key and create a second provider with an empty map. It would do that on whatever thread first reads the location, which is often a Tokio worker with no context class loader, so a vendor jar that comes in through --jars can't be loaded when Comet is on extraClassPath.

Could the location bridges be derived from bridge instead, reusing its handle and bucket string and only creating a new path string? They would then share the registration by construction, whatever create_store passes, and we'd skip an ensureInitialized round trip per location.

This doesn't need to hold up this PR. We can file a follow-on issue for it, as long as the fix lands before or together with #6023.

[andygrove]

Thanks, deriving from the bucket's bridge closes this. The location bridges now share the handle by construction. I also checked that the bucket bridge is always Read in create_store, so the inherited mode matches what the factory passed before.

[andygrove]

The refresh only fires on a 403 from S3. A provider with no policy for the path it's asked about will usually throw instead, and get_credential turns that into a Generic error, which skips the refresh. I tried two cases with a fake location store that returns that error, and both fetched the locations zero times across three reads. In the first, the provider has no bucket-wide policy and throws for /, then adds warehouse/finance mid-job. Reads under it route to / and keep failing, so the new location is never picked up. In the second, the provider folds warehouse/finance into warehouse and throws for the old location. Later reads under it keep failing, while a freshly built store serves the same path from /warehouse. The store lives as long as the executor, so both stay broken until a restart.

The user guide says a location added while a job runs is picked up, and suggests keying the provider's cache on the location, which leads naturally to that throw. Should a credential failure from a location's store trigger the same refresh as a 403? Or should the contract require getCredentialsForPath to answer for / and for locations the provider has dropped, and the guide say so?

This one can also be a follow-on issue rather than blocking the PR.

[andygrove]

Thanks @snmvaughan

[sunchao]

Summary

• Prior state and problem: A bucket’s cached S3 store requested credentials using its first file’s path, preventing reads across independently authorized locations.
• Design approach: The opt-in CometS3LocationScopedCredentialProvider supplies policy locations. One bucket-level wrapper routes each read to the longest matching location after percent-decoding and segment comparison.
• Correctness / compatibility analysis: The rewrite addresses the earlier A/B/A routing, prefix matching, concurrent-403, nested-runtime and JNI concerns. I compared Spark’s SparkPath and V1/V2 Parquet readers across 3.4.3, 3.5.9, 4.0.4, 4.1.3 and 4.2.0. The per-file routing and decoding match those paths. No additional introduced P1/P2 issues found within this review; the existing credential-error refresh concern remains reproducible.
• Key design decisions: Location bridges reuse the bucket’s provider registration, and region resolution precedes asynchronous reads. Keeping routing inside the registered store removes the previous mutable backing-store identity and simplifies ownership.
• Implementation sketch: Java validates and copies locations into String[], JNI retrieves them through the checked call, and native code combines a location index, lazy stores and serialized refresh generations. The reuse test served six reads using three store constructions and no policy refreshes. This validates reuse, not end-to-end performance.
• Behavioral changes worth calling out: Scoped providers receive a location path or /. A 403 refreshes locations and permits one retry when the route changes. Base providers retain their existing behavior, and Iceberg does not use location routing.
• Suggested improvements: Address the existing credential-error refresh concern. At location_scoped.rs:130, only PermissionDenied triggers refresh. A provider exception becomes Generic, so an added location with no root credential, or a removed location whose provider now throws, remains unreadable through the cached store. The bounded reproduction exercised both get_opts and get_ranges: three failures, zero location refreshes, and a successful read through a fresh store. Recognize credential-fetch failures explicitly and apply the same bounded refresh and changed-route retry. This is existing feedback, so no duplicate finding is added.

Reviewed the entire 16-file diff from base 5705a58ac2ef7e6b2674c87f3b495f789ebfcc8d to head 13e6344631fef807166f3b5b94efcfc0fab31f41. The PR remained non-draft. Read AGENTS.md and the discussion snapshot, excluding Copilot. Routed skill: review-comet-pr; no sibling skill applied.

Exact-head CI: the label check succeeded. Comet CI and CodeQL remain action_required, each with zero jobs. There is no product-build or test verdict.

Validation: all 16 tests from the unchanged routing module passed in an isolated crate using the locked direct dependency versions, including object_store 0.13.2 and Tokio 1.53.1. Two additional credential-error probes confirmed the existing concern without network requests. All nine dispatcher JUnit tests passed using Java 21 with source/target 17. Java 17 release checking was unavailable. Full Comet builds, integrated JNI execution, Spark SQL, MinIO and end-to-end benchmarks were not run. Project files remain unchanged.

[sunchao]

Summary

• Prior state and problem: One cached S3 store used the first file’s credential scope for the whole bucket, preventing reads across independently authorized locations.
• Design approach: The opt-in CometS3LocationScopedCredentialProvider supplies policy locations. A bucket-level wrapper routes each read to the longest matching location.
• Correctness / compatibility analysis: The current implementation addresses the earlier routing, concurrent-403, nested-runtime and JNI findings. I compared URI handling and per-file Parquet reads against verified Spark sources for 3.4.3, 3.5.9, 4.0.4, 4.1.3 and 4.2.0. No additional introduced P1/P2 issues found within this review. The existing credential-error refresh defect remains reproducible.
• Key design decisions: Location bridges share the bucket’s provider registration, and region resolution precedes asynchronous reads. Keeping routing inside one wrapper simplifies ownership and removes the previous mutable backing-store identity.
• Implementation sketch: Java validates and copies locations into String[], checked JNI retrieves them, and native code maintains a location index, lazy stores and serialized refresh generations. The reuse test served six reads with three store constructions and no policy refreshes. This establishes reuse, not end-to-end performance.
• Behavioral changes worth calling out: Scoped providers receive a location path or /. A 403 refreshes locations and allows one retry when the route changes. Base providers retain their existing behavior. Iceberg does not use location routing.
• Suggested improvements: Address the existing credential-error refresh concern. At native/core/src/parquet/objectstore/location_scoped.rs:130, only PermissionDenied triggers refresh. Provider exceptions become Generic, so adding a location when no root credential exists, or removing a location whose provider subsequently throws, leaves authorized files unreadable through the cached store. Both get_opts and get_ranges reproduced three failures with zero refreshes, while a fresh store succeeded. Recognize credential-fetch failures explicitly and apply bounded refresh and changed-route retry. This remains an existing P2 blocker and is not duplicated as a new finding.

Reviewed the entire 16-file diff from base 5705a58ac2ef7e6b2674c87f3b495f789ebfcc8d to head 13e6344631fef807166f3b5b94efcfc0fab31f41. The PR remains non-draft. Read AGENTS.md and all supplied discussion and reviews, excluding Copilot. Routed skill: review-comet-pr. No sibling skill applied.

Exact-head CI: the label check passed. Comet CI and CodeQL remain action_required, each with zero jobs. There is no product-build or test verdict.

Validation: reran all 16 routing tests using a byte-identical production module in an isolated crate with matching locked direct dependency versions. Both additional credential-error probes confirmed the existing defect without network requests. Recompiled the dispatcher tests with Java 17 and --release 17; all nine passed. Full Comet builds, integrated JNI execution, Spark SQL, MinIO and end-to-end benchmarks were not run. Project files remain unchanged.

[sunchao]

Summary

• Prior state and problem: One cached S3 store used the first file’s credential scope for the bucket, preventing reads across independently authorized locations.
• Design approach: The opt-in CometS3LocationScopedCredentialProvider supplies policy locations. A bucket-level wrapper routes each request to its longest covering location.
• Correctness / compatibility analysis: The rewrite addresses the earlier routing, concurrent-403, nested-runtime and JNI findings. I compared URI handling and per-file Parquet reads against verified Spark sources for 3.4.3, 3.5.9, 4.0.4, 4.1.3 and 4.2.0. No additional introduced P1/P2 issues found within this review. The existing credential-error refresh defect remains reproducible.
• Key design decisions: Location bridges share the bucket’s provider registration, and region resolution precedes asynchronous reads. Keeping routing inside one wrapper simplifies ownership and removes mutable backing-store identity.
• Implementation sketch: Java validates and copies locations into String[], checked JNI retrieves them, and native code combines a location index, lazy stores and serialized refresh generations. The reuse test serves six reads with three store constructions and no policy refreshes. This establishes reuse, not end-to-end performance.
• Behavioral changes worth calling out: Scoped providers receive a location path or /. A 403 refreshes locations and permits one retry when the route changes. Base providers retain their behavior. Iceberg does not use location routing.
• Suggested improvements: Address the existing credential-error refresh concern. At native/core/src/parquet/objectstore/location_scoped.rs:130, only PermissionDenied triggers refresh. Provider exceptions become Generic, so adding a location without a root credential, or removing a location whose provider subsequently throws, leaves authorized files unreadable through the cached store. Both cases reproduced three failures with zero refreshes through each of get_opts and get_ranges, while a fresh store succeeded. Recognize credential-fetch failures explicitly and apply bounded refresh with changed-route retry. This remains an existing P2 concern and is not duplicated as a new finding.

Reviewed the entire 16-file diff from base 5705a58ac2ef7e6b2674c87f3b495f789ebfcc8d to head 13e6344631fef807166f3b5b94efcfc0fab31f41. The PR remains non-draft. Read AGENTS.md and supplied reviews, comments and threads, excluding Copilot. Routed skill: review-comet-pr. No sibling skill applied.

Exact-head CI: Comet CI and Required Checks now pass, including native tests, all four Linux Spark 4.1 Comet suites, and TPC-H/TPC-DS result verification. CodeQL also passes. Jobs tested merge commit d3211cdcc9e83a26941ae5c8e3dcad077bb12628 with newer main. Fourteen of the sixteen changed files match the pinned head, including the routing, bridge, dispatcher and tests. Spark SQL, Iceberg, macOS and benchmark jobs were skipped.

Validation: reran all 16 routing tests using a byte-identical production module in an isolated crate with matching locked direct dependency versions. Both credential-error probes confirmed the existing defect without network requests. Recompiled the dispatcher tests using Java 17 and --release 17; all nine passed. No local full Comet build, integrated JNI/MinIO run, Spark SQL suite or end-to-end benchmark was performed. Project files remain unchanged.

[andygrove]

Thanks @snmvaughan. Deriving the location bridges from the bucket's bridge closes the interaction with #6023. I checked 13e634463 locally, merged with main. The location_scoped, objectstore and parquet_support tests pass, and clippy and fmt are clean.

The refresh concern on location_scoped.rs:130 still reproduces, and I filed #6221 for it. I don't think it needs to hold up this PR. It is not a regression, because only providers that implement the new interface take the new path, and nobody does yet. It fails loudly, with no wrong results and no fallback to a broader credential, and a one-off provider error doesn't stick. What persists is stale routing after a vendor's locations change while the executors are up, until they restart.

I would like the fix to land in the same release that ships the interface, though. Otherwise vendors building against it would have to learn the workaround, and the guide's "picked up" claim wouldn't hold for a provider without a bucket-wide credential. Would you be able to pick up #6221?

[andygrove]

Thanks @snmvaughan. Deriving the location bridges from the bucket's bridge closes the interaction with #6023. I checked 13e634463 locally, merged with main. The location_scoped, objectstore and parquet_support tests pass, and clippy and fmt are clean.

The refresh concern on location_scoped.rs:130 still reproduces, and I filed #6221 for it. I don't think it needs to hold up this PR. It is not a regression, because only providers that implement the new interface take the new path, and nobody does yet. It fails loudly, with no wrong results and no fallback to a broader credential, and a one-off provider error doesn't stick. What persists is stale routing after a vendor's locations change while the executors are up, until they restart.

I would like the fix to land in the same release that ships the interface, though. Otherwise vendors building against it would have to learn the workaround, and the guide's "picked up" claim wouldn't hold for a provider without a bucket-wide credential. Would you be able to pick up #6221?

I'm going to create a PR now to fix the issue

[sunchao]

Summary

• Prior state and problem: One cached S3 store used the first file’s credential scope for the bucket, preventing reads across independently authorized locations.
• Design approach: CometS3LocationScopedCredentialProvider supplies policy locations. A bucket-level wrapper routes each request to its longest covering location after percent-decoding and segment comparison.
• Correctness / compatibility analysis: The rewrite addresses the earlier routing, concurrent-403, nested-runtime and JNI findings. I verified relevant Spark sources for 3.4.3, 3.5.9, 4.0.4, 4.1.3 and 4.2.0 and compared URI handling and per-file Parquet reads. No additional introduced P1/P2 issues found within this review. The previously reported credential-error refresh defect remains reproducible.
• Key design decisions: Location bridges share the bucket’s provider registration, and region resolution happens before asynchronous reads. Keeping routing inside one wrapper simplifies ownership and removes the earlier mutable backing-store identity.
• Implementation sketch: Java validates and copies locations into String[], checked JNI retrieves them, and native code maintains the location index, lazy stores and serialized refresh generations. The reuse test serves six reads with three store constructions and no policy refreshes. This demonstrates reuse, without establishing end-to-end performance.
• Behavioral changes worth calling out: Scoped providers receive a location path or /. A 403 refreshes locations and allows one retry when the route changes. Base providers retain their behavior, and Iceberg does not use location routing.
• Suggested improvements: Complete the existing credential-error refresh fix tracked in #6221. At native/core/src/parquet/objectstore/location_scoped.rs:130, only PermissionDenied triggers refresh. Provider exceptions become Generic, so adding a location without a root credential, or removing a location whose provider subsequently throws, leaves authorized files unreadable through the cached store. Both cases reproduced three failures and zero refreshes through each of get_opts and get_ranges, while a fresh store succeeded. Recognize credential-fetch failures explicitly and apply bounded refresh with changed-route retry. Maintainers accepted follow-up handling, but this P2 concern remains unresolved at the reviewed head and is not duplicated as a new finding.

Reviewed the entire 16-file diff from base 5705a58ac2ef7e6b2674c87f3b495f789ebfcc8d to head 13e6344631fef807166f3b5b94efcfc0fab31f41, including all prerequisite commits. The PR remains non-draft. Read AGENTS.md and the supplied reviews, issue comments, inline comments and threads, excluding Copilot. Routed skill: review-comet-pr. No sibling skill applied.

Exact-head CI: 23 checks passed and 14 were skipped. Comet CI, Required Checks and CodeQL passed, including native tests, all four Linux Spark 4.1 Comet suites and TPC-H/TPC-DS verification. CI tested merge commit d3211cdcc9e83a26941ae5c8e3dcad077bb12628 against newer main. Fourteen of the sixteen changed files match the pinned head, including the routing, bridge, dispatcher and tests. Spark SQL, Iceberg, macOS and benchmark jobs were skipped.

Validation: reran all 16 routing tests using a byte-identical production module in an isolated crate with matching locked direct dependency versions. Both additional credential-error probes confirmed the existing defect without network requests. Compiled the dispatcher tests with Java 17 and --release 17; all nine passed. No local full Comet build, integrated JNI/MinIO run, Spark SQL suite or end-to-end benchmark was performed. Project files remain unchanged.