andygrove commented on PR #5053:
URL:
https://github.com/apache/datafusion-comet/pull/5053#issuecomment-5441458995
> **Note on this review:** this was generated by an LLM (Claude Code) at my
request while I worked through a review backlog. I have not verified the
individual findings myself. Please treat everything below as suggestions to
evaluate rather than as authoritative review feedback, and push back on
anything that is wrong or already handled.
This is a real correctness bug and the diagnosis is clear: two containers in
the same storage account are two different stores, and keying on
`scheme://host:port` collapses them. Returning data from the wrong container is
about as bad as it gets, so this is worth fixing.
A few questions before merge.
**Is the custom registry installed everywhere a `RuntimeEnv` is built?**
`CometObjectStoreRegistry` is wired in at
`prepare_datafusion_session_context`. Is that the only place Comet constructs a
`RuntimeEnv`? The native Parquet reader entry point in `parquet/mod.rs` also
does `session_ctx.runtime_env()`, and if any path builds its own runtime with
the default registry, ABFS isolation quietly stops working there while the
process-wide cache in `prepare_object_store_with_configs` still does the right
thing. A grep for `RuntimeEnvBuilder::new` in the description, or an assertion
somewhere, would settle it. The two-layer design here means both layers have to
agree, and only one of them is hard to forget.
**Should this be fixed upstream instead?**
`DefaultObjectStoreRegistry::get_store` builds its key from
`Position::BeforeHost`, dropping userinfo. That is arguably a DataFusion bug
rather than something specific to Comet, since any object store that encodes
identity in userinfo hits it. Has an issue been filed upstream? If it has,
linking it here and marking `CometObjectStoreRegistry` as a temporary
workaround would make it easier to remove later. If DataFusion would not accept
the change, that is worth writing down too.
**The S3 test asserts a URL shape that does not occur**
`test_s3_store_cache_keys_by_host` constructs
`s3://{bucket}@shared-host/path/file.parquet`, putting the bucket in userinfo
and a separate host after it. Real S3 URLs put the bucket in the host position.
So the test proves that userinfo is ignored for non-Azure schemes, which is
what you intended, but the name and the variable called `bucket` suggest it is
testing something about buckets that it is not. Could it be renamed to
something like `test_non_azure_schemes_ignore_userinfo`, with a comment saying
the URL shape is synthetic?
**Two smaller notes**
`azure_stores` uses `parking_lot::RwLock` while the neighbouring
`ObjectStoreCache` uses `std::sync::RwLock`. Not a problem, but consistency
within one file is worth having unless there is a reason for the difference.
`register_store` for Azure inserts into `azure_stores` and never touches
`self.default`. So `DefaultObjectStoreRegistry` never learns about ABFS stores
at all. Is there anything in DataFusion that enumerates the registry, or looks
up a store through a path other than `get_store`, that would now miss them?
--
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]
