Reserved snapshot summary metric keys can still be set by user properties when the metric isn't computed
- Dominant language
- Rust
- Stars
- 1.4k
- Forks
- 567
- Avg merge
- 2d 2h
- Merged PRs (30d)
- 93
Description
### Background
Follow-up to #2725 / #2726 (cc @dannycjones).
#2726 fixed the main problem: user-supplied `snapshot_properties` could overwrite computed summary metrics (e.g. `added-data-files`), and a non-integer value could panic total computation. After that PR, computed metrics now overwrite user properties on a key collision, matching iceberg-java's `SnapshotProducer`/`SnapshotSummary`.
### The residual gap
The overwrite is only effective for metric keys that are *actually computed for the commit in question*. A user property whose key collides with a reserved metric name **but isn't computed/updated this commit** still survives into the snapshot summary:
- In `SnapshotProducer::summary` (`transaction/snapshot.rs`), `summary_collector.build()` only emits `added-*` / `removed-*` for metrics that changed this commit — `set_if_positive` skips zero-valued deltas. So if a commit adds no delete files, `added-delete-files` is not emitted, and a user-supplied `added-delete-files=whatever` is not overwritten.
- In `update_totals` (`spec/snapshot_summary.rs`), a `total-*` is only recomputed/overwritten when the previous snapshot's summary already had that total. Otherwise a user-supplied `total-*` survives.
Net effect: a user can still place an arbitrary value under a reserved metric key as long as that metric happens not to be computed this commit. It won't panic (that's fixed), but it pollutes the summary with a value we didn't compute.
### Relationship to iceberg-java
Worth being explicit, because it bounds what "fully fixing" this means: **iceberg-java has the same residual behavior.** `SnapshotSummary.Builder.build()` does:
```java
builder.putAll(properties); // user/custom properties first
metrics.addTo(builder); // computed metrics overwrite, but only the ones present
```
There is no reserved-key validation or blocklist in `set(String, String)` or `build()` — it relies purely on overwrite ordering. So in Java too, a user property for a metric that isn't computed this commit survives. After #2726, iceberg-rust matches Java here.
Therefore this issue is about going **one step beyond Java**: proactively rejecting or dropping any user-supplied snapshot property whose key collides with a reserved/computed metric name, so the summary can never carry a user-provided value under a reserved key regardless of whether that metric was computed.
### Possible directions
1. **Reserved-key filter.** Define the set of reserved metric keys (the `added-*` / `removed-*` / `total-*` / `deleted-*` constants in `spec/snapshot_summary.rs`) and drop (or reject with an error) any user `snapshot_properties` entry using one of them, before merging. Simplest, fully closes the gap.
2. **Restructure total computation (suggested by @dannycjones in #2726).** Move the `total-*` computation into the same place as the delta calculation so we never serialize values to strings and then re-parse them in `update_totals`. This removes the string round-trip (and the parse-tolerance branch added in #2726 becomes unnecessary) and naturally keeps computed metrics authoritative. Larger refactor.
Either way, behavior should stay spec-compatible — reserved metric keys are owned by the writer, not the caller.
### Willingness to contribute
I can contribute this.
Contributor guide
Research direction
Read SnapshotProducer::summary in transaction/snapshot.rs and update_totals plus the reserved metric constants in spec/snapshot_summary.rs. Trace how user snapshot_properties are merged with computed metrics, then inspect existing snapshot-summary tests. Done means a user-supplied value cannot survive under any reserved added-, removed-, or total-* metric key, including when that metric is not computed in the commit.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- data-engineering
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 55/100