andygrove opened a new issue, #23926:
URL: https://github.com/apache/datafusion/issues/23926
## Describe the bug
`get_url_key` in `datafusion-execution/src/object_store.rs` builds the
registry key from `Position::BeforeHost..Position::AfterPort`, which excludes
URL userinfo:
```rust
/// Get the key of a url for object store registration.
/// The credential info will be removed
fn get_url_key(url: &Url) -> String {
format!(
"{}://{}",
url.scheme(),
&url[url::Position::BeforeHost..url::Position::AfterPort],
)
}
```
The doc comment describes this as removing "credential info," which is
accurate for HTTP-style basic auth (`user:pass@host`). It is not accurate for
ABFS. Azure Data Lake Storage Gen2 URLs encode the storage container as the URL
userinfo:
```
abfss://<container>@<account>.dfs.core.windows.net/<path>
```
`object_store::azure::MicrosoftAzureBuilder::parse_url` reads the container
from the userinfo and binds it into the resulting store instance. The resource
`Path` is then container-relative. So
`abfss://[email protected]/...` and
`abfss://[email protected]/...` are two different stores, but
`get_url_key` reduces both to `abfss://acct.dfs.core.windows.net`.
The consequence within a single `RuntimeEnv`: registering the `c2` store
overwrites the `c1` store, and any subsequent lookup for `c1` returns the `c2`
store, which reads from the wrong container.
## To Reproduce
Register two Azure stores for different containers on the same account
against one `RuntimeEnv`:
```rust
let rt = RuntimeEnv::default();
let url_c1 = Url::parse("abfss://[email protected]/").unwrap();
let url_c2 = Url::parse("abfss://[email protected]/").unwrap();
rt.register_object_store(&url_c1, store_c1);
rt.register_object_store(&url_c2, store_c2);
// rt.object_store(&url_c1) now returns store_c2
```
## Expected behavior
Two Azure containers on the same account should be addressable as two
distinct stores from one `RuntimeEnv`.
## Additional context
This was noticed while reviewing a Comet fix for the same class of bug in a
Comet-owned process-wide store cache (apache/datafusion-comet#4993 /
apache/datafusion-comet#5053). Comet dodges the DataFusion-layer issue by
constructing a fresh `RuntimeEnv` per Parquet file read, so today the DF layer
never sees two ABFS containers registered on one env. That workaround is
fragile — any refactor that shares a `RuntimeEnv` across container reads
reintroduces the bug at the DF layer.
Two possible directions, roughly in preference order:
1. **Scheme-aware key derivation.** For schemes where userinfo is a
namespace rather than a credential (`abfs`, `abfss`), include the userinfo in
the key. For other schemes, keep the current behavior so existing basic-auth
callers are unaffected.
2. **Include userinfo unconditionally.** Simpler and arguably more correct —
treating userinfo as identity rather than credential in a *cache key* is safe
(the key is not logged, and two callers with different credentials for the same
host arguably *should* get different stores anyway). This is a semantic change
and would need a migration note.
Happy to send a PR once maintainers pick a direction.
--
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]
