Skip to content

fix: include ABFS container in object store cache key - #5053

Open
peterxcli wants to merge 2 commits into
apache:mainfrom
peterxcli:fix/include-abfs-cotainer-in-object-store-cache-key
Open

fix: include ABFS container in object store cache key#5053
peterxcli wants to merge 2 commits into
apache:mainfrom
peterxcli:fix/include-abfs-cotainer-in-object-store-cache-key

Conversation

@peterxcli

Copy link
Copy Markdown
Member

Which issue does this PR close?

Closes #4993.

Rationale for this change

ABFS URLs encode the container in URL userinfo, for example
abfss://container@account.dfs.core.windows.net/path. The process-wide object store cache
previously keyed stores by scheme, host, and port only, so different containers in the same
storage account shared one cache entry. Since each Azure object store is bound to the container
from its URL, a later read could reuse the first container's store and return data from the wrong
container.

What changes are included in this PR?

  • Preserve URL userinfo when building object store keys for abfs and abfss, while retaining
    the existing host-only behavior for other schemes.
  • Update the cache documentation to describe container-scoped keys.
  • Add an offline regression test that prepares two containers in the same account with identical
    configuration and verifies that they resolve to distinct object store instances.

How are these changes tested?

  • cargo test --manifest-path native/Cargo.toml -p datafusion-comet --lib
    (129 passed; 4 ignored)
  • cargo clippy --manifest-path native/Cargo.toml -p datafusion-comet --lib --tests -- -D warnings
  • cargo fmt --manifest-path native/Cargo.toml --all -- --check

The regression constructs the Azure stores locally and does not issue network requests.

@peterxcli
peterxcli marked this pull request as ready for review July 27, 2026 16:11
@andygrove

Copy link
Copy Markdown
Member

Nice targeted fix. I confirmed the analysis by reading native/core/src/parquet/objectstore/azure.rsMicrosoftAzureBuilder::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 Object store cache key drops the ABFS container, so two containers in one storage account share a store instance #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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Object store cache key drops the ABFS container, so two containers in one storage account share a store instance

2 participants