feat(storage): Implement ObjectStoreStorage::S3 (Supersedes #2257) by Sruhvx-jpg · Pull Request #3165 · apache/iceberg-rust

Sruhvx-jpg · 2026-09-07T10:17:00Z

Which issue does this PR close?

What changes are included in this PR?

Implement ObjectStoreStorage::S3 backed by Apache Arrow's object_store crate. Originally drafted by @CTTY in #2257 and revived onto current main:

Hoist object_store 0.13 to workspace dependencies (aligned with DataFusion).
Support s3://, s3a://, and s3n:// URL schemes with empty bucket validation.
Implement zero-copy writes via WriteMultipart::put(bs) instead of slice copying.
Implement concurrent delete_stream using try_for_each_concurrent.
Add unit tests for URL parsing (including edge cases) and FileIO/Storage serialization roundtrips.
Wire crate workspace lints and publish flag.

Are these changes tested?

Yes, all 12 unit tests covering S3 URL parsing, empty bucket checks, store cache reuse, and FileIO/StorageFactory serialization roundtrips passing (cargo test -p iceberg-storage-object_store).

…oncurrent deletes - Hoist `object_store` 0.13 to workspace dependencies to align with DataFusion. - Support `s3n://` scheme alongside `s3://` and `s3a://` in `parse_s3_url`. - Optimize `delete_stream` with `try_for_each_concurrent` instead of sequential loop. - Add unit tests for `s3n://` URL parsing and FileIO/Storage serialization roundtrips. - Wire crate workspace lints and publish flag.

…_store

Sruhvx-jpg · 2026-09-07T11:03:43Z

Apologies for any notification noise from the extra PR earlier. Everything has been cleanly unified into this PR :)

Sruhvx-jpg · 2026-09-11T18:21:15Z

Hey everyone, got all the CI checks passing and green now!

Since this is a pretty big PR, just wanted to say that if you guys like the work, I'd really love to stick around and continue making it better—handling any feedback, tuning performance, and helping add other backends like GCS or Azure down the line.

Whenever you get some time to check it out, let me know what you think! :)

Sruhvx-jpg · 2026-09-14T12:55:22Z

cc @CTTY @kevinjqliu — CI is completely green on this.

Since this directly revives and finishes #2257, whenever you have a moment to take a look, I'd really appreciate your review on the S3 backend implementation!

laskoviymishka

Really glad to see object_store wired into the Storage framework — the factory, per-bucket cache, and path plumbing are all in good shape, and this is the right base for the follow-up backends. I'd hold it before merge though, since the whole object_store stack is going to build on top of this crate and a few of these are hard to walk back once it's published.

The one that worries me most is build_s3_store silently dropping most of S3Config. When an operator sets s3.sse.type=kms or custom, TryFrom populates the SSE fields but nothing forwards them to the builder, so we'd write data unencrypted even though encryption was explicitly required — a silent security regression versus opendal/Java. AmazonS3Builder has with_sse_kms_encryption/with_ssec_encryption for this, and for the fields object_store genuinely can't express (assume-role, disable-ec2-metadata) I'd return an error rather than drop them silently.

Things I'd like to settle in this PR before the follow-ups build on it:

Forward the SSE config, and error on the config fields object_store can't express instead of dropping them
Rework parse_s3_url to use the parsed Url fields instead of slicing the raw string (uppercase scheme + percent-encoded bucket both break today)
Make the store_cache/config variant fields private before the first publish
Add a Drop that aborts the multipart upload so a dropped writer doesn't orphan parts
Land at least a thin integration test against localstack/MinIO — nothing currently exercises a real read/write

None of it is structural — the design is right. Once those are addressed I'm happy to take another pass and approve.

laskoviymishka · 2026-09-14T18:08:49Z

+        Error::new(ErrorKind::DataInvalid, format!("Invalid URL: {path}")).with_source(e)
+    })?;
+
+    let scheme = &path[..url.scheme().len()];


This parses with Url but then slices the raw input using lengths taken from the normalized parsed fields, and the two don't always line up.

