andygrove commented on PR #5053:
URL:
https://github.com/apache/datafusion-comet/pull/5053#issuecomment-5095074456
Nice targeted fix. I confirmed the analysis by reading
`native/core/src/parquet/objectstore/azure.rs` —
`MicrosoftAzureBuilder::parse_url` binds the container into the store instance
and the resource `Path` is container-relative, so a store genuinely cannot be
reused across containers. `Position::BeforeUsername..AfterPort` correctly
includes the userinfo when present and collapses to `BeforeHost` when it isn't,
so non-Azure schemes are untouched.
A few small things worth considering:
1. **DataFusion's own registry has the same blind spot.**
`RuntimeEnv::register_object_store` uses `get_url_key`, which also strips
userinfo (`datafusion-execution/src/object_store.rs`). So within a single
`RuntimeEnv`, only one abfss store per account can be registered — a second
registration would overwrite the first at the DF layer. Today this is fine
because Comet constructs a fresh `RuntimeEnv` per Parquet file read, as the
module doc calls out. But that invariant is now load-bearing for correctness on
Azure. A sentence tying the two together in the module doc block would help a
future reader who tries to share a `RuntimeEnv` across container reads.
2. **A one-line comment on the `is_azure_scheme(scheme)` branch** explaining
that ABFS URLs encode the container in the userinfo would save future readers a
trip to #4993.
3. **Two small extensions to the new test**, if you want them:
- Assert that a second call with the same container returns the same
`Arc`, so a future change that accidentally invalidates the same-URL cache path
is also caught.
- Add an `s3://bucket@…`-style case to pin down that the non-Azure branch
still keys by host only.
None of these are blockers.
--
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]
