Skip to content

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

Merged
CTTY merged 2 commits intoapache:mainfrom
rchowell:remove-configured-scheme
Apr 17, 2026
Merged

refactor(storage): remove the configured_scheme parameter from storage impls#2338
CTTY merged 2 commits intoapache:mainfrom
rchowell:remove-configured-scheme

Conversation

@rchowell
Copy link
Copy Markdown
Contributor

@rchowell rchowell commented Apr 15, 2026

Which issue does this PR close?

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

// 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.

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

Comment thread crates/storage/opendal/src/resolving.rs Outdated
Copy link
Copy Markdown
Collaborator

@CTTY CTTY left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for the contribution

// Check prefix of s3 path.
let prefix = format!("{}://{}/", configured_scheme, op_info.name());
// Use the URL scheme in the path for prefix matching. This enables
// use of S3-compatible storage backends using custom schemes (e.g., `minio://`, `r2://`).
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a good point, maybe OSS storage can use this directly so we don't even need to fallback to OpenDalStorage::OSS?

cc @plusplusjiajia

@CTTY CTTY merged commit fda82a2 into apache:main Apr 17, 2026
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove configured_scheme from OpenDalStorage::{S3, Azdls}

2 participants