andygrove commented on PR #5503:
URL:
https://github.com/apache/datafusion-comet/pull/5503#issuecomment-5607681403
I chased the test failure I pointed you at rather than leaving it with you,
and it is not your branch. Sorry for the noise.
`isolates_backends_even_when_s3_alias_and_configs_match` passes on
`deaf061bb` as it stands. I checked out the head and ran the five isolation
tests locally:
```
test
parquet::parquet_support::tests::native_s3_aliases_share_cache_and_registration_identity
... ok
test
parquet::parquet_support::tests::keeps_native_file_url_separate_from_explicit_hadoop_file_routing
... ok
test
parquet::parquet_support::tests::preserves_custom_hadoop_scheme_when_routing_changes
... ok
test
parquet::parquet_support::tests::isolates_backends_even_when_s3_alias_and_configs_match
... ok
test
parquet::parquet_support::tests::isolates_native_stores_with_different_configurations
... ok
```
CI tests the merge with `main`, and the branch point here is `ec0f7975`,
before #5314 landed. That is what @comphead was pointing at with the #5314
reference. #5314 replaced
```rust
let is_hdfs_scheme = is_hdfs_scheme(&url, object_store_configs);
let mut scheme = url.scheme();
if !is_hdfs_scheme && scheme == "s3a" { scheme = "s3";
url.set_scheme("s3")?; }
```
with
```rust
let url = normalize_object_store_url(url.as_str(), object_store_configs)?;
let is_hdfs_scheme = is_hdfs_scheme(&url, object_store_configs);
```
so the libhdfs classification now runs against the *rewritten* scheme. Your
test's first case is `s3a` input with `fs.comet.libhdfs.schemes=s3`, which
normalizes to `s3://` and then matches the list, so it comes back as the Hadoop
backend just like the second case. Both URLs end up
`s3+comet-ba6b8898acdeb3c1-hdfs`, `assert_ne!` fires, and the values in the
failure I quoted are exactly that.
I confirmed the mechanism on `424c31aa7` directly rather than inferring it
from the diff:
```
PROBE typed scheme = s3a
PROBE typed is_hdfs = false
PROBE normalized scheme = s3
PROBE final is_hdfs = true
```
So this is a live behaviour bug on `main`, not a test problem: with
`fs.comet.libhdfs.schemes=s3` and no `s3a` entry, an `s3a://` scan is now
served by `create_hdfs_object_store` instead of the native S3 store. I filed it
as #5816 with the reproducer. The converse is fine, since listing `s3a` makes
`normalize_object_store_url` return early and the spelling survives.
Which leaves the question of ordering. Your test is the right regression
guard for #5816, so I would rather it landed here and #5816 was fixed on top of
it than have the test weakened to accommodate the current `main`. Could you
rebase onto `main` and confirm whether you would prefer to fold the one-line
ordering fix into this PR, given that it is in the function you are already
changing, or keep #5816 separate and mark the test `#[ignore]` with a link
until it lands? I have a mild preference for folding it in: taking the
classification once from the typed scheme and threading it into
`normalize_object_store_url` is a couple of lines, and it makes the backend
component of your new cache key mean what it says.
The documentation follow-up in `deaf061bb` reads well, and the cache-key and
registration-URL notes cover the question I raised about why `comet` ends up in
the scheme.
--
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]
