andygrove opened a new issue, #6536:
URL: https://github.com/apache/datafusion-comet/issues/6536
#6478 (for #6462) added per-location S3 credentials for native Iceberg reads
and writes. Reviewing it turned up the follow-on work below. None of it blocked
the PR.
- [ ] Native Iceberg scans skip the configured provider when the table's
metadata location has no host
- [ ] Narrow the multi-bucket wording in the user guide and the
`CometS3LocationScopedCredentialProvider` Javadoc
- [ ] Update the design doc's description of how location fetches block
- [ ] Add native write and cross-bucket read cases to
`CometS3CredentialBridgeSuite`
### 1. Hostless metadata locations skip the provider
When an Iceberg catalog sets `s3.comet.credential.provider.class`, the
native Iceberg scan builds the credential bridge from the bucket in the table's
metadata location (`build_s3_access` in
`native/core/src/execution/operators/iceberg_common.rs`). An opted-in
S3-compliant alias scheme (`fs.comet.s3Compliant.schemes`) can record hostless
locations such as `blob:///bucket/warehouse/db/t/metadata/v3.metadata.json`,
with the bucket as the first path segment. That URL has no host, so
`build_s3_access` returns `S3Access::Loader(None)` before it looks up the
provider class. The scan then signs every request with opendal's default
credential chain. The configured provider is never called, and a
`CometS3LocationScopedCredentialProvider`'s policy locations are not used. The
IRSA web-identity take-over is skipped as well, and nothing is logged.
Such a table still scans natively. `CometScanRule` checks the metadata
location's scheme but not its host, and `hasOpenableAuthority` accepts hostless
alias data and delete files, because `BlobHostPromotingS3Storage` promotes the
bucket when a file is opened. So the requests reach S3 with whatever
credentials the executor's environment provides. Depending on those, reads fail
with 403 or succeed with broader credentials than the provider would have
vended.
The Parquet path promotes the bucket from the first path segment before it
builds the store (`prepare_object_store_with_configs`), so this gap is specific
to the Iceberg path. Native Iceberg writes are not affected, because the write
gate declines alias schemes. #6478 documents the limitation in the user guide
("Enabling a bridge") and in the Javadoc.
This was found by reading the code. The test FileSystem in `CometS3TestBase`
only handles host-bearing `blob://bucket/...` paths, so the MinIO suites cannot
produce a hostless table today.
Expected: the configured provider serves the table, as it does for
`blob://<bucket>/...` locations and on the Parquet path. If Comet cannot do
that, it should fall back to Spark rather than read with the default chain. One
possible fix is to promote the reference path with `promote_hostless_alias_url`
before `build_s3_access` reads its host, when the scheme is an opted-in alias.
The `FileIO` cache key can keep the raw path.
### 2. Multi-bucket wording
The user guide (`s3-credential-providers.md`, "Credentials per location")
and the `@Public` `CometS3LocationScopedCredentialProvider` Javadoc say a table
whose files span several locations or buckets gets each file's credential
right. But `CometScanRule` still falls back to Spark when a scan's data and
delete files span more than one S3 bucket, because one `FileIO` carries one S3
configuration. So the cross-bucket case Comet serves natively is a table whose
data and delete files sit in a single bucket other than its metadata's.
That case does work. A MinIO run with the metadata in one bucket and
`write.data.path` in another requested each data file's credential for its own
bucket and location, and called `getPolicyLocations` once per bucket. Both
places should describe that case instead, since vendors read the Javadoc as the
contract.
### 3. Design doc on blocking calls
`s3-credential-provider-design.md` ("Location-scoped credentials on the
Iceberg path") says another bucket's locations are fetched inside an async
storage call, "so the JVM call runs in `tokio::task::block_in_place`". Since
fe0384671 every fetch, including refreshes, goes through `run_blocking` in
`iceberg_location_scoped.rs`. It uses `block_in_place` only on a multi-thread
runtime and otherwise runs the call directly. `block_in_place` panics on a
current-thread runtime, which is where `AbortOnDrop` deletes a failed write's
files, and a 403 on one of those deletes refreshes the locations. The doc
should describe `run_blocking` and that reason, so the invariant is not lost.
### 4. End-to-end tests
`CometS3CredentialBridgeSuite` (a manual MinIO suite) covers a native
Iceberg read through a location-scoped provider. Writes and other buckets are
covered only by Rust tests with fake storages. So nothing drives the real
OpenDAL storage, the derived bridges (`CometS3CredentialBridge::for_location`),
or a second bucket's `getPolicyLocations` end to end. Two cases would close
that:
- A native write that asserts `CometIcebergWriteExec` ran and that each
location got a WRITE-mode credential request.
- A read of a table whose metadata is in `scopedBucket` and whose
`write.data.path` is in `testBucketName`. It should assert each data file's
bucket and location, and one `getPolicyLocations` call per bucket.
`MinioLocationScopedCredentialProvider` would need to record the mode and
bucket along with the path. Both cases pass when run alone against #6478.
One thing to watch: a registration keeps its locations while any of its
cached `FileIO`s is alive. So a test that installs new locations after an
earlier test already read through the same catalog still sees the old list, and
its reads route to `/`. Install the locations before the catalog's first read,
or give the new tests their own catalog.
--
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]
