mbutrovich commented on code in PR #3363:
URL: https://github.com/apache/iceberg-rust/pull/3363#discussion_r4208942345
##########
crates/iceberg/src/transaction/snapshot.rs:
##########
@@ -34,6 +34,16 @@ use crate::table::Table;
use crate::transaction::ActionCommit;
use crate::{Error, ErrorKind, TableRequirement, TableUpdate};
+const ICEBERG_VERSION_PROP: &str = "iceberg-version";
Review Comment:
Should `ICEBERG_VERSION_PROP` live in `spec/snapshot_summary.rs` with the
other summary keys
([`snapshot_summary.rs#L27-L50`](https://github.com/apache/iceberg-rust/blob/ab047b5e2f6e2ba6b999731653a3f7722b84e6f7/crates/iceberg/src/spec/snapshot_summary.rs#L27-L50))?
That module defines every other key the library writes into
`additional_properties`.
##########
crates/iceberg/src/transaction/append.rs:
##########
@@ -764,6 +764,34 @@ mod tests {
);
}
+ #[tokio::test]
+ async fn test_snapshot_summary_records_writer_identity() {
+ let table = make_v2_minimal_table();
+ let tx = Transaction::new(&table);
+
+ let mut snapshot_properties = HashMap::new();
+ snapshot_properties.insert("key".to_string(), "val".to_string());
+
+ let action = tx
+ .fast_append()
+ .set_snapshot_properties(snapshot_properties);
+ let mut action_commit = Arc::new(action).commit(&table).await.unwrap();
+ let updates = action_commit.take_updates();
+
+ let new_snapshot = if let TableUpdate::AddSnapshot { snapshot } =
&updates[0] {
+ snapshot
+ } else {
+ unreachable!()
+ };
+ let props = &new_snapshot.summary().additional_properties;
+ let version = env!("CARGO_PKG_VERSION");
+
+ assert_eq!(
+ props.get("iceberg-version").unwrap(),
+ &format!("Apache Iceberg Rust {version}")
+ );
Review Comment:
What should happen when a caller passes `iceberg-version` in
`set_snapshot_properties`? At the head commit the library value replaces it. I
ran a fast append with `iceberg-version = caller` in the snapshot properties,
and the summary recorded `Apache Iceberg Rust 0.10.1`. If that's the intended
behavior, could the test cover it? `key` is there to force the commit, so could
the test also check that it survives?
```suggestion
let snapshot_properties = HashMap::from([
("key".to_string(), "val".to_string()),
("iceberg-version".to_string(), "caller".to_string()),
]);
let action = tx
.fast_append()
.set_snapshot_properties(snapshot_properties);
let mut action_commit =
Arc::new(action).commit(&table).await.unwrap();
let updates = action_commit.take_updates();
let new_snapshot = if let TableUpdate::AddSnapshot { snapshot } =
&updates[0] {
snapshot
} else {
unreachable!()
};
let props = &new_snapshot.summary().additional_properties;
let version = env!("CARGO_PKG_VERSION");
assert_eq!(
props.get("iceberg-version").unwrap(),
&format!("Apache Iceberg Rust {version}")
);
assert_eq!(props.get("key").unwrap(), "val");
```
--
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]