Two concrete failures: an uppercase scheme like S3://bucket/key gets sliced as &path[..2] = "S3", which falls through the match to the unsupported-scheme error and rejects a valid URL. And url.host_str() is percent-decoded, so s3://my%2Dbucket/key gives bucket_str = "my-bucket" (9 bytes) while the raw span is 11 bytes — the bucket slice at line 67 returns the wrong bytes and prefix_len is off, which can panic on a char boundary.

I'd match on url.scheme() directly (it's already lowercased) and pull bucket/relative from url.host_str() / url.path().trim_start_matches('/'), returning owned Strings instead of slicing the input — that's what the opendal sibling does. wdyt?

laskoviymishka · 2026-09-14T18:08:49Z

+}
+
+/// Build an `AmazonS3` store from iceberg's `S3Config` for a given bucket.
+pub(crate) fn build_s3_store(config: &S3Config, bucket: &str) -> Result<Arc<dyn ObjectStore>> {


This maps 7 of the 16 S3Config fields and silently drops the rest, and the SSE fields are the dangerous ones: when an operator sets s3.sse.type=kms or custom, TryFrom populates the SSE fields on S3Config but nothing here forwards them, so we write data unencrypted even though encryption was explicitly required. AmazonS3Builder exposes with_sse_kms_encryption / with_ssec_encryption, and the opendal sibling maps all three SSE types — I'd mirror that.

The assume-role fields (role_arn, external_id, role_session_name) and disable_ec2_metadata / disable_config_load are also dropped, and object_store has no builder API for those. Silently ignoring them is worse than not supporting them — a role-based config falls through to the credential chain and only fails at first I/O. I'd return a FeatureUnsupported/DataInvalid error listing the unsupported non-default fields rather than dropping them.

Fix the SSE forwarding and error on the fields we can't express, and this one's resolved.

laskoviymishka · 2026-09-14T18:08:49Z

+        config: Arc<S3Config>,
+        /// Per-bucket store cache.
+        #[serde(skip, default)]
+        store_cache: Arc<DashMap<String, Arc<dyn ObjectStore>>>,


Since this crate is publish = true, these two variant fields become part of the public API the moment it hits crates.io — public-api.txt already records both as pub. store_cache especially is pure implementation detail; exposing Arc<DashMap<...>> as a public field locks the cache structure into semver, so we couldn't later switch to Mutex<HashMap> or add a store abstraction without a breaking change.

I'd wrap the variant data in a struct with private fields and a pub fn new(config) constructor, exposing the config through a getter if callers need it. Better to lock this down before the first publish than after.

laskoviymishka · 2026-09-14T18:08:49Z

+
+/// Writer that implements `FileWrite` using `object_store` multipart upload.
+struct ObjectStoreWriter {
+    writer: Option<WriteMultipart>,


WriteMultipart doesn't complete or abort on drop, and there's no Drop impl here, so if an ObjectStoreWriter is dropped without close() — panic unwind, an early ? return, a cancelled future — the uploaded parts are orphaned in the bucket, billed indefinitely and never committed.

I'd add a Drop that best-effort aborts the inner WriteMultipart via take(). Worth flagging that no test will catch this since it only surfaces as leaked S3 state. wdyt?

laskoviymishka · 2026-09-14T18:08:49Z

+
+/// Convert an `object_store::Error` into an `iceberg::Error`.
+fn from_object_store_error(e: object_store::Error) -> Error {
+    Error::new(ErrorKind::Unexpected, "Failure in doing io operation").with_source(e)


This collapses every object_store::Error to ErrorKind::Unexpected, so a NotFound coming back from read/metadata/delete is indistinguishable from a network failure without downcasting the source. exists special-cases NotFound itself, but the others don't, and callers rely on ErrorKind::NotFound for control flow like commit-conflict detection and manifest reads.

I'd dispatch on the object_store::Error variant here — NotFound → ErrorKind::NotFound, PermissionDenied → the closest matching kind, else Unexpected — so every caller gets the right kind for free.

laskoviymishka · 2026-09-14T18:08:49Z

+    use super::*;
+
+    #[cfg(feature = "object_store-s3")]
+    fn make_s3_storage() -> ObjectStoreStorage {


The tests here all run against S3Config::default(), and AmazonS3Builder::build() doesn't validate eagerly, so the cache and roundtrip tests pass without ever touching a backend — none of write/read/reader/delete/delete_prefix/delete_stream/metadata is actually exercised. That's false confidence about exactly the paths most likely to break (multipart lifecycle, serial-vs-batch delete, range reads).

The opendal sibling has a localstack-backed test in CI. I'd add a feature/env-gated integration target covering a write+read roundtrip, a range read, and delete_prefix over 10+ objects before the follow-up backends lean on this crate. wdyt?

laskoviymishka · 2026-09-14T18:08:49Z

+futures = { workspace = true }
+iceberg = { workspace = true }
+object_store = { workspace = true }
+serde = { workspace = true }


serde = { workspace = true } inherits only features = ["rc"], but this crate uses #[derive(Serialize, Deserialize)]. It compiles in-workspace only because typetag/iceberg happen to activate serde/derive through feature unification — a downstream consumer depending on just this crate would hit use of undeclared crate serde_derive.

Since publish = true, I'd declare it explicitly: serde = { workspace = true, features = ["derive"] }.

laskoviymishka · 2026-09-14T18:08:49Z

+
+    async fn delete_prefix(&self, path: &str) -> Result<()> {
+        let (store, object_path) = self.get_store_and_path(path)?;
+        let prefix = if object_path.as_ref().ends_with('/') {


ObjectStorePath::from always strips trailing slashes, so ends_with('/') is always false and the else branch just re-appends-then-strips — this whole if/else collapses to let prefix = object_path;. It's only correct today because store.list matches on path-segment boundaries anyway.

laskoviymishka · 2026-09-14T18:08:49Z

+    async fn delete_stream(&self, paths: BoxStream<'static, String>) -> Result<()> {
+        paths
+            .map(Ok)
+            .try_for_each_concurrent(16, |path| async move {


Same batching point as delete_prefix — this issues one DeleteObject per path capped at 16 in flight, where DeleteObjects takes 1,000 per request. I'd route this through store.delete_stream(paths) too; if we keep the concurrent form, pull the 16 out into a named const.

laskoviymishka · 2026-09-14T18:08:49Z

+            .await
+            .map_err(from_object_store_error)?;
+        Ok(FileMetadata {
+            size: meta.size as u64,


ObjectMeta::size is already u64 in object_store 0.13, so this cast is a no-op that trips clippy::useless_conversion. Just size: meta.size,.

Sruhvx-jpg · 2026-09-14T18:18:12Z

@laskoviymishka thanks for follow up, perhaps it's my fault I didnt properly review the CITY's code.

Now that u have pointed out these issues I suspect there must be more of em, So I would like to take about 2 days minimum to get everything resolved-review-amended and also look for unknown hiccups. This means more research

Currently its night here so I will get to reading ur followup thoroughly tommarrow 😊

Again, thanks for the detailed follow up

CTTY and others added 2 commits September 7, 2026 14:59

Implement Storage for object-store

76009ee

This was referenced Sep 7, 2026

feat(storage): write pipeline robustness and edge-case handling for object_store backend #3166

Open

feat(storage): zero-copy writes and empty bucket validation for object_store backend #3167

Closed

feat(storage): zero-copy writes and empty bucket validation in object…

b22898d

…_store

Sruhvx-jpg force-pushed the revive-os-s3 branch from 3cb6dc2 to a476be2 Compare September 11, 2026 17:50

chore: add public-api.txt, format toml with taplo, and fix doc links

37180ce

Sruhvx-jpg force-pushed the revive-os-s3 branch from a476be2 to 37180ce Compare September 11, 2026 17:56

laskoviymishka requested changes Sep 14, 2026

View reviewed changes

Uh oh!

Conversation

Sruhvx-jpg commented Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

What changes are included in this PR?

Are these changes tested?

Uh oh!

Sruhvx-jpg commented Sep 7, 2026

Uh oh!

Sruhvx-jpg commented Sep 11, 2026

Uh oh!

Sruhvx-jpg commented Sep 14, 2026

Uh oh!

laskoviymishka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sruhvx-jpg commented Sep 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Sruhvx-jpg commented Sep 7, 2026 •

edited

Loading

Sruhvx-jpg commented Sep 14, 2026 •

edited

Loading