fix(spec): Fix incorrect totals when previous is omitted or invalid by dannycjones · Pull Request #2589 · apache/iceberg-rust

dannycjones · 2026-06-04T22:00:30Z

Which issue does this PR close?

Closes Snapshot summary totals appear to be calculated incorrectly when previous totals don't exist #2576.

What changes are included in this PR?

This change prevents totals being incorrectly calculated where the total is omitted, or if any of the values associated with the calculation are not valid positive integers. Additionally, a warning trace will be emitted when invalid values are found for the previous total.

Of note: we will still calculate some of the statistics where we can, while others may be omitted.

This change also removes the table truncation metrics path which is currently unused / dead code. I do not believe that it was implemented correctly for partial overwrites, and so I've removed it for now. I think we'd be better suited letting the snapshot collector handle this behavior (which is yet to be implemented).

Are these changes tested?

New unit tests are added to cover these cases.

dannycjones · 2026-06-04T22:07:23Z

Please can you take a look, @mansi153 @CTTY?

blackmwk · 2026-06-05T09:04:54Z

+                        return;
+                    }
+                },
+                None => return,


We should return 0 here?

I don't think so. There exists a previous summary however the total is omitted, which is fine since its optional. The way I'm interpreting the spec is that any one of these metrics could be missing. https://iceberg.apache.org/spec/#metrics

When the previous snapshot summary exists but there's no previous total, I don't think we can come up with a correct total for the new snapshot. The previous snapshot could have had data files (as an example).

I believe my change aligns with the Java implementation, which specifies no total if the previous one is missing or it fails to parse the integer:

https://github.com/apache/iceberg/blob/c00669fde813cac7e9d474c1a2c38fa8e4f75a95/core/src/main/java/org/apache/iceberg/SnapshotProducer.java#L926-L956

blackmwk · 2026-06-05T09:06:31Z

-            .map_or(0, |value| value.parse::<u64>().unwrap())
-    });
+    let previous_total = match previous_summary {
+        Some(previous_summary) => {


This is actually more tedious than the original one, if we want to avoid unwrap, Result has a inspect method.

I'll see what I can do here to make it less tedious, though I think it depends on the outcome of other comment threads.

I've reworked the snapshot collector a bit, which I think has made this logic of determining the previous total clearer.

blackmwk · 2026-06-05T09:07:58Z

+    if let Some(value) = summary.additional_properties.get(added_property) {
+        match value.parse::<u64>() {
+            Ok(v) => new_total += v,
+            Err(_) => return,


This is incorret, we should either return error, or prints a warning and uses a default value.

I agree that we should not swallow the error.

Would it make sense to use expect here? We just set this value, so we should expect it to be valid.

If not expect, I would propose to return an Iceberg error. I think a warning is the wrong choice here - if we cannot read our own properties, we're doing something wrong and should bail out of the commit.

I think we should consider rethinking this part of the code. I find it weird that we are parsing the number back from the string when we already had it as an integer. Instead, I think we should store the added and removed values as integers rather than strings, and then write it out to properties.

That being said, I'd prefer not to tackle in this PR if that's OK with you.

Actually, I think it would be worth rethinking now.

My thoughts are that this logic should live in the snapshot collector and not as an afterthought. I'd propose taking the previous snapshot into the build() method on the collector, such that the summaries are final once built.

iceberg-rust/crates/iceberg/src/transaction/snapshot.rs

Lines 405 to 407 in 97711b9

let previous_snapshot = table_metadata.current_snapshot();

let mut additional_properties = summary_collector.build();

I've pushed a change which updates build(&self) to build(&self, prev_summary: Option<&Summary>).

This allows us to access the collected metrics directly rather than serializing and deserializing them.

blackmwk · 2026-06-05T09:08:05Z

-        .map(|value| value.parse::<u64>().unwrap())
-    {
-        new_total -= value;
+    if let Some(value) = summary.additional_properties.get(removed_property) {


dannycjones

Thank you for the quick review, @blackmwk! I responded in comment threads.

dannycjones · 2026-06-05T10:35:30Z

+                        return;
+                    }
+                },
+                None => return,


I don't think so. There exists a previous summary however the total is omitted, which is fine since its optional. The way I'm interpreting the spec is that any one of these metrics could be missing. https://iceberg.apache.org/spec/#metrics

When the previous snapshot summary exists but there's no previous total, I don't think we can come up with a correct total for the new snapshot. The previous snapshot could have had data files (as an example).

I believe my change aligns with the Java implementation, which specifies no total if the previous one is missing or it fails to parse the integer:

https://github.com/apache/iceberg/blob/c00669fde813cac7e9d474c1a2c38fa8e4f75a95/core/src/main/java/org/apache/iceberg/SnapshotProducer.java#L926-L956

dannycjones · 2026-06-05T10:39:47Z

+    if let Some(value) = summary.additional_properties.get(added_property) {
+        match value.parse::<u64>() {
+            Ok(v) => new_total += v,
+            Err(_) => return,


I agree that we should not swallow the error.

Would it make sense to use expect here? We just set this value, so we should expect it to be valid.

If not expect, I would propose to return an Iceberg error. I think a warning is the wrong choice here - if we cannot read our own properties, we're doing something wrong and should bail out of the commit.

… minor fixes from feedback

dannycjones · 2026-06-05T14:29:58Z

FYI I've removed the overwrite truncation. It was dead code and I don't think it implemented overwrite correctly. I think we should address truncation as part of the standard summary stats collection and not have a special path for it.

Please do let me know if you think this is the wrong approach, happy to rework it. (FYI @blackmwk)

dannycjones commented Jun 4, 2026

View reviewed changes

Comment thread crates/iceberg/src/spec/snapshot_summary.rs Outdated

fix(spec): Fix incorrect totals when previous is omitted or invalid

30e7f68

dannycjones force-pushed the fix-update-totals branch from b47076d to 30e7f68 Compare June 4, 2026 22:05

blackmwk reviewed Jun 5, 2026

View reviewed changes

dannycjones commented Jun 5, 2026

View reviewed changes

move totals calculation into SnapshotSummaryCollector::build(), other…

3011e49

… minor fixes from feedback

dannycjones force-pushed the fix-update-totals branch from 90de202 to 3011e49 Compare June 5, 2026 13:48

dannycjones requested a review from blackmwk June 5, 2026 14:39

add test for summaries where no totals changed

70e5354

dannycjones mentioned this pull request Jun 5, 2026

Unify RewriteManifestsAction with SnapshotProducer once Replace/Delete ops are supported #2556

Open

	let previous_snapshot = table_metadata.current_snapshot();

	let mut additional_properties = summary_collector.build();

Conversation

dannycjones commented Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

What changes are included in this PR?

Are these changes tested?

Uh oh!

Uh oh!

dannycjones commented Jun 4, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dannycjones left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dannycjones commented Jun 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dannycjones commented Jun 4, 2026 •

edited

Loading