comphead opened a new pull request, #5825:
URL: https://github.com/apache/datafusion-comet/pull/5825
## Which issue does this PR close?
Closes #5816
## Rationale for this change
`fs.comet.libhdfs.schemes` is matched against the scheme *after* the
`s3a`/alias rewrite, so listing `s3` also routes `s3a://` reads through libhdfs.
`prepare_object_store_with_configs` reads:
```rust
let url = normalize_object_store_url(url.as_str(), object_store_configs)?;
let is_hdfs_scheme = is_hdfs_scheme(&url, object_store_configs);
```
`normalize_object_store_url` guards its own early return with
`is_hdfs_scheme`, so an `s3a://` URL whose scheme is not in the list falls
through to `rewrite_alias_to_s3` and comes back as `s3://`. The next line then
sees `s3`, matches the list, and selects the libhdfs backend with a name node
of `s3://bucket`. Schemes opted in via `fs.comet.s3Compliant.schemes` normalize
onto `s3` too, so they hit the same trap.
Before #5314 the flag was computed on the original URL and the rewrite was
guarded by it, so the rewrite could not flip the decision. #5314 replaced that
inline rewrite with a call to `normalize_object_store_url` and moved the check
after it.
The JVM gate matches `fs.comet.libhdfs.schemes` against the scheme the user
wrote (`CometScanRule.classifyRootPaths`) and so admits these scans as
object_store-native, which means the recompute also desyncs the planner from
the executor.
Only an asymmetric list triggers it. Listing both `s3` and `s3a`, or leaving
the config unset (default `hdfs`), behaves identically before and after.
## What changes are included in this PR?
- `normalize_object_store_url` returns `NormalizedObjectStoreUrl { url,
is_hdfs }`, carrying the libhdfs decision it already takes on the URL as
written. Callers read the flag rather than re-deriving it from the rewritten
URL, which makes the recompute structurally impossible instead of merely
discouraged.
- `prepare_object_store_with_configs` destructures that flag.
- The object-key call site in `planner.rs` takes `.url`.
No behavior change for any other configuration.
## How are these changes tested?
Two new Rust unit tests:
- `test_libhdfs_routing_uses_the_scheme_as_written` in
`s3_blob_fs_support.rs` covers `s3a` and an opted-in `blob` alias under
`fs.comet.libhdfs.schemes=s3` (must not route to libhdfs), the legitimate
`fs.comet.libhdfs.schemes=s3a` opt-in (must still route there, with the URL
left unrewritten), and the unset default. It also pins the trap explicitly:
`is_hdfs_scheme` on the normalized URL still returns `true`, which is why the
flag is carried.
- `test_prepare_object_store_keeps_s3a_off_libhdfs_when_only_s3_is_listed`
in `parquet_support.rs` covers the real dispatch, asserting an S3 store is
built for both spellings and that an explicitly listed `s3a` still reaches the
libhdfs backend.
Verified as regression guards: with the defect reintroduced, both fail on
the exact assertion, while the five pre-existing tests in the same two modules
pass, confirming the gap was previously uncovered.
The full `datafusion-comet` lib suite passes under both feature
configurations, 337 tests with `--no-default-features` and 334 with default
features (`hdfs-opendal`). `cargo fmt --check` and `cargo clippy --all-targets
-- -D warnings` are clean under both.
Per #5816, the `isolates_backends_even_when_s3_alias_and_configs_match` test
in #5503 covers this same pair and was failing on merge with `main`. This
should unblock it.
--
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]
