viirya commented on code in PR #2726:
URL: https://github.com/apache/iceberg-rust/pull/2726#discussion_r3493180570
##########
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:
No worries — and thanks for the context on the original `expect`. You're
right that the intent (we just wrote these, so they should be parsable) was
sound; it only broke once user properties could reach the same keys. Tolerating
the bad value here is a cheap safety net regardless, since a stale/garbage
value can also arrive from a previous snapshot's summary.
Agreed that collapsing total computation into the same place as the delta
calculation (so we never round-trip through strings) is the cleaner long-term
shape — happy to leave that as a separate follow-up as you suggest.
##########
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:
Good catch — that was a bad copy/paste on my part. Removed the duplicated
block in 5b6a124.
##########
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:
You're right, and worth being precise about the residual gap:
`summary_collector.build()` only emits `added-*`/`removed-*` for metrics that
actually changed this commit (`set_if_positive` skips zeros), and
`update_totals` only overwrites a `total-*` when the previous summary had it.
So a user value for a metric key that *isn't* computed/updated this commit
still survives.
I checked iceberg-java for comparison: `SnapshotSummary.Builder.build()` has
no reserved-key filtering either — it relies purely on `putAll(properties)`
then `metrics.addTo(builder)` overwrite ordering, so a user property for a
metric not computed this commit survives there too. So after this PR we match
Java's behavior.
Proactively dropping any user property whose key collides with a reserved
metric name would go a step beyond Java and close the gap fully. I'd prefer to
keep that as a follow-up rather than widen this PR — agreed it's worth doing. I
can open an issue to track it.
--
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]
