peterxcli commented on PR #5053: URL: https://github.com/apache/datafusion-comet/pull/5053#issuecomment-5445032389
Thanks for the careful pass — all five points were worth acting on; addressed in ff694ec5a. **Registry coverage:** Good catch. Auditing all `RuntimeEnv` construction sites turned up one production path without the custom registry: `initRecordBatchReader` (`native_iceberg_compat`) builds a private `SessionContext` per reader. It wasn't observably wrong — a per-reader registry only ever sees one store — but isolation there depended on that invariant staying true, so I've installed `CometObjectStoreRegistry` on that path as well. The only other construction sites are test-only (`PhysicalPlanner::default()` and friends). **Upstream:** Yes — apache/datafusion#23935 fixes `get_url_key` upstream. I've added a doc comment on `CometObjectStoreRegistry` marking it a temporary workaround with a link, so it can be removed at the version bump. **S3 test:** Agreed the name oversold it. Renamed to `test_non_azure_schemes_ignore_userinfo` with a comment that the URL shape is synthetic (real S3 URLs carry the bucket in the host position). **Lock consistency:** Switched to `std::sync::RwLock` to match the rest of the file, recovering from poisoning via `PoisonError::into_inner`. **Registry bypass:** Nothing in DataFusion 54.1 enumerates the registry, and `RuntimeEnv::object_store` is the only lookup path — but your question surfaced that the trait's provided `deregister_store` defaults to a not-implemented error, and `DefaultObjectStoreRegistry`'s own implementation wouldn't see Azure stores either. Nothing calls it today, but I've implemented it on `CometObjectStoreRegistry` so the registry is complete. -- 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]
