From dfabcd8febda347b8768b19541ad9e3bc235001b Mon Sep 17 00:00:00 2001 From: Robert Kruszewski Date: Fri, 7 Aug 2026 11:35:25 +0000 Subject: [PATCH] feat(jni): resolve every URL through the vortex-cloud registry, adding `hf://` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Java binding built stores through its own scheme dispatch — bespoke S3, Azure and GCS builders plus an authority-keyed cache — and keyed every read by the full URL path. That assumption held for the schemes it served, but not for `hf://`: a Hugging Face store is rooted at a repository and revision, which occupy path segments, so a full-URL-path key would send the repository name to the Hub as part of the file path. And the authority-keyed cache cannot serve `hf://` at all, since every Hub repository shares the `datasets` authority. Replace the dispatch with `vortex_cloud::Registry`, the same resolution the Python and DuckDB bindings use. `make_object_store` reports the path of the URL within the store it returns, and every caller (metadata reads, listing, deletes, globbed data sources, the writer) keys by that. Caller properties are `object_store` configuration keys already (`aws_access_key_id`, ...), so they layer over the process environment into a per-property-set registry — stores built with one caller's credentials must not serve another's requests. The hardcoded S3 endpoint/path-style/allow-http and the Azure timeout survive as defaults. A default must yield to *any* spelling of its key (`endpoint` and `aws_endpoint` are one configuration), or both spellings reach the store builder and whichever iterates last wins — so each default lists the spellings that suppress it, and a test pins that the suppressed default is fully absent. The OpenDAL-backed schemes keep a properties-native branch, since their property names (`secret_id`, ...) are the services' own rather than environment names. The crate's `opendal` feature flag is gone entirely: vortex-jni is an unpublished cdylib built exactly one way, no Rust consumer exists to opt out, and CI never exercised the off-combo, so the flag only added untested cfg branches. The dependency is now unconditional and the shipped library serves every scheme. Verified with the crate's unit tests, the Java suite (`./gradlew :vortex-jni:test`, 32 tests), and a live Hub read through `DataSource.open("hf://datasets/...")` (10k rows, 1190 columns). The S3Mock container test needs Docker and is left to CI; its requirements (http endpoint, path-style) are what the retained S3 defaults preserve. Towards #5379. Signed-off-by: Robert Kruszewski Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Lc5zw7Le2T3pakDEUdKTYd --- vortex-cloud/src/registry/mod.rs | 16 +- vortex-cloud/src/registry/tests.rs | 2 +- vortex-jni/Cargo.toml | 7 +- vortex-jni/src/data_source.rs | 40 +--- vortex-jni/src/file.rs | 15 +- vortex-jni/src/object_store.rs | 319 +++++++++++++++++------------ vortex-jni/src/writer.rs | 4 +- 7 files changed, 199 insertions(+), 204 deletions(-) diff --git a/vortex-cloud/src/registry/mod.rs b/vortex-cloud/src/registry/mod.rs index 51a4042d7ac..b854569959a 100644 --- a/vortex-cloud/src/registry/mod.rs +++ b/vortex-cloud/src/registry/mod.rs @@ -86,17 +86,12 @@ pub struct Registry { } /// Source of the configuration variables consulted when building a store. -/// -/// Tests construct a registry over a fixed set of variables rather than mutating the process -/// environment, which is unsound when tests run on multiple threads within one process (the -/// `std::env::set_var` block became `unsafe` in Rust 2024 for exactly this reason). #[derive(Debug, Default)] enum EnvSource { /// Read from the process environment. #[default] Process, /// A fixed set of variables. - #[cfg(test)] Fixed(Vec<(String, String)>), } @@ -106,7 +101,6 @@ impl EnvSource { fn lookup(&self, key: &str) -> Option { match self { EnvSource::Process => std::env::var(key).ok(), - #[cfg(test)] EnvSource::Fixed(vars) => vars .iter() .find(|(k, _)| k.eq_ignore_ascii_case(key)) @@ -120,7 +114,6 @@ impl EnvSource { EnvSource::Process => std::env::vars() .map(|(k, v)| (k.to_ascii_lowercase(), v)) .collect(), - #[cfg(test)] EnvSource::Fixed(vars) => vars .iter() .map(|(k, v)| (k.to_ascii_lowercase(), v.clone())) @@ -177,9 +170,12 @@ impl Registry { Self::default() } - /// Create a registry over a fixed set of configuration variables. - #[cfg(test)] - fn with_env(vars: I) -> Self + /// Create a registry over a fixed set of configuration variables, consulted instead of the + /// process environment when building stores. + /// + /// Lookups are case-insensitive. Pass each key at most once — the set's consumers disagree + /// on which duplicate wins. + pub fn with_vars(vars: I) -> Self where I: IntoIterator, { diff --git a/vortex-cloud/src/registry/tests.rs b/vortex-cloud/src/registry/tests.rs index 79dc931b28d..ef0931b05fa 100644 --- a/vortex-cloud/src/registry/tests.rs +++ b/vortex-cloud/src/registry/tests.rs @@ -14,7 +14,7 @@ use super::Registry; /// A registry whose S3 configuration comes from a fixed map rather than the process environment, /// so these tests neither read nor mutate global state. fn registry() -> Registry { - Registry::with_env([("AWS_REGION".to_string(), "us-east-3".to_string())]) + Registry::with_vars([("AWS_REGION".to_string(), "us-east-3".to_string())]) } /// A percent-encoded segment (as HuggingFace dataset URLs use for `refs/convert/parquet`) decodes diff --git a/vortex-jni/Cargo.toml b/vortex-jni/Cargo.toml index f511f9614d3..7b8cbd556be 100644 --- a/vortex-jni/Cargo.toml +++ b/vortex-jni/Cargo.toml @@ -33,18 +33,13 @@ tracing-subscriber = { workspace = true, features = ["env-filter"] } url = { workspace = true } vortex = { workspace = true, features = ["object_store", "files"] } vortex-arrow = { workspace = true } -vortex-cloud = { workspace = true, optional = true } +vortex-cloud = { workspace = true, features = ["hf", "opendal", "registry"] } vortex-geo = { workspace = true } vortex-parquet-variant = { workspace = true } [dev-dependencies] jni = { workspace = true, features = ["invocation"] } -[features] -# Enable OpenDAL-backed object stores (Tencent COS, Alibaba OSS) for `cos://` and `oss://` URLs. -# This pulls in the `opendal` dependency, so it is opt-in. -opendal = ["vortex-cloud/opendal"] - [lib] crate-type = ["cdylib"] diff --git a/vortex-jni/src/data_source.rs b/vortex-jni/src/data_source.rs index 516a0aa7f55..7e50b0d934b 100644 --- a/vortex-jni/src/data_source.rs +++ b/vortex-jni/src/data_source.rs @@ -30,7 +30,6 @@ use vortex::io::filesystem::FileSystemRef; use vortex::io::runtime::BlockingRuntime; use vortex::io::session::RuntimeSessionExt; use vortex::scan::DataSourceRef; -use vortex::utils::aliases::hash_map::HashMap; use vortex_arrow::ArrowSessionExt; use crate::RUNTIME; @@ -93,23 +92,11 @@ pub extern "system" fn Java_dev_vortex_jni_NativeDataSource_open( .map(|g| parse_uri_or_path(g.as_str())) .collect::>()?; - let mut fs_cache: HashMap = HashMap::new(); - for glob_url in &glob_urls { - let base = base_url(glob_url); - if !fs_cache.contains_key(&base) { - let fs = object_store_fs(glob_url, &properties, session.handle())?; - fs_cache.insert(base, fs); - } - } - + // Glob by the path the resolver reports — only it knows how deep each store is mounted. let mut builder = MultiFileDataSource::new(session.clone()); for glob_url in &glob_urls { - let base = base_url(glob_url); - let fs = fs_cache - .get(&base) - .cloned() - .unwrap_or_else(|| unreachable!("fs cached for every base url")); - builder = builder.with_glob(glob_url.path(), Some(fs)); + let (fs, glob) = object_store_fs(glob_url, &properties, session.handle())?; + builder = builder.with_glob(glob, Some(fs)); } let inner = RUNTIME @@ -190,13 +177,6 @@ pub extern "system" fn Java_dev_vortex_jni_NativeDataSource_openFiles( }) } -/// URL with the path cleared, used as a cache key for filesystem reuse. -fn base_url(url: &Url) -> Url { - let mut base = url.clone(); - base.set_path(""); - base -} - #[unsafe(no_mangle)] pub extern "system" fn Java_dev_vortex_jni_NativeDataSource_free( _env: EnvUnowned, @@ -274,17 +254,3 @@ pub extern "system" fn Java_dev_vortex_jni_NativeDataSource_byteSize( Ok(()) }); } - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn test_base_url_strips_path() { - let url = Url::parse("s3://bucket/a/b/c").unwrap(); - let base = base_url(&url); - assert_eq!(base.scheme(), "s3"); - assert_eq!(base.host_str(), Some("bucket")); - assert_eq!(base.path(), ""); - } -} diff --git a/vortex-jni/src/file.rs b/vortex-jni/src/file.rs index 1044ed7dc01..5369f9b965b 100644 --- a/vortex-jni/src/file.rs +++ b/vortex-jni/src/file.rs @@ -16,7 +16,6 @@ use jni::objects::JObjectArray; use jni::objects::JString; use jni::sys::jlong; use jni::sys::jobject; -use object_store::path::Path; use vortex::buffer::ByteBuffer; use vortex::error::VortexResult; use vortex::error::vortex_err; @@ -159,11 +158,7 @@ pub extern "system" fn Java_dev_vortex_jni_NativeFiles_readMetadata( let url = parse_uri_or_path(&uri)?; let properties = extract_properties(env, &options)?; - let fs = object_store_fs(&url, &properties, session.handle())?; - // `FileSystem` keys are literal, already-decoded paths, so decode as `listFiles` does. - let path = Path::from_url_path(url.path()) - .map_err(|_| vortex_err!("cannot parse uri as object_store Path"))? - .to_string(); + let (fs, path) = object_store_fs(&url, &properties, session.handle())?; let source = RUNTIME.block_on(async { fs.open_read(&path).await })?; let segments = read_metadata_segments(session, source, &path, None)?; @@ -226,11 +221,9 @@ pub extern "system" fn Java_dev_vortex_jni_NativeFiles_listFiles( let properties = extract_properties(env, &options)?; - let fs = object_store_fs(&url, &properties, session.handle())?; - let prefix = Path::from_url_path(url.path()) - .map_err(|_| vortex_err!("cannot parse root_path as object_store Path"))?; + let (fs, prefix) = object_store_fs(&url, &properties, session.handle())?; - let mut stream = fs.list(prefix.as_ref()); + let mut stream = fs.list(&prefix); let paths_vec = RUNTIME.block_on(async move { let mut paths = Vec::new(); @@ -287,7 +280,7 @@ pub extern "system" fn Java_dev_vortex_jni_NativeFiles_delete( let properties = extract_properties(env, &options)?; - let fs = object_store_fs(&store_url, &properties, session.handle())?; + let (fs, _path) = object_store_fs(&store_url, &properties, session.handle())?; RUNTIME.block_on(async { for uri in delete_uris { diff --git a/vortex-jni/src/object_store.rs b/vortex-jni/src/object_store.rs index a2534fccda0..591db657034 100644 --- a/vortex-jni/src/object_store.rs +++ b/vortex-jni/src/object_store.rs @@ -1,182 +1,142 @@ // SPDX-License-Identifier: Apache-2.0 // SPDX-FileCopyrightText: Copyright the Vortex contributors -use std::str::FromStr; use std::sync::Arc; use std::sync::LazyLock; -use std::time::Duration; -use object_store::ClientOptions; use object_store::ObjectStore; -use object_store::ObjectStoreScheme; -use object_store::aws::AmazonS3Builder; -use object_store::aws::AmazonS3ConfigKey; -use object_store::azure::AzureConfigKey; -use object_store::azure::MicrosoftAzureBuilder; -use object_store::gcp::GoogleCloudStorageBuilder; -use object_store::gcp::GoogleConfigKey; -use object_store::local::LocalFileSystem; +use object_store::path::Path; +use object_store::registry::ObjectStoreRegistry; use parking_lot::Mutex; use url::Url; use vortex::error::VortexError; use vortex::error::VortexResult; -use vortex::error::vortex_bail; +use vortex::error::vortex_err; use vortex::io::compat::Compat; use vortex::io::filesystem::FileSystemRef; use vortex::io::object_store::ObjectStoreFileSystem; use vortex::io::runtime::Handle; use vortex::utils::aliases::hash_map::HashMap; +use vortex_cloud::Registry; +/// Resolve `url` to a filesystem plus the path of the URL *within* it — not every scheme mounts +/// its store at the URL authority, so callers must key their reads by the returned path. pub(crate) fn object_store_fs( url: &Url, properties: &HashMap, handle: Handle, -) -> VortexResult { - let object_store = make_object_store(url, properties)?; +) -> VortexResult<(FileSystemRef, String)> { + let (object_store, path) = make_object_store(url, properties)?; let object_store = Arc::new(Compat::new(object_store)) as Arc; - Ok(Arc::new(ObjectStoreFileSystem::new(object_store, handle))) + Ok(( + Arc::new(ObjectStoreFileSystem::new(object_store, handle)), + path.to_string(), + )) } -/// Process-wide cache of constructed object stores, keyed by URL + properties so that repeated -/// requests against the same bucket/configuration share a single client. -static OBJECT_STORES: LazyLock>>> = +/// Defaults applied when neither the caller's properties nor the environment say otherwise. +/// A default must yield to *any* spelling of its key — both spellings would otherwise reach the +/// store builder, which applies whichever iterates last — so each lists its spellings. +const DEFAULT_VARS: [(&str, &[&str], &str); 4] = [ + ( + "aws_endpoint", + &[ + "aws_endpoint", + "aws_endpoint_url", + "endpoint", + "endpoint_url", + ], + "https://s3.amazonaws.com", + ), + ( + "aws_virtual_hosted_style_request", + &[ + "aws_virtual_hosted_style_request", + "virtual_hosted_style_request", + ], + "false", + ), + ("aws_allow_http", &["aws_allow_http", "allow_http"], "true"), + ("azure_timeout", &["azure_timeout", "timeout"], "120s"), +]; + +/// Registries keyed by the caller's properties: a store built with one caller's credentials +/// must not serve another's requests. +static REGISTRIES: LazyLock>>> = LazyLock::new(|| Mutex::new(HashMap::new())); -#[expect(clippy::cognitive_complexity)] +/// Process-wide cache of OpenDAL-backed stores, keyed by URL authority + properties. +static OPENDAL_STORES: LazyLock>>> = + LazyLock::new(|| Mutex::new(HashMap::new())); + +/// Resolve `url` to a store plus the path of the URL within it, configured from the caller's +/// `object_store` properties over the process environment over [`DEFAULT_VARS`]. pub(crate) fn make_object_store( url: &Url, properties: &HashMap, -) -> VortexResult> { +) -> VortexResult<(Arc, Path)> { let start = std::time::Instant::now(); - // The cache key depends only on the URL authority + sorted properties, so we can hoist it - // above the scheme dispatch. This lets every store (including OpenDAL-backed ones) share - // a single client across repeated requests against the same bucket/configuration. - let cache_key = url_cache_key(url, properties); - - { - if let Some(cached) = OBJECT_STORES.lock().get(&cache_key) { - return Ok(Arc::clone(cached)); - } - // guard dropped at close of scope - } - - // OpenDAL-backed stores (Tencent COS, Alibaba OSS) use schemes that `object_store` does not - // recognize natively. Resolve them via the optional `opendal` feature, and cache the result so - // subsequent calls for the same URL share a single client. Asking `supports_scheme` rather - // than matching scheme strings here keeps this call site correct as services are added. - #[cfg(feature = "opendal")] + // OpenDAL schemes take the services' own property names (e.g. `secret_id`), not environment + // names, and mount at the URL authority, which makes the authority-keyed cache sound. if vortex_cloud::opendal::supports_scheme(url.scheme()) { + let path = Path::from_url_path(url.path()) + .map_err(|e| vortex_err!("cannot parse url path as object_store Path: {e}"))?; + let cache_key = url_cache_key(url, properties); + { + if let Some(cached) = OPENDAL_STORES.lock().get(&cache_key) { + return Ok((Arc::clone(cached), path)); + } + } let store = vortex_cloud::opendal::make_opendal_store(url, properties) .map_err(|e| VortexError::from(object_store::Error::from(e)))?; - return cache_and_return(store, url, properties, &start); + OPENDAL_STORES.lock().insert(cache_key, Arc::clone(&store)); + return Ok((store, path)); } - let (scheme, _) = ObjectStoreScheme::parse(url) - .map_err(|error| VortexError::from(object_store::Error::from(error)))?; + let (store, path) = registry_for(properties) + .resolve(url) + .map_err(VortexError::from)?; - // Configure extra properties on that scheme instead. - let store: Arc = match scheme { - ObjectStoreScheme::Local => { - tracing::trace!("using LocalFileSystem object store"); - Arc::new(LocalFileSystem::default()) - } - ObjectStoreScheme::AmazonS3 => { - tracing::trace!("using AmazonS3 object store"); - let mut builder = AmazonS3Builder::new() - .with_url(url.to_string()) - // Use generic S3 endpoint to avoid DNS resolution issues with region-specific endpoints - .with_endpoint("https://s3.amazonaws.com") - // Use path-style URLs - .with_virtual_hosted_style_request(false) - // Allow user to override endpoint to HTTP endpoints, e.g. LocalStack, Minio - .with_allow_http(true); - - // Try to load credentials from environment if not provided in properties - if !properties.contains_key("access_key_id") - && let Ok(access_key) = std::env::var("AWS_ACCESS_KEY_ID") - { - builder = builder.with_access_key_id(access_key); - } - if !properties.contains_key("secret_access_key") - && let Ok(secret_key) = std::env::var("AWS_SECRET_ACCESS_KEY") - { - builder = builder.with_secret_access_key(secret_key); - } - if !properties.contains_key("region") - && let Ok(region) = std::env::var("AWS_DEFAULT_REGION") - { - builder = builder.with_region(region); - } + let duration = start.elapsed(); + tracing::debug!("make_object_store latency = {duration:?}"); - for (key, val) in properties { - if let Ok(config_key) = AmazonS3ConfigKey::from_str(key.as_str()) { - builder = builder.with_config(config_key, val); - } else { - tracing::warn!("Skipping unknown Amazon S3 config key: {key}"); - } - } + Ok((store, path)) +} - Arc::new(builder.build()?) - } - ObjectStoreScheme::MicrosoftAzure => { - tracing::trace!("using MicrosoftAzure object store"); - - // NOTE(aduffy): anecdotally Azure often times out after 30 seconds, this bumps us up - // to avoid that. - let client_opts = ClientOptions::new().with_timeout(Duration::from_secs(120)); - let mut builder = MicrosoftAzureBuilder::new() - .with_url(url.to_string()) - .with_client_options(client_opts); - for (key, val) in properties { - if let Ok(config_key) = AzureConfigKey::from_str(key.as_str()) { - tracing::warn!("setting azure config {key:?} = {val}"); - builder = builder.with_config(config_key, val); - } else { - tracing::warn!("Skipping unknown Azure config key: {key}"); - } - } +/// The registry serving `properties`, created on first use. +fn registry_for(properties: &HashMap) -> Arc { + let mut sorted_props: Vec<_> = properties.iter().collect(); + sorted_props.sort_by_key(|(k, _)| *k); + let key: String = sorted_props + .iter() + .map(|(k, v)| format!("{k}={v}")) + .collect::>() + .join(","); - Arc::new(builder.build()?) - } - ObjectStoreScheme::GoogleCloudStorage => { - tracing::trace!("using GoogleCloudStorage object store"); - - let mut builder = GoogleCloudStorageBuilder::new().with_url(url.to_string()); - for (key, val) in properties { - if let Ok(config_key) = GoogleConfigKey::from_str(key.as_str()) { - builder = builder.with_config(config_key, val); - } else { - tracing::warn!("Skipping unknown Google Cloud Storage config key: {key}"); - } + let mut registries = REGISTRIES.lock(); + Arc::clone(registries.entry(key).or_insert_with(|| { + // Later inserts win; keys are lowercased because the registry matches case-insensitively + // and requires each key at most once. + let mut vars: HashMap = std::env::vars() + .map(|(k, v)| (k.to_ascii_lowercase(), v)) + .collect(); + vars.extend( + properties + .iter() + .map(|(k, v)| (k.to_ascii_lowercase(), v.clone())), + ); + for (canonical, spellings, value) in DEFAULT_VARS { + if spellings + .iter() + .all(|spelling| !vars.contains_key(*spelling)) + { + vars.insert(canonical.to_string(), value.to_string()); } - - Arc::new(builder.build()?) - } - store => { - vortex_bail!("Unsupported store scheme: {store:?}"); } - }; - - cache_and_return(store, url, properties, &start) -} - -/// Insert the built store into the process-wide cache (keyed by URL + properties) and return it, -/// logging the construction latency. -fn cache_and_return( - store: Arc, - url: &Url, - properties: &HashMap, - start: &std::time::Instant, -) -> VortexResult> { - let cache_key = url_cache_key(url, properties); - OBJECT_STORES.lock().insert(cache_key, Arc::clone(&store)); - - let duration = start.elapsed(); - tracing::debug!("make_object_store latency = {duration:?}"); - - Ok(store) + Arc::new(Registry::with_vars(vars)) + })) } fn url_cache_key(url: &Url, properties: &HashMap) -> String { @@ -195,3 +155,90 @@ fn url_cache_key(url: &Url, properties: &HashMap) -> String { props_str, ) } + +#[cfg(test)] +mod tests { + use std::fmt::Write; + + use vortex::error::vortex_err; + + use super::*; + + fn parse(url: &str) -> VortexResult { + Url::parse(url).map_err(|e| vortex_err!("{e}")) + } + + #[test] + fn test_hf_url_reports_the_in_repository_path() -> VortexResult<()> { + let url = parse("hf://datasets/org/name/data/train.vortex")?; + let (_store, path) = make_object_store(&url, &HashMap::new())?; + + assert_eq!(path.as_ref(), "data/train.vortex"); + Ok(()) + } + + #[test] + fn test_hf_repositories_do_not_share_a_store() -> VortexResult<()> { + let a = parse("hf://datasets/org/one/train.vortex")?; + let b = parse("hf://datasets/org/two/train.vortex")?; + + let (store_a, _) = make_object_store(&a, &HashMap::new())?; + let (store_b, _) = make_object_store(&b, &HashMap::new())?; + + assert!(!Arc::ptr_eq(&store_a, &store_b)); + Ok(()) + } + + /// `object_store` offers no way to read a store's configuration back; the Debug output is + /// the only observable. + #[test] + #[expect(clippy::use_debug)] + fn test_s3_properties_and_defaults_reach_the_store() -> VortexResult<()> { + let url = parse("s3://bucket/dir/data%20file.vortex")?; + let properties = HashMap::from_iter([("region".to_string(), "eu-central-9".to_string())]); + + let (store, path) = make_object_store(&url, &properties)?; + assert_eq!(path.as_ref(), "dir/data file.vortex"); + + let mut debug_str = String::new(); + write!(&mut debug_str, "{store:?}").map_err(|e| vortex_err!("{e}"))?; + assert!(debug_str.contains("eu-central-9"), "{debug_str}"); + assert!(debug_str.contains("s3.amazonaws.com"), "{debug_str}"); + Ok(()) + } + + #[test] + #[expect(clippy::use_debug)] + fn test_property_overrides_default_endpoint() -> VortexResult<()> { + let url = parse("s3://bucket/key.vortex")?; + let properties = HashMap::from_iter([ + ("region".to_string(), "eu-central-9".to_string()), + ("endpoint".to_string(), "http://localhost:9000".to_string()), + ]); + + let (store, _path) = make_object_store(&url, &properties)?; + + let mut debug_str = String::new(); + write!(&mut debug_str, "{store:?}").map_err(|e| vortex_err!("{e}"))?; + // The default must be fully suppressed, not merely accompanied — `endpoint` and + // `aws_endpoint` are spellings of one key. + assert!(debug_str.contains("localhost:9000"), "{debug_str}"); + assert!(!debug_str.contains("s3.amazonaws.com"), "{debug_str}"); + Ok(()) + } + + #[test] + fn test_stores_are_shared_per_property_set() -> VortexResult<()> { + let url = parse("s3://bucket/key.vortex")?; + let a = HashMap::from_iter([("region".to_string(), "eu-central-9".to_string())]); + let b = HashMap::from_iter([("region".to_string(), "us-west-7".to_string())]); + + let (store_a1, _) = make_object_store(&url, &a)?; + let (store_a2, _) = make_object_store(&url, &a)?; + let (store_b, _) = make_object_store(&url, &b)?; + + assert!(Arc::ptr_eq(&store_a1, &store_a2)); + assert!(!Arc::ptr_eq(&store_a1, &store_b)); + Ok(()) + } +} diff --git a/vortex-jni/src/writer.rs b/vortex-jni/src/writer.rs index 18c6a5fa101..794aa1cc65c 100644 --- a/vortex-jni/src/writer.rs +++ b/vortex-jni/src/writer.rs @@ -89,9 +89,7 @@ fn resolve_store( .map_err(|_| vortex_err!("invalid file URL: {url_or_path}"))?; Ok(ResolvedStore::Path(path)) } else { - let path = ObjectStorePath::from_url_path(url.path()) - .map_err(|_| vortex_err!("invalid object_store path: {}", url.path()))?; - let store = make_object_store(&url, properties)?; + let (store, path) = make_object_store(&url, properties)?; Ok(ResolvedStore::ObjectStore(store, path)) } }