From 900b95699d561963369b56db06daf5038334b27d Mon Sep 17 00:00:00 2001 From: David Linton Date: Mon, 24 Aug 2026 18:03:21 +0200 Subject: [PATCH] Resolve object-store URLs in MultiFileDataSource via the cloud registry MultiFileDataSource::with_glob documents S3/GCS support through an explicit FileSystemRef, but a bare URL with fs: None was treated as a local path - cwd-joined and mangled - so object-store URLs only worked for callers that carry their own store wiring (the Python and Java bindings each do). Anything on the C FFI hit a local-filesystem error. Resolve them in core instead, so every consumer gets the same behavior: with_glob leaves object-store URLs untouched, and build() routes a (url, None) source through the shared vortex-cloud Registry - the same env-based resolution as the Python binding's resolve_store - wrapping the store in Compat and an ObjectStoreFileSystem. Parse failures, file URLs, and single-character schemes (Windows drive paths) keep resolving locally, and the behavior is gated behind a new vortex-file feature object_store_registry, threaded through the vortex facade and enabled by vortex-ffi. Tests at both levels assert that an unconfigured az:// URL fails in the registry's store builder rather than as an unmatched local glob. Signed-off-by: David Linton --- Cargo.lock | 1 + vortex-cloud/Cargo.toml | 4 +- vortex-ffi/Cargo.toml | 5 +- vortex-ffi/src/data_source.rs | 32 ++++++++++ vortex-file/Cargo.toml | 4 ++ vortex-file/src/multi/mod.rs | 107 ++++++++++++++++++++++++++++++++-- vortex/Cargo.toml | 6 +- 7 files changed, 151 insertions(+), 8 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0010a4e6cec..0a79f55ac69 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -10151,6 +10151,7 @@ dependencies = [ "vortex-btrblocks", "vortex-buffer", "vortex-bytebool", + "vortex-cloud", "vortex-datetime-parts", "vortex-decimal-byte-parts", "vortex-edition", diff --git a/vortex-cloud/Cargo.toml b/vortex-cloud/Cargo.toml index 7ac7de96104..08d5519bf2d 100644 --- a/vortex-cloud/Cargo.toml +++ b/vortex-cloud/Cargo.toml @@ -35,7 +35,9 @@ tempfile = { workspace = true } tokio = { workspace = true, features = ["rt", "macros"] } vortex-array = { workspace = true } vortex-error = { workspace = true, features = ["object_store"] } -vortex-file = { workspace = true, features = ["object_store", "tokio"] } +# Path-only (no version) so the publish graph stays acyclic now that +# vortex-file optionally depends on this crate. +vortex-file = { path = "../vortex-file", features = ["object_store", "tokio"] } vortex-io = { workspace = true, features = ["object_store", "tokio"] } vortex-layout = { workspace = true } diff --git a/vortex-ffi/Cargo.toml b/vortex-ffi/Cargo.toml index a64aabac03f..8f2bd2385af 100644 --- a/vortex-ffi/Cargo.toml +++ b/vortex-ffi/Cargo.toml @@ -29,7 +29,10 @@ mimalloc = { workspace = true, optional = true } paste = { workspace = true } tracing = { workspace = true, features = ["log"] } tracing-subscriber = { workspace = true, features = ["env-filter"] } -vortex = { workspace = true, features = ["object_store"] } +vortex = { workspace = true, features = [ + "object_store", + "object_store_registry", +] } vortex-arrow = { workspace = true } [dev-dependencies] diff --git a/vortex-ffi/src/data_source.rs b/vortex-ffi/src/data_source.rs index 50d31b3036d..6deb4515a8a 100644 --- a/vortex-ffi/src/data_source.rs +++ b/vortex-ffi/src/data_source.rs @@ -253,6 +253,38 @@ mod tests { } } + #[test] + #[cfg_attr(miri, ignore)] + fn test_object_store_url_routing() { + unsafe { + let session = vx_session_new(); + let mut error = ptr::null_mut(); + + // A URL scheme must route through the object-store registry, not + // the local filesystem. The Azure builder deterministically + // fails without configuration, and that failure can only arise + // on the registry path. + let url = vx_view::from_str("az://account/container/missing.vortex"); + let opts = vx_data_source_options { + paths: &raw const url, + paths_len: 1, + }; + let ds = vx_data_source_new(session, &raw const opts, &raw mut error); + assert!(ds.is_null()); + assert!(!error.is_null()); + let message = crate::error::vx_error_message(error).as_str().unwrap(); + // Local-path handling would report an unmatched glob pattern; + // the registry path fails earlier, in the store builder. + assert!( + !message.contains("glob"), + "URL was resolved as a local path: {message}" + ); + assert_error(error); + + vx_session_free(session); + } + } + #[test] #[cfg_attr(miri, ignore)] fn test_row_count() { diff --git a/vortex-file/Cargo.toml b/vortex-file/Cargo.toml index bddaff61a02..afe825bec75 100644 --- a/vortex-file/Cargo.toml +++ b/vortex-file/Cargo.toml @@ -36,6 +36,7 @@ vortex-array = { workspace = true } vortex-btrblocks = { workspace = true } vortex-buffer = { workspace = true } vortex-bytebool = { workspace = true } +vortex-cloud = { workspace = true, optional = true, features = ["registry"] } vortex-datetime-parts = { workspace = true } vortex-decimal-byte-parts = { workspace = true } @@ -79,6 +80,9 @@ harness = false [features] default = ["wasm-bindgen"] object_store = ["dep:object_store", "vortex-io/object_store", "tokio"] +# Resolve object-store URLs (s3://, gs://, az://, https://, ...) passed to +# `MultiFileDataSource::with_glob` through the `vortex-cloud` registry. +object_store_registry = ["object_store", "dep:vortex-cloud"] tokio = [ "dep:tokio", "vortex-error/tokio", diff --git a/vortex-file/src/multi/mod.rs b/vortex-file/src/multi/mod.rs index b8f371d05b4..7ca82cd1d6e 100644 --- a/vortex-file/src/multi/mod.rs +++ b/vortex-file/src/multi/mod.rs @@ -91,9 +91,18 @@ impl MultiFileDataSource { /// filesystem to use the local filesystem (auto-created in [`Self::build`]). /// /// Relative paths are resolved against the process working directory. + /// + /// With the `object_store_registry` feature enabled, an object-store URL (`s3://`, `gs://`, + /// `az://`, `https://`, ...) passed with `None` is instead resolved through the + /// `vortex-cloud` registry, configured from environment variables. pub fn with_glob(mut self, glob: impl Into, fs: Option) -> Self { let glob = glob.into(); - let glob = if fs.is_none() && std::path::Path::new(&glob).is_relative() { + let glob = if object_store_url(&glob).is_some() { + // Object-store URLs are resolved against the registry in + // `build`; joining them onto the working directory or trimming + // leading slashes would corrupt them. + glob + } else if fs.is_none() && std::path::Path::new(&glob).is_relative() { std::env::current_dir() .map(|cwd| cwd.join(&glob).to_string_lossy().into_owned()) .unwrap_or(glob) @@ -142,12 +151,26 @@ impl MultiFileDataSource { stream::iter(self.glob_sources.into_iter().map(|(glob, maybe_fs)| { // Use the provided filesystem, or fall back to the local filesystem. // We know local_fs is Some when maybe_fs is None (by construction above). - let fs = maybe_fs - .or_else(|| local_fs.as_ref().map(Arc::clone)) - .unwrap_or_else(|| { + let local_fs = local_fs.clone(); + let session = self.session.clone(); + async move { + // An object-store URL without an explicit filesystem is + // resolved through the shared registry; everything else + // falls back to the local filesystem as before. + let (glob, maybe_fs) = if maybe_fs.is_none() { + match object_store_url(&glob) { + Some(url) => { + let (store_path, fs) = registry_filesystem(&url, &session)?; + (store_path, Some(fs)) + } + None => (glob, None), + } + } else { + (glob, maybe_fs) + }; + let fs = maybe_fs.or(local_fs).unwrap_or_else(|| { unreachable!("local_fs is set when any glob lacks a filesystem") }); - async move { let files: Vec = fs.glob(&glob)?.try_collect().await?; Ok::<_, VortexError>( files @@ -202,6 +225,54 @@ impl MultiFileDataSource { } } +/// Parse `glob` as an object-store URL, mirroring the resolution of the +/// Python binding's `resolve_store`: a parse failure or a `file` scheme is a +/// local path. Single-character schemes are also treated as local so Windows +/// drive paths (`C:/data.vortex`) never match. +#[cfg(feature = "object_store_registry")] +fn object_store_url(glob: &str) -> Option { + let url = url::Url::parse(glob).ok()?; + (url.scheme().len() > 1 && url.scheme() != "file").then_some(url) +} + +#[cfg(not(feature = "object_store_registry"))] +fn object_store_url(_glob: &str) -> Option { + None +} + +/// Resolve `url` through the shared [`vortex_cloud::Registry`], returning the +/// store-relative path and a filesystem over the store. The store's I/O is +/// bridged onto a tokio-compatible thread via `Compat`, as in the Python +/// binding. +#[cfg(feature = "object_store_registry")] +fn registry_filesystem( + url: &url::Url, + session: &VortexSession, +) -> VortexResult<(String, FileSystemRef)> { + use std::sync::LazyLock; + + use object_store::registry::ObjectStoreRegistry; + use vortex_cloud::Registry; + use vortex_io::compat::Compat; + use vortex_io::object_store::ObjectStoreFileSystem; + use vortex_io::session::RuntimeSessionExt; + + static REGISTRY: LazyLock = LazyLock::new(Registry::default); + + let (store, path) = REGISTRY.resolve(url)?; + let store = Arc::new(Compat::new(store)); + let fs: FileSystemRef = Arc::new(ObjectStoreFileSystem::new(store, session.handle())); + Ok((path.to_string(), fs)) +} + +#[cfg(not(feature = "object_store_registry"))] +fn registry_filesystem( + _url: &url::Url, + _session: &VortexSession, +) -> VortexResult<(String, FileSystemRef)> { + unreachable!("object_store_url returns None without the object_store_registry feature") +} + /// Creates a local filesystem backed by `object_store::local::LocalFileSystem`. // TODO(ngates): create a native file system without an object_store dependency. // Turns out it's not a trivial change because we have always used object_store with its own @@ -502,4 +573,30 @@ mod tests { ); Ok(()) } + + #[cfg(feature = "object_store_registry")] + #[tokio::test] + async fn test_object_store_url_resolution() { + let session = array_session() + .with::() + .with::() + .with::(); + + // A URL scheme resolves through the registry, not the local + // filesystem. The Azure builder deterministically fails without + // configuration, and that failure can only arise on the registry + // path — local-path handling would report an unmatched glob. + let message = match MultiFileDataSource::new(session) + .with_glob("az://account/container/missing.vortex", None) + .build() + .await + { + Ok(_) => panic!("unconfigured az:// URL must not resolve"), + Err(e) => e.to_string(), + }; + assert!( + !message.contains("glob"), + "URL was resolved as a local path: {message}" + ); + } } diff --git a/vortex/Cargo.toml b/vortex/Cargo.toml index fc3f9e420f8..0cdb75ad587 100644 --- a/vortex/Cargo.toml +++ b/vortex/Cargo.toml @@ -75,7 +75,11 @@ files = ["dep:vortex-file"] memmap2 = ["vortex-buffer/memmap2"] object_store = ["vortex-file?/object_store", "vortex-io/object_store"] # The URL -> ObjectStore registry, plus the cloud backends it resolves URLs to. -object_store_registry = ["object_store", "vortex-cloud/registry"] +object_store_registry = [ + "object_store", + "vortex-cloud/registry", + "vortex-file?/object_store_registry", +] # The Hugging Face Hub (`hf://`) in the object store registry. Served over HTTP, so unlike # `opendal` it pulls in no cloud SDK. hf = ["object_store_registry", "vortex-cloud/hf"]