blackmwk commented on code in PR #3090:
URL: https://github.com/apache/iceberg-rust/pull/3090#discussion_r3885182994
##########
crates/iceberg/src/io/file_io.rs:
##########
@@ -59,13 +60,14 @@ use crate::Result;
/// .with_prop("key", "value")
/// .build();
/// ```
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, Serialize, Deserialize)]
pub struct FileIO {
/// Storage configuration containing properties
config: StorageConfig,
Review Comment:
Addressed in
[1bd015b4](https://github.com/apache/iceberg-rust/pull/3090/commits/1bd015b4cdde2cf2e1e1652eb5413ee325af44c0).
`FileIO` now documents that all `StorageConfig` properties, including
credentials, are serialized and that applications must protect the payload in
transit and at rest. The core snapshot also pins preservation of an
`s3.session-token` property.
##########
crates/iceberg/src/io/file_io.rs:
##########
@@ -59,13 +60,14 @@ use crate::Result;
/// .with_prop("key", "value")
/// .build();
/// ```
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, Serialize, Deserialize)]
Review Comment:
Addressed in
[1bd015b4](https://github.com/apache/iceberg-rust/pull/3090/commits/1bd015b4cdde2cf2e1e1652eb5413ee325af44c0).
Added a `# Serialization` section covering the skipped/rebuilt cache, full
configuration payload, typetag registration in the receiving binary, and the
requirement for third-party factories to use `#[typetag::serde]`.
##########
crates/iceberg/src/io/file_io.rs:
##########
@@ -544,4 +546,42 @@ mod tests {
assert_eq!(file_io.config().get("key1"), Some(&"value1".to_string()));
assert_eq!(file_io.config().get("key2"), Some(&"value2".to_string()));
}
+
+ #[tokio::test]
+ async fn test_file_io_serialization_roundtrip() {
+ let file_io = FileIOBuilder::new(Arc::new(MemoryStorageFactory))
+ .with_prop("key", "value")
+ .build();
+
+ // Initialize the storage cache before serializing. The cache is
process-local and should
+ // be rebuilt from the factory and configuration after deserialization.
+ file_io
+ .new_output("memory://test/file.txt")
+ .unwrap()
+ .write("test".into())
+ .await
+ .unwrap();
+
+ let serialized = serde_json::to_string(&file_io).unwrap();
+ let deserialized: FileIO = serde_json::from_str(&serialized).unwrap();
+
+ assert_eq!(deserialized.config().get("key"),
Some(&"value".to_string()));
+ assert!(!deserialized.exists("memory://test/file.txt").await.unwrap());
Review Comment:
Addressed in
[1bd015b4](https://github.com/apache/iceberg-rust/pull/3090/commits/1bd015b4cdde2cf2e1e1652eb5413ee325af44c0).
Added a LocalFS roundtrip that writes and rereads a real file, directly checks
cache reset/rebuild, verifies configuration fidelity, and pins the JSON
shape/type tag. Coverage now also includes every in-repo `StorageFactory` and
all seven OpenDAL backend variants.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
