[PR] feat: [branch-1.0] per-location credentials for the S3 credential SPI (#6031) [datafusion-comet]

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

andygrove opened a new pull request, #6226:
URL: https://github.com/apache/datafusion-comet/pull/6226

   Backport of #6031 to `branch-1.0`.
   
   Cherry-picked from `e5d0b7575f1b7bfe4b19da7246f2d9733357e18f`. Fifteen of the sixteen files applied cleanly, and their changes match upstream's apart from line offsets. `native/core/src/parquet/objectstore/s3.rs` had one conflict, over an error message. It is described under "What changes are included" below.
   
   ## Which issue does this PR close?
   
   Closes #6207 on `branch-1.0`. #6031 already closed it on `main`.
   
   ## Rationale for this change
   
   The limitation that #6031 removes ships in 1.0.0. On the Parquet path, `object_store::CredentialProvider::get_credential()` receives no request path, so a `CometS3CredentialProvider` gets one credential per bucket. Comet requests it with the path of the first file it reads. A vendor whose policies differ by location within a bucket, for example `warehouse/sales` and `warehouse/finance`, gets 403s on every location but the first.
   
   On `branch-1.0`, the SPI classes, the JNI dispatcher and the credential bridge match `main` just before #6031, apart from an `#[allow(dead_code)]` on `AccessMode::Write`. `create_store` differs only in the error message below.
   
   The change is opt-in. Providers that implement only `CometS3CredentialProvider` get the same store as before. The only addition is one dispatcher call when the store cache misses. It returns `null` without reaching the provider.
   
   ## What changes are included in this PR?
   
   The change is the original one, so see #6031 for the details. It adds the `@Public` `CometS3LocationScopedCredentialProvider`, `CometS3CredentialDispatcher.getPolicyLocations` and the native `LocationScopedObjectStore`. It also updates the user guide, the design notes and the versioning policy's list of SPI classes.
   
   One adaptation:
   
   - In `s3.rs`, `S3StoreTemplate::new` keeps `branch-1.0`'s region-lookup error, `Failed to resolve region: {e}`. #6031 moved that code into the new template. On `main`, #5314 had already extended the message with advice for S3-compatible services. #5314 isn't on `branch-1.0`, so the message stays as it is here. Apart from the message, the file matches `main`'s copy after #6031.
   
   #6212 affects this code on `branch-1.0` as well. The store fetches the locations again only after a 403. A provider with no policy for the path it is asked about usually throws instead, and a throw does not trigger the refresh. So reads under a location that was added or dropped mid-job keep failing until the executor restarts. The fix is #6223, which is open on `main` and also labelled `backport-1.0`. It would go on top of this PR. It also corrects the user guide's "When Comet asks" paragraph, which this PR carries unchanged.
   
   ## How are these changes tested?
   
   The original PR's tests, run locally on `branch-1.0`:
   
   - Rust: all 16 `location_scoped` tests pass, as does the new `builds_from_a_template_inside_the_runtime` test in `s3.rs`. The rest of `parquet::objectstore` passes too, 65 tests in all. `cargo fmt --all -- --check` and `cargo clippy --all-targets --workspace -- -D warnings` pass on rustc 1.97.
   - JVM, on the branch's default profile (Spark 4.1.3, Scala 2.13, JDK 17): the new `CometS3LocationScopedCredentialProviderTest` (9 tests) passes. So do the existing `CometS3CredentialDispatcherTest` (16 tests) and `CometPublicApiSuite`, which now pins the new type.
   - The Lint Java job's command for Spark 3.4, Scala 2.12 and JDK 11 passes: `package -DskipTests scalafix:scalafix -Dscalafix.mode=CHECK -Psemanticdb`. It compiles the test sources on Scala 2.12, the build that #6031's `Set.of` fix was for. Spotless and scalastyle pass, and so does Prettier on the three changed docs.
   
   `CometS3CredentialBridgeSuite`, the MinIO end-to-end suite, was not run because it needs Docker. CI doesn't run it on either branch.
   
   ## Are there any user-facing changes?
   
   For S3 credential provider vendors, yes. `branch-1.0` gains the `@Public` `CometS3LocationScopedCredentialProvider` interface, and the versioning policy's list of SPI classes names it. Providers that don't implement it see no change. There are no config changes.
   


-- 
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

Re: [PR] feat: [branch-1.0] per-location credentials for the S3 credential SPI (#6031) [datafusion-comet]

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

andygrove merged PR #6226:
URL: https://github.com/apache/datafusion-comet/pull/6226


-- 
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