rfc: Making Storage a Trait #1885

CTTY · 2025-11-25T00:05:33Z

Which issue does this PR close?

Related to
- Make FileIO a Trait #1314
- Consider Using object_store as IO Abstraction #172

What changes are included in this PR?

Are these changes tested?

CTTY · 2025-11-25T00:09:01Z

docs/rfcs/0002_storage_trait.md

+
+    // Deletion operations
+    async fn delete(&self, path: &str) -> Result<()>;
+    async fn remove_dir_all(&self, path: &str) -> Result<()>;


Migrating comments from @Fokko

This name feels very much file-system like, while Iceberg is designed to work against object stores. How about delete_prefix?

CTTY · 2025-11-25T00:10:40Z

docs/rfcs/0002_storage_trait.md

+#[async_trait]
+pub trait Storage: Debug + Send + Sync {
+    // File existence and metadata
+    async fn exists(&self, path: &str) -> Result<bool>;


Migrating comments from @c-thiel

I know that we use these results everywhere, but I think introducing more specific error types that we can match on for storage operations makes sense. They can implement Into of course.
For example, a RateLimited error that we got from the storage service should be treated differently from NotFound or CredentialsExpired.
With Lakekeeper we are currently using our own trait based IO due to many limitations in iceberg-rust, mainly due to unsupported signing mechanisms, missing refresh mechanisms, intransparent errors and missing extendability.
I would gladly switch to iceberg-rust if we get these solved.
Maybe this can serve as some inspiration: https://github.com/lakekeeper/lakekeeper/blob/b8fcf54c627d48a547ef0baf6863949b68579388/crates/io/src/error.rs#L291

To address @c-thiel 's comments, we have several approaches:

Introduce another set of errors for storage.

Extend current ErrorKind for storage errors.

Extend current ErrorKind, but with another enum, for example

pub enum IoErrorKind { FileNotFound, CredentialExpired, } pub enum ErrorKind { // Existing variants ... Io(IoErrorKind) }

CTTY · 2025-11-25T00:11:48Z

docs/rfcs/0002_storage_trait.md

+    // File object creation
+    fn new_input(&self, path: &str) -> Result<InputFile>;
+    fn new_output(&self, path: &str) -> Result<OutputFile>;
+}


Migrating comments from @c-thiel

Many object stores have a good way of running batch deletions, for example the DeleteObjects API in AWS S3. How would you feel about including a delete_batch method too?

I think we can provides a function like delete_iter.

+1, I think this should not be included in the initial cut tho. We can add the new function when stabilizing the Storage trait. I'll update the implementation plan to mention it

liurenjie1024

Thanks @CTTY for this pr, generally LGTM! One missing point is, I want the StorageBuilderRegistry to have some built in StorageBuilder registered when user creating a new catalog instance. I currenlty don't have a good solution, one approach would be to have a standalone crate, which loads built in StorageBuilders when StorageBuilderRegistry is initiated. And then we could have catalog crates to depend on it.

liurenjie1024 · 2025-11-25T11:06:42Z

docs/rfcs/0002_storage_trait.md

+#[async_trait]
+pub trait Storage: Debug + Send + Sync {
+    // File existence and metadata
+    async fn exists(&self, path: &str) -> Result<bool>;


To address @c-thiel 's comments, we have several approaches:

Introduce another set of errors for storage.

Extend current ErrorKind for storage errors.

Extend current ErrorKind, but with another enum, for example

pub enum IoErrorKind { FileNotFound, CredentialExpired, } pub enum ErrorKind { // Existing variants ... Io(IoErrorKind) }

docs/rfcs/0002_storage_trait.md

liurenjie1024 · 2025-11-25T11:07:44Z

docs/rfcs/0002_storage_trait.md

+
+    // Deletion operations
+    async fn delete(&self, path: &str) -> Result<()>;
+    async fn remove_dir_all(&self, path: &str) -> Result<()>;


liurenjie1024 · 2025-11-25T11:09:12Z

docs/rfcs/0002_storage_trait.md

+The `StorageBuilder` trait defines how storage backends are constructed:
+
+```rust
+pub trait StorageBuilder: Debug + Send + Sync {


Suggested change

pub trait StorageBuilder: Debug + Send + Sync {

pub trait StorageFactory: Debug + Send + Sync {

nit: Factory sounds a litte better.

It should also be Serializable

+1, I haven't figured out how to have both Serializable and dyn Trait yet, and I'm planning to explore more with typetag. Serializability is a huge pain and I'll add more details once I have more clarity

liurenjie1024 · 2025-11-25T11:10:25Z

docs/rfcs/0002_storage_trait.md

+    pub fn new() -> Self { /* ... */ }
+    pub fn register(&mut self, scheme: impl Into<String>, builder: Arc<dyn StorageBuilder>);
+    pub fn get_builder(&self, scheme: &str) -> Result<Arc<dyn StorageBuilder>>;
+    pub fn supported_types(&self) -> Vec<String>;


Suggested change

pub fn supported_types(&self) -> Vec<String>;

pub fn supported_types(&self) -> impl Iterator<Item=&str>>;

liurenjie1024 · 2025-11-25T11:12:29Z

docs/rfcs/0002_storage_trait.md

+use iceberg::io::FileIOBuilder;
+
+// Basic usage (same as the existing code)
+let file_io = FileIOBuilder::new("s3")


We no longer need FileIOBuilder?

I agree that FileIOBuilder seems excessive. I'm keeping FileIOBuilder here for now mainly because I'm uncertain where to keep Extensions right now mainly due to serde-related concerns

After StorageBuilder becomes serializable, we should be able to remove it.

I've explored a bit more on the serialization issue, and I realized that the existing FileIOBuilder is not serializable due to pub struct Extensions(HashMap<TypeId, Arc<dyn Any + Send + Sync>>);. And due to the unserializable nature of Any, it won't make sense to move Extensions to either Storage or StorageBuilder

We should be able to remove Extensions and the FileIOBuilder once the Storage trait is ready to use, so users can register a storage with credential loader directly. But I think it's better to address this problem later when we are stabilizing the Storage trait

liurenjie1024 · 2025-11-25T11:14:45Z

docs/rfcs/0002_storage_trait.md

+    ↓
+FileIOBuilder::build()
+    ↓
+StorageBuilderRegistry::new()


I think we should remove FileIOBuilder since then?

The StorageBuilderRegistery should be an instance in catalog instance?

Co-authored-by: Renjie Liu <liurenjie2008@gmail.com>

This reverts commit e7d67d8.

liurenjie1024 · 2025-12-02T10:10:36Z

docs/rfcs/0002_storage_trait.md

+
+```rust
+#[derive(Debug, Clone)]
+pub struct StorageBuilderRegistry {


I'm thinking maybe we also need a trait for StorageBuilderRegistry, and this trait lives in iceberg crate.

pub trait StorageBuilderRegistry { }

The motivation is that we may also need a default concrete registry implementation, which by default loads builtin builders. And the default registry implementaion should have a standalone crate, and it needs to depend on different storages.

Xuanwo

Thank you for working on this and really happy that RFC methods been adopted.

Xuanwo · 2025-12-02T13:13:33Z

docs/rfcs/0002_storage_trait.md

+        ┌───────────────┼───────────────┬───────────────┐
+        │               │               │               │
+┌───────┴────────────┐  │  ┌────────────┴──────┐  ┌────┴──────────┐
+│ crates/storage/    │  │  │ crates/storage/   │  │  Third-Party  │
+│    opendal/        │  │  │  object_store/    │  │    Crates     │
+│ ┌────────────────┐ │  │  │ ┌───────────────┐ │  │ ┌───────────┐ │
+│ │ opendal-s3     │ │  │  │ │ objstore-s3   │ │  │ │  custom   │ │
+│ │ impl Storage   │ │  │  │ │ impl Storage  │ │  │ │  storage  │ │
+│ │ impl Builder   │ │  │  │ │ impl Builder  │ │  │ │impl traits│ │
+│ └────────────────┘ │  │  │ └───────────────┘ │  │ └───────────┘ │
+│ ┌────────────────┐ │  │  │ ┌───────────────┐ │  └───────────────┘
+│ │ opendal-fs     │ │  │  │ │ objstore-gcs  │ │
+│ │ impl Storage   │ │  │  │ │ impl Storage  │ │
+│ │ impl Builder   │ │  │  │ │ impl Builder  │ │
+│ └────────────────┘ │  │  │ └───────────────┘ │
+│ ┌────────────────┐ │  │  │ ┌───────────────┐ │
+│ │ opendal-gcs    │ │  │  │ │ objstore-azure│ │
+│ │ impl Storage   │ │  │  │ │ impl Storage  │ │
+│ │ impl Builder   │ │  │  │ │ impl Builder  │ │
+│ └────────────────┘ │  │  │ └───────────────┘ │
+│ ... (oss, azure,   │  │  └───────────────────┘
+│      memory)       │  │
+└────────────────────┘  │
+```


My current feeling is that we don't need to add impls for every service, instead we add impls for every provider. One provider can provide multiple impls, and that can simplify a lot of our work.

For example, opendal can provide S3, GCS, OSS, Hf, and more, while object_store supports S3 and GCS. We can also have a fs implementation that only provides FS support. And maybe in the future, we can have hdfs which can provide hdfs-s3, hdfs-gcs and more.

This design matches the actual status better.

I think this thread on the old google design doc is related: https://docs.google.com/document/d/1-CEvRvb52vPTDLnzwJRBx5KLpej7oSlTu_rg0qKEGZ8/edit?disco=AAABrRO9Prk

This design decision definitely deserves another round of discussion. Whether we use a single, general Storage type (e.g., OpenDALStorage) or multiple scheme-specific types (OpenDALS3Storage, OpenDALOSSStorage, etc.) depends on how we define Storage. Is it essentially an Iceberg wrapper around something like object_store::ObjectStore, or is it intended to serve as a higher-level Iceberg abstraction that operates across different storage backends?

If it’s more of a thin wrapper, then having multiple scheme-specific types makes more sense. If it’s meant to be a higher-level abstraction, a unified multi-scheme implementation feels more appropriate

We can also have a fs implementation that only provides FS support

I'm feeling that having a "default" Storage will be a must, just like we are having MemoryCatalog in the iceberg crate, we will need to at least allow users to do basic fs operations in places like iceberg crate where concrete Storage implementations like ObjectStoreStorage are unavailable

Xuanwo · 2025-12-02T13:15:12Z

docs/rfcs/0002_storage_trait.md

+│  │  - pub trait Storage                          │  │
+│  │  - pub trait StorageBuilder                   │  │
+│  │  - pub struct StorageBuilderRegistry          │  │


As mentioned in the previous comment, we should only have one trait that provides similar functionality from Storage and StorageBuilderRegistry. The new storage abstraction should handle s3://path/to/file directly.

Just for my understanding: do you mean that we don't need a registry. Instead, we can just configure to use a general Storage implementation that should be able to handle multiple storage schemes like s3/gcs/...?

Xuanwo · 2025-12-02T13:15:43Z

docs/rfcs/0002_storage_trait.md

+
+    // Deletion operations
+    async fn delete(&self, path: &str) -> Result<()>;
+    async fn remove_dir_all(&self, path: &str) -> Result<()>;


Xuanwo · 2025-12-02T13:16:19Z

docs/rfcs/0002_storage_trait.md

+    // File object creation
+    fn new_input(&self, path: &str) -> Result<InputFile>;
+    fn new_output(&self, path: &str) -> Result<OutputFile>;
+}


I think we can provides a function like delete_iter.

CTTY added 2 commits November 24, 2025 16:03

rfc: Making Storage a trait

0506c1e

add license header

16bea19

CTTY commented Nov 25, 2025

View reviewed changes

CTTY changed the title ~~rfc: Making Storage a trait~~ rfc: Making Storage a Trait Nov 25, 2025

liurenjie1024 reviewed Nov 25, 2025

View reviewed changes

CTTY and others added 4 commits November 25, 2025 10:02

Update docs/rfcs/0002_storage_trait.md

e7d67d8

Co-authored-by: Renjie Liu <liurenjie2008@gmail.com>

Merge branch 'main' into ctty/storage-rfc

8253d4f

Revert "Update docs/rfcs/0002_storage_trait.md"

fe59361

This reverts commit e7d67d8.

Merge branch 'main' into ctty/storage-rfc

d3a4659

liurenjie1024 reviewed Dec 2, 2025

View reviewed changes

Xuanwo reviewed Dec 2, 2025

View reviewed changes

	pub trait StorageBuilder: Debug + Send + Sync {
	pub trait StorageFactory: Debug + Send + Sync {

	pub fn supported_types(&self) -> Vec<String>;
	pub fn supported_types(&self) -> impl Iterator<Item=&str>>;

rfc: Making Storage a Trait #1885

Are you sure you want to change the base?

rfc: Making Storage a Trait #1885

Uh oh!

Conversation

CTTY commented Nov 25, 2025

Which issue does this PR close?

What changes are included in this PR?

Are these changes tested?

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!

liurenjie1024 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

CTTY Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Xuanwo 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

CTTY Dec 2, 2025 •

edited

Loading