refactor(object-store): share one URL registry across Python, Java and DuckDB - #8999
Open
robert3005 wants to merge 5 commits into
Open
refactor(object-store): share one URL registry across Python, Java and DuckDB#8999robert3005 wants to merge 5 commits into
robert3005 wants to merge 5 commits into
Conversation
…d DuckDB URL-to-ObjectStore resolution had grown three independent implementations: `vortex-python`'s `Registry`, `vortex-jni`'s `make_object_store`, and `vortex-duckdb`'s two-scheme match. Only the first knew about the OpenDAL-backed schemes, and it was `pub(crate)`, so no Rust consumer could reach it. Hoist that registry into `vortex-io::object_store::Registry` as public API and point the bindings at it: - `vortex-python` drops its private copy and uses the shared one. - `vortex-duckdb` replaces its `file`/`s3`-only match (no caching, no credentials beyond `from_env`) with the registry, gaining Azure, GCS, HTTP and per-bucket client reuse. - `vortex-jni` keeps its property-driven builders, which have S3 behaviour the registry does not reproduce, but now asks `supports_scheme` instead of hard-coding `"cos"`. The registry lives behind a new `object_store_registry` feature so that plain file IO does not pull in the cloud HTTP stack: `vortex-file` with `object_store` still resolves zero `reqwest` in its dependency tree. Alibaba Cloud OSS support is added alongside the existing COS support, behind per-service `cos` and `oss` features on `vortex-object-store-opendal`, which is now publishable so that published crates can depend on it. The crate is split into per-service modules and gains `supports_scheme` / `SUPPORTED_SCHEMES` so callers stop matching scheme strings themselves. OSS reads `ALIBABA_CLOUD_*` (with the underscore) and has no `disable_config_load`; its nearest knob, `skip_signature`, is exposed for public buckets. Store construction no longer depends on ambient process state in tests: the registry takes its configuration from an injected source, replacing the three `unsafe std::env::set_var` blocks the moved tests used. The suite now passes with a deliberately hostile environment set. Signed-off-by: "Claude" <robert@spiraldb.com>
Merging this PR will improve performance by 17.43%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | WallTime | cuda/bitpacked_u8/unpack/3bw[100M] |
351.5 µs | 299.3 µs | +17.43% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing claude/opendal-integration-review-1h8f3a (9dc7a81) with develop (b15394a)
Footnotes
-
3 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
I, Claude <robert@spiraldb.com>, hereby add my Signed-off-by to this commit: c85a848 Signed-off-by: Claude <robert@spiraldb.com>
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
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.
Rationale for this change
Follow-up to #8845, which added the OpenDAL-backed
cos://store.URL-to-
ObjectStoreresolution had grown three independent implementations:vortex-python'sRegistryvortex-jni'smake_object_storevortex-duckdb'sresolve_filesystemfrom_envonlyOnly the first knew about the OpenDAL-backed schemes, and it was
pub(crate), so no Rustconsumer could reach it. Adding a provider meant touching every caller — which is exactly
what #8845 had to do, and what the next provider would have to do again.
This also unblocks a concrete gap:
vortex-object-store-opendalwaspublish = false, so nopublished crate could depend on it.
What changes are included in this PR?
Hoist the registry into
vortex-io.vortex-python's privateRegistrybecomes publicvortex_io::object_store::Registry, and the bindings point at it:vortex-pythondrops its private copy (309 lines deleted) and uses the shared one.vortex-duckdbreplaces itsfile/s3-only match with the registry, gaining Azure, GCS,HTTP and per-bucket client reuse.
vortex-jnikeeps its property-driven builders — they have S3 behaviour the registry does notreproduce — but asks
supports_schemeinstead of hard-coding"cos", so the call site stayscorrect as services are added.
Feature-gate the cloud stack. The registry lives behind a new
object_store_registryfeature so plain file IO does not pull in the cloud HTTP stack:
vortex-filewithobject_storestill resolves zeroreqwestin its dependency tree.Alibaba Cloud OSS. Added alongside COS, behind per-service
cos/ossfeatures. The crateis split into per-service modules and gains
supports_scheme/SUPPORTED_SCHEMESso callersstop matching scheme strings. OSS reads
ALIBABA_CLOUD_*(with the underscore) and has nodisable_config_load; its nearest knob,skip_signature, is exposed for public buckets.Make
vortex-object-store-opendalpublishable, so published crates can depend on it.opendal/object_store_opendalmove to[workspace.dependencies]alongside every otherthird-party dependency.
Remove ambient process state from tests. The registry takes its configuration from an
injected
EnvSource, replacing the threeunsafe std::env::set_varblocks the moved testsused.
set_varbecameunsafein Rust 2024 precisely becausecargo testruns tests onmultiple threads in one process; the suite now passes under a deliberately hostile environment.
Test coverage for the moved registry (
vortex-io/src/object_store/registry/tests.rs, 8 tests):resolution, per-key caching,
Arc::ptr_eqon two objects in one bucket, prefix registration,empty-path
file://, registered-store-wins-over-build, and OpenDAL scheme dispatch.What APIs are changed? Are there any user-facing changes?
New public Rust API:
vortex_io::object_store::Registry(+Registry::new), gated on the newobject_store_registryfeature; re-exported as
vortex::io::object_store::Registry.vortex-io/vortexfeaturesobject_store_registryandopendal.vortex_object_store_opendal::{supports_scheme, SUPPORTED_SCHEMES, OSS_SCHEME, OssConfig, make_oss_store},and per-service
cos/ossfeatures. The crate is now published.vortex-duckdbgains anopendalfeature.No Python-facing API change:
vortex-pythonswaps a private implementation for the shared one.DuckDB users gain Azure/GCS/HTTP scheme support that previously errored with
Unsupported URL scheme.Checks
cargo check -p vortex-object-store-opendal --all-featurescargo check -p vortex-io --all-featurescargo check -p vortex-python --all-features --all-targetscargo check -p vortex-jni --all-features --all-targetscargo check -p vortex --features files,tokio,object_store,object_store_registrycargo test -p vortex-object-store-opendal --all-featurescargo test -p vortex-io --all-featurescargo test -p vortex-python --all-features --libcargo clippy -p vortex-object-store-opendal -p vortex-io -p vortex-python -p vortex-jni --all-targets --all-featurescargo clippy -p vortex-object-store-opendal -p vortex-io --no-default-featurescargo +nightly fmt --all --checkcargo metadata --lockedCargo.lockup to dateuv run --all-packages make -C docs clean html--fail-on-warning)uv run --all-packages make -C docs clean doctestCould not run locally:
cargo check -p vortex-duckdb— the build script downloads DuckDBsource from
github.com/duckdb/duckdb/archive, which returns 403 through this environment's proxy(
codeload.github.comtoo). Thevortex-duckdbchange is 2 files; I verified by inspection thatvortex::io::object_store::Registryis re-exported under the features the manifest requests, thatFrom<object_store::Error> for VortexErrorexists for the new?, and that no import becameunused. CI's DuckDB jobs will be the first real compile of it.
cargo denywas also not run (failed to build locally). The new OSS dependencies(
opendal-service-oss,reqsign-aliyun-oss) are both Apache-2.0, matching the COS equivalentsalready accepted in #8845.
🤖 Generated with Claude Code
https://claude.ai/code/session_012TyPy9MbKP2j546m1CaKLk
Generated by Claude Code