Support prefix-scoped object stores in the registry#23287
Draft
kszucs wants to merge 6 commits into
Draft
Conversation
github-actions
Bot
added
core
kszucs
force-pushed
the
prefixed-object-store-registry
branch
from
7936d42 to
a46c62c
Compare
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
kszucs
force-pushed
the
prefixed-object-store-registry
branch
from
a46c62c to
041dd61
Compare
Register object stores under a path prefix (e.g. `hf://bucket/user/repo`) so multiple stores can coexist under one scheme+authority, and hand each store paths with its registered prefix stripped. This fixes double-prefixing for stores rooted at a prefix (OpenDAL apache#7786). - Wrap object_store's path-aware registry (register/resolve/deregister) and add `ObjectStoreRegistry::resolve` returning the store plus a store-relative path. - Add `ListingTableUrl::resolve` and rewire the read and schema/partition inference paths to list against the rebased, store-relative url. - Allow `ObjectStoreUrl` to carry a path prefix; key the datafusion-cli stdin store by authority.
Remove the bespoke StdinUtils::store_url helper. The stdin store is a single per-session store identified by the stdin:// authority, which is exactly what ListingTableUrl::object_store() returns, so register under that identity and let the registry resolve object paths to it.
Replace the prefix-blind ListingTableUrl::object_store() with a single registry-aware ListingTableUrl::resolve() that returns the serving store, the store identity (scheme + authority + registered prefix), and the url rebased into the store's coordinate space (ResolvedTableUrl). The registry resolve() returns (store, store-relative path); the identity is derived from the url and that path. This makes both read and write paths correct for stores registered under a path prefix.
- datafusion-cli: register object stores at their scheme/authority identity instead of the full object path, so a store rooted at the authority is not scoped to a deep prefix and handed an empty relative path (broke every CLI CREATE EXTERNAL TABLE / COPY with a path component). - catalog-listing: reject a ListingTable whose paths resolve to different object stores (a scan carries a single store), replace the positional ResolvedTablePaths tuple with a named struct, and resolve the first path once. - execution: preserve the strict "No suitable object store found" error by gating resolve on explicit registration, so the registry never lazily creates a store from ambient environment credentials. Restore the file:// default registration for DefaultObjectStoreRegistry::default(). - Add regression tests for each fix.
kszucs
force-pushed
the
prefixed-object-store-registry
branch
from
53730fb to
25e1cd5
Compare
…istry - Restore ListingTableUrl::object_store() as #[deprecated] in favor of resolve(), which correctly handles stores registered under a path prefix. - Document the registered-set linear scan in DefaultObjectStoreRegistry as an intentional tradeoff at expected registration cardinalities. - Add a positive test for a multi-path ListingTable whose paths all resolve to the same prefixed store. - Add a proto round-trip test asserting a path-bearing ObjectStoreUrl survives ParquetExec serde.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Addresses the double-prefixing problem described in apache/opendal#7786, where an object store registered under a path-prefixed URI (e.g. an OpenDAL store rooted at a specific repo) is handed paths that still contain the registered prefix.
Rationale for this change
DataFusion's object store registry keys stores by scheme + authority only. A store registered for
hf://bucket/user/repotherefore collapses to the keyhf://bucket, so (1) multiple repos under one authority cannot coexist, and (2) the full path (user/repo/data/f.parquet) is handed to the store, which — being rooted at the repo — prepends its root again, producinguser/repo/user/repo/...and a not-found error.This PR lets stores be registered under a path prefix and hands each store paths with its registered prefix stripped.
What changes are included in this PR?
object_store's path-aware registry (longest-prefixregister/resolve/deregister) and addObjectStoreRegistry::resolve, returning the store plus a store-relative path.ListingTableUrl::resolve, returning the store, the baseObjectStoreUrlscan key (including any registered prefix), and a rebasedListingTableUrlwhose prefix is store-relative.ObjectStoreUrlto carry a path prefix; key thedatafusion-clistdin://store by authority.Warning
Draft: this depends on a
deregistermethod being added toobject_store's registry (apache/arrow-rs-object-store#784). TheCargo.tomlpatch currently points at a fork branch and must be replaced with a releasedobject_storeversion before merge.Are these changes tested?
Yes:
ListingTableUrl::resolveedge cases: authority-only (prefix unchanged), prefixed (prefix stripped), collection trailing-slash preserved, query at the registered root, and percent-encoding preserved in the base key.object_store_access): a store registered at a prefixed URL receives store-relative paths (no double-prefixing) and a query succeeds; two repos coexist under one authority.Are there any user-facing changes?
Yes.
ObjectStoreUrl::parsenow accepts a path prefix (previously rejected), andObjectStoreRegistrygains aresolvemethod (defaulted, so existing implementations keep working). Behavior for authority-only registrations (s3://bucket,file://) is unchanged.