peterxcli commented on PR #5053: URL: https://github.com/apache/datafusion-comet/pull/5053#issuecomment-5227479899
@andygrove thanks for further explanation! you're really right on this. I've addressed all of your review/suggestion. > `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: > .... > 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. Thanks for catching this! sorry my previous response just refuse to address this because that's my blindspot. > 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. added as `test_shared_runtime_env_uses_distinct_azure_container_stores` in `native/core/src/parquet/parquet_support.rs` > 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. added. > 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. Nice! -- 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]
