Re: [PR] feat: Credential provider support [datafusion-comet]

["mbutrovich (via GitHub)" <gi...@apache.org>]

mbutrovich commented on PR #4335:
URL: https://github.com/apache/datafusion-comet/pull/4335#issuecomment-4478448502

   Thanks for the careful citations @karuppayya, those helped a lot.
   
   I'm trying to figure out whether the broadcast and refresh machinery in this PR is something Comet should own, or whether a vendor implementation under #4309's SPI could give you the same end result with less Comet-side surface area. I'd love your read on that.
   
   One scope thing I want to flag up front: we also need this for raw Parquet on S3, and any other native S3 reader Comet grows. Iceberg-vended creds are one important case, but vendors with custom signers or per-path STS for plain Parquet hit the same wall (Hadoop S3A signers don't surface the secret, `AWSCredentialsProvider.getCredentials()` is parameterless). The SPI shape in this PR is fairly Iceberg-shaped (named `CometCredentialProvider` under `org.apache.comet.iceberg`, `initialize(catalogProperties)` keyed off the catalog property bag, `ResolveContext(tableLocation)`), and it's not obvious how the Parquet path fits since there's no catalog or table location. #4309 covers both paths through one SPI under `org.apache.comet.cloud.s3` with a per-call `(bucket, path, mode)` context that works for either, and I'd want whatever we land to keep that breadth.
   
   On the REST-vended credentials point, the reason #4309 today can't reach `credentials.uri` and friends is that `CometScanRule.filterStorageProperties` strips them down to `s3.`/`gcs.`/`adls.`/`client.` prefixes before they cross JNI. If we relax that filter so the full FileIO property map crosses as `catalog_properties`, and we only narrow to storage-prefix keys at the iceberg-rust `FileIOBuilder.with_prop` call site, then a vendor under #4309 sees everything `LoadTableResponse` returned. That seems to obsolete the parallel `allFileIOProperties` field this PR adds, with a much smaller change.
   
   On multi-catalog with shared FQCN, you're right that #4309 has the gap today. I think it's fixable inside the existing SPI shape by extending `CometS3CredentialDispatcher` to cache by `(FQCN, catalog id)` and adding `initialize(Map<String,String> catalogProperties)` to `CometS3CredentialProvider`, called once per catalog on first instantiation. That mirrors `S3FileIO.initialize`, which is the shape vendors already write against for Iceberg.
   
   What I'm less sure should live in Comet is refresh and distribution. The Rust-side TTL cache, the per-plan single-catalog invariant that throws on multi-catalog plans, and the driver-side broadcast cache feel like things Iceberg's own `CachedSupplier` / `VendedCredentialsProvider` and Spark's broadcast already cover, just inside the vendor's implementation rather than in Comet. Concretely, an Iceberg REST vended-credential `CometS3CredentialProvider` would look roughly like this:
   
   ```java
   public class IcebergRESTVendedS3Provider implements CometS3CredentialProvider {
     private volatile VendedCredentialsProvider provider;
   
     @Override
     public void initialize(Map<String, String> catalogProperties) {
       this.provider = VendedCredentialsProvider.create(catalogProperties);
     }
   
     @Override
     public CometS3Credentials getCredentialsForPath(
         String bucket, String path, CometS3AccessMode mode) {
       AwsSessionCredentials c = (AwsSessionCredentials) provider.resolveCredentials();
       long expiresAt = c.expirationTime().map(Instant::toEpochMilli).orElse(-1L);
       return new CometS3Credentials(
           c.accessKeyId(), c.secretAccessKey(), c.sessionToken(), expiresAt);
     }
   }
   ```
   
   Iceberg's `CachedSupplier` inside `VendedCredentialsProvider` handles refresh and prefetch, so neither the vendor nor Comet has to reinvent it. We could ship something like this as a reference implementation in Comet alongside the SPI so REST users aren't writing it from scratch, while keeping caching/refresh/distribution policy out of Comet itself.
   
   Does this shape cover what you need, or is there a scenario I'm missing where the vendor model in #4309 (extended with the unfiltered properties + `initialize(Map)`) can't reach? Want to make sure I'm not arguing against a real requirement.
   


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org