From 0d4c9352b0e576b61b2c114e5e104621dd697db2 Mon Sep 17 00:00:00 2001 From: forwardxu Date: Fri, 18 Sep 2026 11:44:49 +0800 Subject: [PATCH 1/4] fix(io): allow goosefs:/// URLs to use site.properties Stop requiring a URL host when resolving the GooseFS master so OpenDAL can still load addresses from goosefs-site.properties. --- docs/src/guide/object_store.md | 36 +++- .../src/object_store/providers/goosefs.rs | 202 ++++++++++++++++-- 2 files changed, 210 insertions(+), 28 deletions(-) diff --git a/docs/src/guide/object_store.md b/docs/src/guide/object_store.md index cdc77af9a76..7984bbb179e 100644 --- a/docs/src/guide/object_store.md +++ b/docs/src/guide/object_store.md @@ -431,9 +431,15 @@ ds = lance.dataset( ## GooseFS Configuration [GooseFS](https://cloud.tencent.com/product/goosefs) is a distributed caching -filesystem. Lance accesses GooseFS through its Master gRPC service. The URL format -is `goosefs://host:port/path`, where `host:port` is the GooseFS Master address -(default port: `9200`, may be omitted, e.g. `goosefs://10.0.0.1/path`) and +filesystem. Lance accesses GooseFS through its Master gRPC service. The URL +format is either: + +- `goosefs://host:port/path`, where `host:port` is the GooseFS Master address + (default port: `9200`, may be omitted, e.g. `goosefs://10.0.0.1/path`) +- `goosefs:///path`, with an empty authority, when the master address comes + from `goosefs_master_addr`, `GOOSEFS_MASTER_ADDR`, or + `goosefs-site.properties` + `/path` is the filesystem path within GooseFS. Manifest commits on `goosefs://` use `ConditionalPutCommitHandler` @@ -573,11 +579,25 @@ versioned manifests. For writes, the same `storageOptions(...)` setter is available on `WriteDatasetBuilder` and `WriteFragmentBuilder`. -The Master address can be resolved from (in priority order): +The Master address is resolved at OpenDAL build time (highest priority first): + +1. The `GOOSEFS_MASTER_ADDR` environment variable. +2. `goosefs.master.rpc.addresses` or `goosefs.master.hostname` in + `goosefs-site.properties` (discovered via `$GOOSEFS_CONFIG_FILE`, + `$GOOSEFS_CONF_DIR`, `$GOOSEFS_HOME/conf`, `~/.goosefs`, and + `/etc/goosefs`). This is the same file the GooseFS SDK already loads; + a Hadoop-style `goosefs:///path` URL relies on it. +3. The `goosefs_master_addr` storage option (supports HA: + `"addr1:port,addr2:port"`). +4. The host and port from the URL authority. + +Lance forwards `goosefs_master_addr` and the URL authority as OpenDAL's +`master_addr`. It does **not** require a host in the URL, so +`goosefs:///path` can rely on `goosefs-site.properties`. OpenDAL fails the +store build only when none of the sources above supply an address. -1. The `goosefs_master_addr` storage option (supports HA: `"addr1:port,addr2:port"`). -2. The `GOOSEFS_MASTER_ADDR` environment variable. -3. The host and port from the URL authority. +A site file that declares masters outranks the URL authority because the file +can carry a full HA master list, which a single URI host cannot express. `storage_options` keys **must be lowercase**. Uppercase or mixed-case spellings such as `GOOSEFS_MASTER_ADDR` are rejected with an explicit error — they are @@ -586,7 +606,7 @@ Environment variables keep the `GOOSEFS_*` form. | storage_options key | env var | Description | |---------------------|---------|-------------| -| `goosefs_master_addr` | `GOOSEFS_MASTER_ADDR` | GooseFS Master address. Supports a single address (`host:port`) or comma-separated HA addresses (`addr1:port,addr2:port`). Optional if the address is provided in the URL. | +| `goosefs_master_addr` | `GOOSEFS_MASTER_ADDR` | GooseFS Master address. Supports a single address (`host:port`) or comma-separated HA addresses (`addr1:port,addr2:port`). Optional if the address is provided in the URL, `GOOSEFS_MASTER_ADDR`, or `goosefs-site.properties`. | | `goosefs_write_type` | `GOOSEFS_WRITE_TYPE` | Write type, e.g. `MUST_CACHE`, `CACHE_THROUGH`, `THROUGH`, `ASYNC_THROUGH`. Optional. | | `goosefs_block_size` | `GOOSEFS_BLOCK_SIZE` | GooseFS block size (this is the GooseFS-side block size, not Lance's I/O block size). Accepts a raw byte count or GooseFS suffixes such as `64MB` (binary units: `1KB = 1024`). Optional. | | `goosefs_chunk_size` | `GOOSEFS_CHUNK_SIZE` | Chunk size used when reading or writing files. Accepts a raw byte count or GooseFS suffixes such as `4MB` (binary units: `1KB = 1024`). Optional. | diff --git a/rust/lance-io/src/object_store/providers/goosefs.rs b/rust/lance-io/src/object_store/providers/goosefs.rs index 90aed8fa49d..4f2ccb43f79 100644 --- a/rust/lance-io/src/object_store/providers/goosefs.rs +++ b/rust/lance-io/src/object_store/providers/goosefs.rs @@ -34,10 +34,12 @@ const STORAGE_OPTION_KEYS: &[&str] = &[ /// GooseFS object store provider. /// /// Uses OpenDAL's GooseFs service to access GooseFS via gRPC. -/// URL format: `goosefs://host:port/path` +/// URL format: `goosefs://host:port/path` or `goosefs:///path`. /// /// Where: -/// - `host:port` is the GooseFS Master address (default port: 9200) +/// - `host:port` is the GooseFS Master address (default port: 9200). It may +/// be omitted (`goosefs:///path`) when the master is supplied by +/// `storage_options`, `GOOSEFS_MASTER_ADDR`, or `goosefs-site.properties`. /// - `/path` is the filesystem path within GooseFS /// /// Path handling model (S3-style): @@ -53,6 +55,9 @@ const STORAGE_OPTION_KEYS: &[&str] = &[ /// /// Supported configuration keys (via `storage_options` or environment variables, /// resolved with priority: `storage_options` > env var > URL authority > default). +/// Master address is the exception: OpenDAL/SDK apply +/// `GOOSEFS_MASTER_ADDR` > `goosefs-site.properties` > `master_addr` +/// (the latter is filled from `storage_options` or the URL authority). /// `storage_options` keys must be lowercase; uppercase/mixed-case spellings are /// rejected rather than silently ignored. /// @@ -131,38 +136,44 @@ impl GooseFsStoreProvider { ))) } - /// Resolve the GooseFS Master address from storage_options, environment, or URL. + /// Resolve an explicit GooseFS Master address for OpenDAL's `master_addr`. /// - /// Priority: + /// Returns `None` when neither `storage_options`, `GOOSEFS_MASTER_ADDR`, nor + /// the URL authority supplies an address. Callers must omit `master_addr` + /// from the OpenDAL config in that case so the SDK can still load masters + /// from `goosefs-site.properties`. Requiring a host here would reject + /// Hadoop-style `goosefs:///path` URLs before OpenDAL runs. + /// + /// Priority among the sources this function *does* consult: /// 1. `storage_options["goosefs_master_addr"]` (supports HA: "addr1:port,addr2:port") /// 2. `GOOSEFS_MASTER_ADDR` environment variable /// 3. URL authority (host:port from the URL) - fn resolve_master_addr(url: &Url, storage_options: &StorageOptions) -> Result { + /// + /// OpenDAL then applies `GOOSEFS_MASTER_ADDR` > `goosefs-site.properties` > + /// this `master_addr` value, so a deployed site file can still supply the + /// HA master list that a single URI authority cannot express. + fn resolve_master_addr(url: &Url, storage_options: &StorageOptions) -> Option { // 1. storage_options if let Some(addr) = storage_options .0 .get("goosefs_master_addr") .filter(|v| !v.is_empty()) { - return Ok(addr.clone()); + return Some(addr.clone()); } // 2. Environment variable if let Ok(addr) = std::env::var("GOOSEFS_MASTER_ADDR") && !addr.is_empty() { - return Ok(addr); + return Some(addr); } - // 3. URL authority - let host = url.host_str().ok_or_else(|| { - Error::invalid_input( - "GooseFS URL must contain a master address (host), e.g. goosefs://host:port/path", - ) - })?; - + // 3. URL authority. Empty host (`goosefs:///path`) is valid: OpenDAL + // will resolve the master from `goosefs-site.properties`. + let host = url.host_str()?; let port = url.port().unwrap_or(DEFAULT_GOOSEFS_PORT); - Ok(format!("{}:{}", host, port)) + Some(format!("{}:{}", host, port)) } /// Resolve a storage option from storage_options or environment variable. @@ -389,8 +400,10 @@ impl ObjectStoreProvider for GooseFsStoreProvider { Self::validate_storage_option_keys(&storage_options)?; - // Resolve master address - let master_addr = Self::resolve_master_addr(&base_path, &storage_options)?; + // Resolve master address. Omit the OpenDAL key when unset so the SDK + // can still read `goosefs-site.properties` (and OpenDAL can still + // reject the build if no source at all supplies a master). + let master_addr = Self::resolve_master_addr(&base_path, &storage_options); // Resolve a stable cluster-wide root. The URL path is *not* used here // because it varies per dataset; per-request keys are supplied by @@ -399,7 +412,9 @@ impl ObjectStoreProvider for GooseFsStoreProvider { // Build OpenDAL config map let mut config_map: HashMap = HashMap::new(); - config_map.insert("master_addr".to_string(), master_addr); + if let Some(master_addr) = master_addr { + config_map.insert("master_addr".to_string(), master_addr); + } config_map.insert("root".to_string(), root); // Optional: write_type @@ -488,10 +503,21 @@ impl ObjectStoreProvider for GooseFsStoreProvider { // that stores built with different roots don't accidentally collide. let opts = StorageOptions(storage_options.cloned().unwrap_or_default()); let root = Self::resolve_root(&opts); + // `goosefs:///path` has an empty authority. Use the resolved master + // when present so two host-less URLs with different + // `goosefs_master_addr` options do not share a cache entry. When the + // master comes only from `goosefs-site.properties`, the prefix stays + // `goosefs$` — that file is process-wide, so one cached Operator is + // correct. + let authority = if url.authority().is_empty() { + Self::resolve_master_addr(url, &opts).unwrap_or_default() + } else { + url.authority().to_string() + }; if root == "/" { - Ok(format!("{}${}", url.scheme(), url.authority())) + Ok(format!("{}${}", url.scheme(), authority)) } else { - Ok(format!("{}${}#{}", url.scheme(), url.authority(), root)) + Ok(format!("{}${}#{}", url.scheme(), authority, root)) } } } @@ -499,7 +525,42 @@ impl ObjectStoreProvider for GooseFsStoreProvider { #[cfg(test)] mod tests { use super::*; + use crate::object_store::StorageOptionsAccessor; use rstest::rstest; + use serial_test::serial; + use std::ffi::OsString; + + /// Restore an environment variable when dropped so tests that mutate + /// `GOOSEFS_*` cannot leak into later cases. + struct RestoreEnv { + key: &'static str, + original: Option, + } + + impl RestoreEnv { + fn unset(key: &'static str) -> Self { + let original = std::env::var_os(key); + unsafe { std::env::remove_var(key) }; + Self { key, original } + } + + fn set(key: &'static str, value: impl AsRef) -> Self { + let original = std::env::var_os(key); + unsafe { std::env::set_var(key, value) }; + Self { key, original } + } + } + + impl Drop for RestoreEnv { + fn drop(&mut self) { + unsafe { + match &self.original { + Some(value) => std::env::set_var(self.key, value), + None => std::env::remove_var(self.key), + } + } + } + } #[test] fn test_goosefs_extract_path_basic() { @@ -509,6 +570,14 @@ mod tests { assert_eq!(path.to_string(), "data/embeddings.lance"); } + #[test] + fn test_goosefs_extract_path_hostless_url() { + let provider = GooseFsStoreProvider; + let url = Url::parse("goosefs:///data/embeddings.lance").unwrap(); + let path = provider.extract_path(&url).unwrap(); + assert_eq!(path.to_string(), "data/embeddings.lance"); + } + #[test] fn test_goosefs_extract_path_root() { let provider = GooseFsStoreProvider; @@ -636,6 +705,99 @@ mod tests { assert_eq!(addr, "10.0.0.2:9200,10.0.0.3:9200"); } + #[test] + fn test_resolve_master_addr_hostless_url_from_storage_options() { + let url = Url::parse("goosefs:///data/foo.lance").unwrap(); + let storage_options = StorageOptions(HashMap::from([( + "goosefs_master_addr".to_string(), + "10.0.0.2:9200".to_string(), + )])); + let addr = GooseFsStoreProvider::resolve_master_addr(&url, &storage_options).unwrap(); + assert_eq!(addr, "10.0.0.2:9200"); + } + + /// `goosefs:///path` must not be rejected in Lance: OpenDAL still loads + /// masters from `goosefs-site.properties` when this returns `None`. + #[test] + #[serial] + fn test_resolve_master_addr_hostless_url_without_explicit_source() { + let _clear_env = RestoreEnv::unset("GOOSEFS_MASTER_ADDR"); + let url = Url::parse("goosefs:///data/foo.lance").unwrap(); + let storage_options = StorageOptions(HashMap::new()); + assert_eq!( + GooseFsStoreProvider::resolve_master_addr(&url, &storage_options), + None + ); + } + + #[test] + #[serial] + fn test_calculate_object_store_prefix_hostless_url() { + let _clear_env = RestoreEnv::unset("GOOSEFS_MASTER_ADDR"); + let provider = GooseFsStoreProvider; + let url = Url::parse("goosefs:///data/foo.lance").unwrap(); + let prefix = provider.calculate_object_store_prefix(&url, None).unwrap(); + assert_eq!(prefix, "goosefs$"); + } + + #[test] + fn test_calculate_object_store_prefix_hostless_url_with_master_option() { + let provider = GooseFsStoreProvider; + let url = Url::parse("goosefs:///data/foo.lance").unwrap(); + let opts = HashMap::from([( + "goosefs_master_addr".to_string(), + "10.0.0.1:9200".to_string(), + )]); + let prefix = provider + .calculate_object_store_prefix(&url, Some(&opts)) + .unwrap(); + assert_eq!(prefix, "goosefs$10.0.0.1:9200"); + } + + #[tokio::test] + async fn test_new_store_hostless_url_with_storage_options() { + let provider = GooseFsStoreProvider; + let url = Url::parse("goosefs:///data/foo.lance").unwrap(); + let params = ObjectStoreParams { + storage_options_accessor: Some(Arc::new(StorageOptionsAccessor::with_static_options( + HashMap::from([( + "goosefs_master_addr".to_string(), + "10.0.0.1:9200".to_string(), + )]), + ))), + ..Default::default() + }; + let store = provider.new_store(url, ¶ms).await.unwrap(); + assert_eq!(store.scheme, "goosefs"); + } + + /// Regression: a Hadoop-style `goosefs:///path` URL plus + /// `goosefs-site.properties` must build an Operator. The previous + /// `resolve_master_addr` required a URL host and rejected this before + /// OpenDAL could read the site file. + #[tokio::test] + #[serial] + async fn test_new_store_hostless_url_uses_site_properties() { + let dir = tempfile::tempdir().unwrap(); + let site = dir.path().join("goosefs-site.properties"); + std::fs::write( + &site, + "goosefs.master.hostname=10.0.0.1\ngoosefs.master.rpc.port=9200\n", + ) + .unwrap(); + + let _config_file = RestoreEnv::set("GOOSEFS_CONFIG_FILE", &site); + let _clear_addr = RestoreEnv::unset("GOOSEFS_MASTER_ADDR"); + + let provider = GooseFsStoreProvider; + let url = Url::parse("goosefs:///data/foo.lance").unwrap(); + let store = provider + .new_store(url, &ObjectStoreParams::default()) + .await + .unwrap(); + assert_eq!(store.scheme, "goosefs"); + } + #[test] fn test_resolve_root_defaults_to_slash() { let opts = StorageOptions(HashMap::new()); From e1ecfa550b4ad35e2eb78c5fd5d20c3bfb869a27 Mon Sep 17 00:00:00 2001 From: forwardxu Date: Fri, 18 Sep 2026 17:43:07 +0800 Subject: [PATCH 2/4] fix(io): derive GooseFS cache prefix from resolved master address The registry cache prefix used the raw URL authority whenever the URL carried one, while new_store passes resolve_master_addr's result to OpenDAL. When goosefs_master_addr / GOOSEFS_MASTER_ADDR overrides the authority, two URLs sharing a placeholder authority but pointing at different masters collided on one cached Operator. Derive the prefix through the same resolve_master_addr chain for all URLs so it always tracks the master the Operator actually uses; the hostless site-properties case still collapses to the shared goosefs$ prefix. --- .../src/object_store/providers/goosefs.rs | 59 ++++++++++++++----- 1 file changed, 43 insertions(+), 16 deletions(-) diff --git a/rust/lance-io/src/object_store/providers/goosefs.rs b/rust/lance-io/src/object_store/providers/goosefs.rs index 4f2ccb43f79..43291a82125 100644 --- a/rust/lance-io/src/object_store/providers/goosefs.rs +++ b/rust/lance-io/src/object_store/providers/goosefs.rs @@ -489,11 +489,19 @@ impl ObjectStoreProvider for GooseFsStoreProvider { /// Calculate the object store prefix used as the registry cache key. /// - /// Format: `goosefs$host:port`. Because the OpenDAL root is now cluster- - /// wide (not per-URL), all datasets under the same master intentionally - /// share the same cached [`ObjectStore`]; the URL path is disambiguated - /// by [`Self::extract_path`] on each request. This is analogous to how - /// two `s3://bucket/a` and `s3://bucket/b` URLs share one store. + /// Format: `goosefs$master_addr`. The address is derived through the same + /// [`Self::resolve_master_addr`] chain that `new_store` feeds to OpenDAL + /// as `master_addr`, so the prefix always tracks the master the Operator + /// actually uses — including when `goosefs_master_addr` / + /// `GOOSEFS_MASTER_ADDR` overrides the URL authority, and for hostless + /// URLs such as `goosefs:///path`. When nothing resolves (the master + /// comes only from `goosefs-site.properties`), the prefix stays + /// `goosefs$` — that file is process-wide, so one cached Operator is + /// correct. Because the OpenDAL root is cluster-wide (not per-URL), all + /// datasets under the same master intentionally share the same cached + /// [`ObjectStore`]; the URL path is disambiguated by + /// [`Self::extract_path`] on each request. This is analogous to how two + /// `s3://bucket/a` and `s3://bucket/b` URLs share one store. fn calculate_object_store_prefix( &self, url: &Url, @@ -502,18 +510,8 @@ impl ObjectStoreProvider for GooseFsStoreProvider { // If a custom `goosefs_root` is provided, include it in the prefix so // that stores built with different roots don't accidentally collide. let opts = StorageOptions(storage_options.cloned().unwrap_or_default()); + let authority = Self::resolve_master_addr(url, &opts).unwrap_or_default(); let root = Self::resolve_root(&opts); - // `goosefs:///path` has an empty authority. Use the resolved master - // when present so two host-less URLs with different - // `goosefs_master_addr` options do not share a cache entry. When the - // master comes only from `goosefs-site.properties`, the prefix stays - // `goosefs$` — that file is process-wide, so one cached Operator is - // correct. - let authority = if url.authority().is_empty() { - Self::resolve_master_addr(url, &opts).unwrap_or_default() - } else { - url.authority().to_string() - }; if root == "/" { Ok(format!("{}${}", url.scheme(), authority)) } else { @@ -754,6 +752,35 @@ mod tests { assert_eq!(prefix, "goosefs$10.0.0.1:9200"); } + /// Regression test (review feedback): the prefix must be derived from the + /// resolved master, not the raw URL authority, even when the URL carries + /// an authority. Otherwise two URLs sharing a placeholder authority but + /// pointing at different masters via `goosefs_master_addr` would collide + /// on one cached Operator while OpenDAL connects to different masters. + #[test] + fn test_prefix_authority_url_with_master_option_override() { + let provider = GooseFsStoreProvider; + let url = Url::parse("goosefs://placeholder:9200/data/foo.lance").unwrap(); + let opts_a = HashMap::from([( + "goosefs_master_addr".to_string(), + "10.0.0.1:9200".to_string(), + )]); + let opts_b = HashMap::from([( + "goosefs_master_addr".to_string(), + "10.0.0.2:9200".to_string(), + )]); + + let pa = provider + .calculate_object_store_prefix(&url, Some(&opts_a)) + .unwrap(); + let pb = provider + .calculate_object_store_prefix(&url, Some(&opts_b)) + .unwrap(); + assert_eq!(pa, "goosefs$10.0.0.1:9200"); + assert_eq!(pb, "goosefs$10.0.0.2:9200"); + assert_ne!(pa, pb, "different masters must not share a cache entry"); + } + #[tokio::test] async fn test_new_store_hostless_url_with_storage_options() { let provider = GooseFsStoreProvider; From 5fe557c4709e18f2e43d2e2edbb68f2242a50ade Mon Sep 17 00:00:00 2001 From: forwardxu Date: Fri, 18 Sep 2026 17:53:42 +0800 Subject: [PATCH 3/4] test(io): collapse GooseFS master-addr cases into rstest tables Fold the overlapping hostless URL, prefix, and storage-option tests into two parametrized tables so the site.properties regression stays unique. --- .../src/object_store/providers/goosefs.rs | 180 ++++++------------ 1 file changed, 54 insertions(+), 126 deletions(-) diff --git a/rust/lance-io/src/object_store/providers/goosefs.rs b/rust/lance-io/src/object_store/providers/goosefs.rs index 43291a82125..bd03ac67868 100644 --- a/rust/lance-io/src/object_store/providers/goosefs.rs +++ b/rust/lance-io/src/object_store/providers/goosefs.rs @@ -523,7 +523,6 @@ impl ObjectStoreProvider for GooseFsStoreProvider { #[cfg(test)] mod tests { use super::*; - use crate::object_store::StorageOptionsAccessor; use rstest::rstest; use serial_test::serial; use std::ffi::OsString; @@ -602,22 +601,6 @@ mod tests { assert_eq!(path.to_string(), "dir/with space/f.lance"); } - #[test] - fn test_calculate_object_store_prefix_default_root() { - let provider = GooseFsStoreProvider; - let url = Url::parse("goosefs://10.0.0.1:9200/data").unwrap(); - let prefix = provider.calculate_object_store_prefix(&url, None).unwrap(); - assert_eq!(prefix, "goosefs$10.0.0.1:9200"); - } - - #[test] - fn test_calculate_object_store_prefix_with_hostname() { - let provider = GooseFsStoreProvider; - let url = Url::parse("goosefs://myhost:9200/data").unwrap(); - let prefix = provider.calculate_object_store_prefix(&url, None).unwrap(); - assert_eq!(prefix, "goosefs$myhost:9200"); - } - /// Regression test: two URLs pointing at different datasets under the /// same master must produce the *same* cache prefix so they share one /// Operator, and correctness must come from `extract_path` returning @@ -676,126 +659,71 @@ mod tests { assert_ne!(default_prefix, custom_prefix); } - #[test] - fn test_resolve_master_addr_from_url() { - let url = Url::parse("goosefs://10.0.0.1:9200/data").unwrap(); - let storage_options = StorageOptions(HashMap::new()); - let addr = GooseFsStoreProvider::resolve_master_addr(&url, &storage_options).unwrap(); - assert_eq!(addr, "10.0.0.1:9200"); - } - - #[test] - fn test_resolve_master_addr_default_port() { - let url = Url::parse("goosefs://10.0.0.1/data").unwrap(); - let storage_options = StorageOptions(HashMap::new()); - let addr = GooseFsStoreProvider::resolve_master_addr(&url, &storage_options).unwrap(); - assert_eq!(addr, "10.0.0.1:9200"); + fn master_option_map(addr: Option<&str>) -> Option> { + addr.map(|addr| HashMap::from([("goosefs_master_addr".to_string(), addr.to_string())])) } - #[test] - fn test_resolve_master_addr_from_storage_options() { - let url = Url::parse("goosefs://10.0.0.1:9200/data").unwrap(); - let storage_options = StorageOptions(HashMap::from([( - "goosefs_master_addr".to_string(), - "10.0.0.2:9200,10.0.0.3:9200".to_string(), - )])); - let addr = GooseFsStoreProvider::resolve_master_addr(&url, &storage_options).unwrap(); - assert_eq!(addr, "10.0.0.2:9200,10.0.0.3:9200"); - } - - #[test] - fn test_resolve_master_addr_hostless_url_from_storage_options() { - let url = Url::parse("goosefs:///data/foo.lance").unwrap(); - let storage_options = StorageOptions(HashMap::from([( - "goosefs_master_addr".to_string(), - "10.0.0.2:9200".to_string(), - )])); - let addr = GooseFsStoreProvider::resolve_master_addr(&url, &storage_options).unwrap(); - assert_eq!(addr, "10.0.0.2:9200"); - } - - /// `goosefs:///path` must not be rejected in Lance: OpenDAL still loads - /// masters from `goosefs-site.properties` when this returns `None`. - #[test] + #[rstest] + #[case::from_url("goosefs://10.0.0.1:9200/data", None, Some("10.0.0.1:9200"))] + #[case::default_port("goosefs://10.0.0.1/data", None, Some("10.0.0.1:9200"))] + #[case::storage_options( + "goosefs://10.0.0.1:9200/data", + Some("10.0.0.2:9200,10.0.0.3:9200"), + Some("10.0.0.2:9200,10.0.0.3:9200") + )] + #[case::hostless_with_option( + "goosefs:///data/foo.lance", + Some("10.0.0.2:9200"), + Some("10.0.0.2:9200") + )] + #[case::hostless_none("goosefs:///data/foo.lance", None, None)] #[serial] - fn test_resolve_master_addr_hostless_url_without_explicit_source() { + fn test_resolve_master_addr( + #[case] uri: &str, + #[case] master_option: Option<&str>, + #[case] expected: Option<&str>, + ) { let _clear_env = RestoreEnv::unset("GOOSEFS_MASTER_ADDR"); - let url = Url::parse("goosefs:///data/foo.lance").unwrap(); - let storage_options = StorageOptions(HashMap::new()); + let url = Url::parse(uri).unwrap(); + let storage_options = StorageOptions(master_option_map(master_option).unwrap_or_default()); assert_eq!( - GooseFsStoreProvider::resolve_master_addr(&url, &storage_options), - None + GooseFsStoreProvider::resolve_master_addr(&url, &storage_options).as_deref(), + expected ); } - #[test] + #[rstest] + #[case::from_url("goosefs://10.0.0.1:9200/data", None, "goosefs$10.0.0.1:9200")] + #[case::hostname("goosefs://myhost:9200/data", None, "goosefs$myhost:9200")] + #[case::hostless("goosefs:///data/foo.lance", None, "goosefs$")] + #[case::hostless_option( + "goosefs:///data/foo.lance", + Some("10.0.0.1:9200"), + "goosefs$10.0.0.1:9200" + )] + #[case::authority_overridden( + "goosefs://placeholder:9200/data/foo.lance", + Some("10.0.0.1:9200"), + "goosefs$10.0.0.1:9200" + )] + #[case::authority_overridden_other_master( + "goosefs://placeholder:9200/data/foo.lance", + Some("10.0.0.2:9200"), + "goosefs$10.0.0.2:9200" + )] #[serial] - fn test_calculate_object_store_prefix_hostless_url() { + fn test_calculate_object_store_prefix( + #[case] uri: &str, + #[case] master_option: Option<&str>, + #[case] expected: &str, + ) { let _clear_env = RestoreEnv::unset("GOOSEFS_MASTER_ADDR"); - let provider = GooseFsStoreProvider; - let url = Url::parse("goosefs:///data/foo.lance").unwrap(); - let prefix = provider.calculate_object_store_prefix(&url, None).unwrap(); - assert_eq!(prefix, "goosefs$"); - } - - #[test] - fn test_calculate_object_store_prefix_hostless_url_with_master_option() { - let provider = GooseFsStoreProvider; - let url = Url::parse("goosefs:///data/foo.lance").unwrap(); - let opts = HashMap::from([( - "goosefs_master_addr".to_string(), - "10.0.0.1:9200".to_string(), - )]); - let prefix = provider - .calculate_object_store_prefix(&url, Some(&opts)) + let url = Url::parse(uri).unwrap(); + let opts = master_option_map(master_option); + let prefix = GooseFsStoreProvider + .calculate_object_store_prefix(&url, opts.as_ref()) .unwrap(); - assert_eq!(prefix, "goosefs$10.0.0.1:9200"); - } - - /// Regression test (review feedback): the prefix must be derived from the - /// resolved master, not the raw URL authority, even when the URL carries - /// an authority. Otherwise two URLs sharing a placeholder authority but - /// pointing at different masters via `goosefs_master_addr` would collide - /// on one cached Operator while OpenDAL connects to different masters. - #[test] - fn test_prefix_authority_url_with_master_option_override() { - let provider = GooseFsStoreProvider; - let url = Url::parse("goosefs://placeholder:9200/data/foo.lance").unwrap(); - let opts_a = HashMap::from([( - "goosefs_master_addr".to_string(), - "10.0.0.1:9200".to_string(), - )]); - let opts_b = HashMap::from([( - "goosefs_master_addr".to_string(), - "10.0.0.2:9200".to_string(), - )]); - - let pa = provider - .calculate_object_store_prefix(&url, Some(&opts_a)) - .unwrap(); - let pb = provider - .calculate_object_store_prefix(&url, Some(&opts_b)) - .unwrap(); - assert_eq!(pa, "goosefs$10.0.0.1:9200"); - assert_eq!(pb, "goosefs$10.0.0.2:9200"); - assert_ne!(pa, pb, "different masters must not share a cache entry"); - } - - #[tokio::test] - async fn test_new_store_hostless_url_with_storage_options() { - let provider = GooseFsStoreProvider; - let url = Url::parse("goosefs:///data/foo.lance").unwrap(); - let params = ObjectStoreParams { - storage_options_accessor: Some(Arc::new(StorageOptionsAccessor::with_static_options( - HashMap::from([( - "goosefs_master_addr".to_string(), - "10.0.0.1:9200".to_string(), - )]), - ))), - ..Default::default() - }; - let store = provider.new_store(url, ¶ms).await.unwrap(); - assert_eq!(store.scheme, "goosefs"); + assert_eq!(prefix, expected); } /// Regression: a Hadoop-style `goosefs:///path` URL plus From 64cba49fb444a47ba4a6e94c29bf06c24f1d0b57 Mon Sep 17 00:00:00 2001 From: forwardxu Date: Fri, 18 Sep 2026 17:53:42 +0800 Subject: [PATCH 4/4] docs(io): drop private intra-doc link in GooseFS prefix docs calculate_object_store_prefix is public and cannot link to resolve_master_addr under -D rustdoc::private-intra-doc-links. --- .../src/object_store/providers/goosefs.rs | 25 ++++++++++--------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/rust/lance-io/src/object_store/providers/goosefs.rs b/rust/lance-io/src/object_store/providers/goosefs.rs index bd03ac67868..eaefc4afa63 100644 --- a/rust/lance-io/src/object_store/providers/goosefs.rs +++ b/rust/lance-io/src/object_store/providers/goosefs.rs @@ -490,18 +490,19 @@ impl ObjectStoreProvider for GooseFsStoreProvider { /// Calculate the object store prefix used as the registry cache key. /// /// Format: `goosefs$master_addr`. The address is derived through the same - /// [`Self::resolve_master_addr`] chain that `new_store` feeds to OpenDAL - /// as `master_addr`, so the prefix always tracks the master the Operator - /// actually uses — including when `goosefs_master_addr` / - /// `GOOSEFS_MASTER_ADDR` overrides the URL authority, and for hostless - /// URLs such as `goosefs:///path`. When nothing resolves (the master - /// comes only from `goosefs-site.properties`), the prefix stays - /// `goosefs$` — that file is process-wide, so one cached Operator is - /// correct. Because the OpenDAL root is cluster-wide (not per-URL), all - /// datasets under the same master intentionally share the same cached - /// [`ObjectStore`]; the URL path is disambiguated by - /// [`Self::extract_path`] on each request. This is analogous to how two - /// `s3://bucket/a` and `s3://bucket/b` URLs share one store. + /// chain that [`Self::new_store`] feeds to OpenDAL as `master_addr` + /// (`goosefs_master_addr`, then `GOOSEFS_MASTER_ADDR`, then the URL + /// authority), so the prefix always tracks the master the Operator + /// actually uses — including when those options override the URL + /// authority, and for hostless URLs such as `goosefs:///path`. When + /// nothing resolves (the master comes only from + /// `goosefs-site.properties`), the prefix stays `goosefs$` — that file + /// is process-wide, so one cached Operator is correct. Because the + /// OpenDAL root is cluster-wide (not per-URL), all datasets under the + /// same master intentionally share the same cached [`ObjectStore`]; the + /// URL path is disambiguated by [`Self::extract_path`] on each request. + /// This is analogous to how two `s3://bucket/a` and `s3://bucket/b` + /// URLs share one store. fn calculate_object_store_prefix( &self, url: &Url,