andygrove commented on PR #5053:
URL:
https://github.com/apache/datafusion-comet/pull/5053#issuecomment-5209585323
> We didn't use the datafusion's `get_url_key`.
I think we may be talking past each other here. Comet doesn't call
`get_url_key` directly, but `runtime_env.register_object_store()` and
`runtime_env.object_store()` both go through `DefaultObjectStoreRegistry`,
which does call it. In `datafusion-execution-54.1.0/src/object_store.rs:268` it
slices `Position::BeforeHost..Position::AfterPort`, and the doc comment on it
reads "The credential info will be removed."
I put together a quick probe on your branch that registers two containers
against one shared `RuntimeEnv`, the way `planner.rs:1587` does:
```rust
#[test]
fn test_shared_runtime_env_collides_across_containers() {
let configs = HashMap::from([("fs.azure.account.key".into(),
"c2VjcmV0".into())]);
// One plan => one shared RuntimeEnv, as in planner.rs
let runtime_env = Arc::new(RuntimeEnv::default());
let register = |container: &str| {
super::prepare_object_store_with_configs(
Arc::clone(&runtime_env),
format!("abfss://{container}@shared-acct.dfs.core.windows.net/path/file.parquet"),
&configs,
)
.unwrap()
.0
};
let url_a = register("container-a");
let url_b = register("container-b");
let store_a = runtime_env.object_store(&url_a).unwrap();
let store_b = runtime_env.object_store(&url_b).unwrap();
println!("url_a={url_a} url_b={url_b}");
println!("same store: {}", Arc::ptr_eq(&store_a, &store_b));
println!("store_a={store_a:?}");
}
```
Output:
```
url_a=abfss://[email protected]/
url_b=abfss://[email protected]/
same store: true
store_a=MicrosoftAzure { ... container: "container-b" ... }
```
Both URLs resolve to the same store, and that store is bound to
`container-b`. So a scan of `container-a` would silently read `container-b`'s
data. `FileScanConfig` resolves the store at execute time
(`datafusion-datasource/src/file_scan_config/mod.rs:640`), after every scan in
the plan has already registered, so the last registration wins. A join or union
across two containers in one storage account would hit this. The
`native_iceberg_compat` path in `parquet/mod.rs:163` is safe because it builds
a fresh `SessionContext` per file, but `native_datafusion` shares the
task-scoped one.
Would you be up for closing this in the same PR? Comet already builds its
`RuntimeEnv` through `RuntimeEnvBuilder` at `jni_api.rs:559`, and DF 54.1
exposes `RuntimeEnvBuilder::with_object_store_registry`, so a small registry
impl that keys on the full authority would fix it now without waiting on
apache/datafusion#23935 and the version bump. If you'd rather keep this PR
scoped to the cache key, that's reasonable, but could we get an issue filed for
the registry side so it doesn't get lost?
Two smaller things while I'm here.
The two new tests build a fresh `RuntimeEnv` per call, which is what makes
the assertions hold. That's the right shape for testing the Comet cache in
isolation. It might be worth adding one more that shares a single `RuntimeEnv`
across both registrations, since that's what `planner.rs` actually does and
it's the case that still resolves to one store today.
On the module doc at `parquet_support.rs:501`, since the container is now
part of the Comet cache key but not part of the DataFusion registry key, could
we add a sentence noting that per-container isolation depends on the registry
side too? As written a reader could come away thinking the key change alone is
sufficient.
One thing I noticed that works in your favour and isn't mentioned in the
description: `translate_hadoop_configs` resolves
`fs.azure.sas.<container>.<account>`, so before this change two containers
sharing one config map would resolve different SAS tokens into the same cached
store. This fixes that as well.
--
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]
