Skip to content

feat(io): make FileIO serializable - #3090

Open
blackmwk wants to merge 4 commits into
apache:mainfrom
liurenjie1024:ir-3088
Open

feat(io): make FileIO serializable#3090
blackmwk wants to merge 4 commits into
apache:mainfrom
liurenjie1024:ir-3088

Conversation

@blackmwk

@blackmwk blackmwk commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

What changes are included in this PR?

  • Implement Serialize and Deserialize for FileIO.
  • Serialize the storage factory and configuration while excluding the process-local lazy storage cache.
  • Rebuild the storage cache lazily after deserialization.
  • Document credential handling, typetag registration, cache rebuilding, and OpenDAL custom credential-loader behavior.

Are these changes tested?

  • Added serialization roundtrip coverage for every in-repo StorageFactory: core memory/local filesystem, the OpenDAL resolving factory, and all seven OpenDAL backend variants.
  • Pinned the serialized JSON shape, configuration and credential-property preservation, and factory type tags.
  • Verified LocalFS persistence, direct cache reset/rebuild behavior, and OpenDAL custom credential-loader omission.
  • cargo test -p iceberg --lib --no-fail-fast
  • cargo test -p iceberg-storage-opendal --all-features --lib --no-fail-fast
  • cargo test -p iceberg-storage-opendal --all-features --test file_io_serialization_test --no-fail-fast
  • cargo test -p iceberg-storage-opendal --no-default-features --test file_io_serialization_test --no-fail-fast
  • cargo clippy -p iceberg --lib -- -D warnings
  • cargo clippy -p iceberg-storage-opendal --all-features --tests -- -D warnings
  • cargo fmt --all -- --check
  • make check-public-api

AI Disclosure

Codex was used to help implement the change, add the regression tests and documentation, and run verification. The resulting code, documentation, and test behavior were reviewed before submission.

Comment thread crates/iceberg/src/io/file_io.rs Outdated
@blackmwk
blackmwk marked this pull request as draft August 28, 2026 09:37
@blackmwk
blackmwk marked this pull request as ready for review August 28, 2026 09:45

@laskoviymishka laskoviymishka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice, the mechanical wiring here is clean. The #[serde(skip)] on the cache with the OnceLock rebuilding lazily is exactly right, and the roundtrip does what it says.

The one thing I'd want to settle before this merges is the credential story. Serializing a FileIO emits the whole StorageConfig.props map verbatim, so an S3-configured one writes its secret access key and session token into the output in plaintext. The stated use case is shipping these across process boundaries, which is exactly where a plaintext credential blob is most dangerous — and because these serde impls are now public API, the format is hard to change later. I'd like the serialized form to be safe by default before we commit to it.

Stepping back a little: did we consider serializing just the StorageConfig plus a factory discriminant and reconstructing through FileIOBuilder, rather than deriving on FileIO directly? That's the pattern the REST catalog already uses to rebuild FileIO from properties, and it's how Java/PyIceberg/iceberg-go all handle it — config in, live object rebuilt. It keeps runtime state out of the wire format and gives us a natural spot to redact. Not a hard blocker, but worth weighing before this becomes API.

A few smaller things I left inline: the custom credential loader gets silently dropped on roundtrip, the new serde behavior (typetag registry, ephemeral storage) needs docs, and the test only exercises MemoryStorageFactory so it doesn't really prove roundtrip fidelity.

Once the credential handling's settled, happy to take another pass and approve.

Comment thread crates/iceberg/src/io/file_io.rs
Comment thread crates/iceberg/src/io/file_io.rs
Comment thread crates/iceberg/src/io/file_io.rs
Comment thread crates/iceberg/src/io/file_io.rs Outdated

@CTTY CTTY left a comment

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.

LGTM!

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.

Make FileIO serializable.

4 participants