refactor(storage): remove the configured_scheme parameter from storage impls (#2338)

rchowell · web-flow · commit fda82a249461 · 2026-04-17T16:59:33.000-07:00
## Which issue does this PR close? - Closes #2245 - Related #2231 ## What changes are included in this PR? - Remove configured_scheme field from OpenDalStorage::{S3,Azdls} - Make S3 storage use the scheme in the file paths, allowing for custom S3-compatible schemes like minio:// - Use `HashMap<Scheme, Arc<OpenDalStorage>>` so aliases share a storage instance - Added new unit tests and removed some now-obsolete scheme-mismatch tests ## Break Change We are removing a struct field from public types, so this would need to be release in 0.10.0 ```rust // Before OpenDalStorageFactory::S3 { configured_scheme: "s3a".to_string(), customized_credential_load: None, } OpenDalStorageFactory::Azdls { configured_scheme: AzureStorageScheme::Abfss, } // After OpenDalStorageFactory::S3 { customized_credential_load: None } OpenDalStorageFactory::Azdls ``` ## Are these changes tested? Beyond the unit tests, I ran these integration tests. ```sh docker compose -f dev/docker-compose.yaml up -d --wait # requires unset on any AWS_ env vars cargo test -p iceberg-integration-tests cargo test -p iceberg-catalog-hms --test hms_catalog_test cargo test -p iceberg-catalog-loader cargo test -p iceberg-storage-opendal --features opendal-s3 --test file_io_s3_test ```
diff --git a/crates/catalog/glue/src/catalog.rs b/crates/catalog/glue/src/catalog.rs
@@ -203,7 +203,6 @@ impl GlueCatalog {
         // Use provided factory or default to OpenDalStorageFactory::S3
         let factory = storage_factory.unwrap_or_else(|| {
             Arc::new(OpenDalStorageFactory::S3 {
-                configured_scheme: "s3a".to_string(),
                 customized_credential_load: None,
             })
         });
diff --git a/crates/catalog/hms/tests/hms_catalog_test.rs b/crates/catalog/hms/tests/hms_catalog_test.rs
@@ -64,7 +64,6 @@ async fn get_catalog() -> HmsCatalog {
 
     // Wait for bucket to actually exist
     let file_io = FileIOBuilder::new(Arc::new(OpenDalStorageFactory::S3 {
-        configured_scheme: "s3a".to_string(),
         customized_credential_load: None,
     }))
     .with_props(props.clone())
@@ -83,7 +82,6 @@ async fn get_catalog() -> HmsCatalog {
 
     HmsCatalogBuilder::default()
         .with_storage_factory(Arc::new(OpenDalStorageFactory::S3 {
-            configured_scheme: "s3a".to_string(),
             customized_credential_load: None,
         }))
         .load("hms", props)
diff --git a/crates/catalog/loader/tests/common/mod.rs b/crates/catalog/loader/tests/common/mod.rs
@@ -233,7 +233,6 @@ async fn glue_catalog() -> GlueCatalog {
     ]);
 
     let file_io = FileIOBuilder::new(Arc::new(OpenDalStorageFactory::S3 {
-        configured_scheme: "s3a".to_string(),
         customized_credential_load: None,
     }))
     .with_props(props.clone())
@@ -285,7 +284,6 @@ async fn hms_catalog() -> HmsCatalog {
     ]);
 
     let file_io = FileIOBuilder::new(Arc::new(OpenDalStorageFactory::S3 {
-        configured_scheme: "s3a".to_string(),
         customized_credential_load: None,
     }))
     .with_props(props.clone())
@@ -302,7 +300,6 @@ async fn hms_catalog() -> HmsCatalog {
 
     HmsCatalogBuilder::default()
         .with_storage_factory(Arc::new(OpenDalStorageFactory::S3 {
-            configured_scheme: "s3a".to_string(),
             customized_credential_load: None,
         }))
         .load("hms", props)
diff --git a/crates/catalog/s3tables/src/catalog.rs b/crates/catalog/s3tables/src/catalog.rs
@@ -202,7 +202,6 @@ impl S3TablesCatalog {
         // Use provided factory or default to OpenDalStorageFactory::S3
         let factory = storage_factory.unwrap_or_else(|| {
             Arc::new(OpenDalStorageFactory::S3 {
-                configured_scheme: "s3".to_string(),
                 customized_credential_load: None,
             })
         });
diff --git a/crates/iceberg/src/catalog/mod.rs b/crates/iceberg/src/catalog/mod.rs
@@ -144,7 +144,6 @@ pub trait CatalogBuilder: Default + Debug + Send + Sync {
     ///
     /// let catalog = MyCatalogBuilder::default()
     ///     .with_storage_factory(Arc::new(OpenDalStorageFactory::S3 {
-    ///         configured_scheme: "s3a".to_string(),
     ///         customized_credential_load: None,
     ///     }))
     ///     .load("my_catalog", props)
diff --git a/crates/integration_tests/tests/common/mod.rs b/crates/integration_tests/tests/common/mod.rs
@@ -28,7 +28,6 @@ pub async fn random_ns() -> Namespace {
     let fixture = get_test_fixture();
     let rest_catalog = RestCatalogBuilder::default()
         .with_storage_factory(Arc::new(OpenDalStorageFactory::S3 {
-            configured_scheme: "s3".to_string(),
             customized_credential_load: None,
         }))
         .load("rest", fixture.catalog_config.clone())
diff --git a/crates/integration_tests/tests/conflict_commit_test.rs b/crates/integration_tests/tests/conflict_commit_test.rs
@@ -43,7 +43,6 @@ async fn test_append_data_file_conflict() {
     let fixture = get_test_fixture();
     let rest_catalog = RestCatalogBuilder::default()
         .with_storage_factory(Arc::new(OpenDalStorageFactory::S3 {
-            configured_scheme: "s3".to_string(),
             customized_credential_load: None,
         }))
         .load("rest", fixture.catalog_config.clone())
diff --git a/crates/integration_tests/tests/read_evolved_schema.rs b/crates/integration_tests/tests/read_evolved_schema.rs
@@ -34,7 +34,6 @@ async fn test_evolved_schema() {
     let fixture = get_test_fixture();
     let rest_catalog = RestCatalogBuilder::default()
         .with_storage_factory(Arc::new(OpenDalStorageFactory::S3 {
-            configured_scheme: "s3".to_string(),
             customized_credential_load: None,
         }))
         .load("rest", fixture.catalog_config.clone())
diff --git a/crates/integration_tests/tests/read_positional_deletes.rs b/crates/integration_tests/tests/read_positional_deletes.rs
@@ -30,7 +30,6 @@ async fn test_read_table_with_positional_deletes() {
     let fixture = get_test_fixture();
     let rest_catalog = RestCatalogBuilder::default()
         .with_storage_factory(Arc::new(OpenDalStorageFactory::S3 {
-            configured_scheme: "s3".to_string(),
             customized_credential_load: None,
         }))
         .load("rest", fixture.catalog_config.clone())
diff --git a/crates/storage/opendal/README.md b/crates/storage/opendal/README.md
@@ -61,7 +61,6 @@ use iceberg_storage_opendal::OpenDalStorageFactory;
 async fn main() -> iceberg::Result<()> {
     let catalog = RestCatalogBuilder::default()
         .with_storage_factory(Arc::new(OpenDalStorageFactory::S3 {
-            configured_scheme: "s3".to_string(),
             customized_credential_load: None,
         }))
         .load(
diff --git a/crates/storage/opendal/src/azdls.rs b/crates/storage/opendal/src/azdls.rs
@@ -91,10 +91,9 @@ pub(crate) fn azdls_config_parse(mut properties: HashMap<String, String>) -> Res
 pub(crate) fn azdls_create_operator<'a>(
     absolute_path: &'a str,
     config: &AzdlsConfig,
-    configured_scheme: &AzureStorageScheme,
 ) -> Result<(opendal::Operator, &'a str)> {
     let path = absolute_path.parse::<AzureStoragePath>()?;
-    match_path_with_config(&path, config, configured_scheme)?;
+    match_path_with_config(&path, config)?;
 
     let op = azdls_config_build(config, &path)?;
 
@@ -160,18 +159,7 @@ impl FromStr for AzureStorageScheme {
 }
 
 /// Validates whether the given path matches what's configured for the backend.
-pub(crate) fn match_path_with_config(
-    path: &AzureStoragePath,
-    config: &AzdlsConfig,
-    configured_scheme: &AzureStorageScheme,
-) -> Result<()> {
-    ensure_data_valid!(
-        &path.scheme == configured_scheme,
-        "Storage::Azdls: Scheme mismatch: configured {}, passed {}",
-        configured_scheme,
-        path.scheme
-    );
-
+pub(crate) fn match_path_with_config(path: &AzureStoragePath, config: &AzdlsConfig) -> Result<()> {
     if let Some(ref configured_account_name) = config.account_name {
         ensure_data_valid!(
             &path.account_name == configured_account_name,
@@ -408,7 +396,6 @@ mod tests {
                         endpoint: Some("https://myaccount.dfs.core.windows.net".to_string()),
                         ..Default::default()
                     },
-                    AzureStorageScheme::Abfss,
                 ),
                 Some(("myfs", "/path/to/file.parquet")),
             ),
@@ -421,33 +408,19 @@ mod tests {
                         endpoint: Some("https://myaccount.dfs.core.windows.net".to_string()),
                         ..Default::default()
                     },
-                    AzureStorageScheme::Abfss,
-                ),
-                None,
-            ),
-            (
-                "different scheme",
-                (
-                    "wasbs://myfs@myaccount.dfs.core.windows.net/path/to/file.parquet",
-                    AzdlsConfig {
-                        account_name: Some("myaccount".to_string()),
-                        endpoint: Some("https://myaccount.dfs.core.windows.net".to_string()),
-                        ..Default::default()
-                    },
-                    AzureStorageScheme::Abfss,
                 ),
                 None,
             ),
             (
                 "incompatible scheme for endpoint",
                 (
-                    "abfs://myfs@myaccount.dfs.core.windows.net/path/to/file.parquet",
+                    // `abfss` implies https; configured endpoint is plain http.
+                    "abfss://myfs@myaccount.dfs.core.windows.net/path/to/file.parquet",
                     AzdlsConfig {
                         account_name: Some("myaccount".to_string()),
                         endpoint: Some("http://myaccount.dfs.core.windows.net".to_string()),
                         ..Default::default()
                     },
-                    AzureStorageScheme::Abfss,
                 ),
                 None,
             ),
@@ -460,7 +433,6 @@ mod tests {
                         endpoint: Some("https://myaccount.dfs.core.chinacloudapi.cn".to_string()),
                         ..Default::default()
                     },
-                    AzureStorageScheme::Abfss,
                 ),
                 None,
             ),
@@ -474,14 +446,27 @@ mod tests {
                         endpoint: None,
                         ..Default::default()
                     },
-                    AzureStorageScheme::Abfs,
+                ),
+                Some(("myfs", "/path/to/file.parquet")),
+            ),
+            (
+                "scheme differs from a previously-configured one is accepted",
+                (
+                    // No configured scheme exists anymore; both abfss and wasbs
+                    // should be accepted by the same storage.
+                    "wasbs://myfs@myaccount.blob.core.windows.net/path/to/file.parquet",
+                    AzdlsConfig {
+                        account_name: Some("myaccount".to_string()),
+                        endpoint: Some("https://myaccount.blob.core.windows.net".to_string()),
+                        ..Default::default()
+                    },
                 ),
                 Some(("myfs", "/path/to/file.parquet")),
             ),
         ];
 
         for (name, input, expected) in test_cases {
-            let result = azdls_create_operator(input.0, &input.1, &input.2);
+            let result = azdls_create_operator(input.0, &input.1);
             match expected {
                 Some((expected_filesystem, expected_path)) => {
                     assert!(result.is_ok(), "Test case {name} failed: {result:?}");
diff --git a/crates/storage/opendal/src/lib.rs b/crates/storage/opendal/src/lib.rs
diff --git a/crates/storage/opendal/src/resolving.rs b/crates/storage/opendal/src/resolving.rs
diff --git a/crates/storage/opendal/tests/file_io_s3_test.rs b/crates/storage/opendal/tests/file_io_s3_test.rs
