dannycjones commented on code in PR #2726:
URL: https://github.com/apache/iceberg-rust/pull/2726#discussion_r3490247122
##########
crates/iceberg/src/transaction/snapshot.rs:
##########
@@ -404,8 +404,18 @@ impl<'a> SnapshotProducer<'a> {
let previous_snapshot = table_metadata.current_snapshot();
- let mut additional_properties = summary_collector.build();
- additional_properties.extend(self.snapshot_properties.clone());
+ // User-supplied snapshot properties are applied first, then the
computed
+ // metrics overwrite any colliding keys. This matches iceberg-java
+ // (`SnapshotProducer.summary`), where computed `added-*`/`total-*`
values
+ // are written after user properties so a user cannot shadow them with
a
+ // bad (or merely wrong) value that would corrupt the snapshot summary.
+ // User-supplied snapshot properties are applied first, then the
computed
+ // metrics overwrite any colliding keys. This matches iceberg-java
+ // (`SnapshotProducer.summary`), where computed `added-*`/`total-*`
values
+ // are written after user properties so a user cannot shadow them with
a
+ // bad (or merely wrong) value that would corrupt the snapshot summary.
+ let mut additional_properties = self.snapshot_properties.clone();
+ additional_properties.extend(summary_collector.build());
Review Comment:
The comment here is duplicated, maybe a bad copy/paste or rebase.
##########
crates/iceberg/src/spec/snapshot_summary.rs:
##########
@@ -506,22 +506,31 @@ fn update_totals(
},
};
- let added = summary
- .additional_properties
- .get(added_property)
- .map_or(0, |value| {
- value
- .parse::<u64>()
- .expect("must be parsable as it was just serialized")
- });
- let removed = summary
- .additional_properties
- .get(removed_property)
- .map_or(0, |value| {
- value
- .parse::<u64>()
- .expect("must be parsable as it was just serialized")
- });
+ // Parse the added/removed deltas, tolerating an unparsable value by
skipping
+ // the total entirely rather than panicking. Computed metrics always
overwrite
+ // user-supplied summary properties (see `SnapshotProducer::summary`), so
a bad
+ // value should only ever come from a previous snapshot's summary; matching
+ // iceberg-java's `updateTotal`, we ignore it instead of failing the
commit.
+ let parse_delta = |property: &str| -> Option<u64> {
+ match summary.additional_properties.get(property) {
+ None => Some(0),
+ Some(value) => match value.parse::<u64>() {
+ Ok(v) => Some(v),
+ Err(parse_err) => {
+ tracing::warn!(
+ "Property '{property}' could not be parsed when
computing '{total_property}': {parse_err}. \
+ Skipping total computation.",
+ );
+ None
+ }
+ },
+ }
+ };
+
+ let (Some(added), Some(removed)) = (parse_delta(added_property),
parse_delta(removed_property))
+ else {
+ return;
+ };
Review Comment:
My bad, I introduced this bug. I wasn't aware that users can overwrite the
computed deltas.
The original intent was that at this point in the code, we _should_ be the
ones who wrote these values, hence the `expect` rather than parsing and error
handling.
I think we're still open to bad values being passed in so avoiding `expect`
sounds reasonable, although I wonder if we should be just avoiding the
possibility of passing values matching totals/added/removed all together.
Related: one of the things I wanted to do is move total updates into the
same part of the code as delta calculations - then we wouldn't need deserialize
values we just wrote in the first place. Not for this PR though!
##########
crates/iceberg/src/transaction/snapshot.rs:
##########
@@ -404,8 +404,18 @@ impl<'a> SnapshotProducer<'a> {
let previous_snapshot = table_metadata.current_snapshot();
- let mut additional_properties = summary_collector.build();
- additional_properties.extend(self.snapshot_properties.clone());
+ // User-supplied snapshot properties are applied first, then the
computed
+ // metrics overwrite any colliding keys. This matches iceberg-java
+ // (`SnapshotProducer.summary`), where computed `added-*`/`total-*`
values
+ // are written after user properties so a user cannot shadow them with
a
+ // bad (or merely wrong) value that would corrupt the snapshot summary.
+ // User-supplied snapshot properties are applied first, then the
computed
+ // metrics overwrite any colliding keys. This matches iceberg-java
+ // (`SnapshotProducer.summary`), where computed `added-*`/`total-*`
values
+ // are written after user properties so a user cannot shadow them with
a
+ // bad (or merely wrong) value that would corrupt the snapshot summary.
+ let mut additional_properties = self.snapshot_properties.clone();
+ additional_properties.extend(summary_collector.build());
Review Comment:
It is still possible for a user to supply bad values, where the new
snapshots value would be 0 as it won't be populated. Should we try and avoid
this, for instance by dropping those values?
As a fix, I think this change is fine, but I am wondering if we need to
follow-up here.
--
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]
