Snapshot summary: user-supplied properties can occupy reserved metric keys when the metric isn't computed
- Dominant language
- Java
- Stars
- 9.2k
- Forks
- 3.5k
- Avg merge
- 2d 11h
- Merged PRs (30d)
- 132
Description
### Question / discussion
I'd like to check whether the following is intentional or an oversight, before proposing anything.
`SnapshotUpdate.set(String, String)` lets callers add arbitrary key/value pairs to a snapshot summary, and the computed metric key names (`ADDED_RECORDS_PROP = "added-records"`, `TOTAL_RECORDS_PROP = "total-records"`, etc.) are `public static final`. In `SnapshotSummary.Builder.build()`:
```java
// copy custom summary properties
builder.putAll(properties);
metrics.addTo(builder);
```
custom properties are added first and then computed metrics overwrite them. That correctly protects a metric **when it is computed for the commit**. But `metrics.addTo` only emits the metrics that actually changed, and `SnapshotProducer.updateTotal` only writes a `total-*` when the previous summary had it. So a user-supplied value under a reserved metric key that is **not** computed/updated this commit (e.g. `set("added-delete-files", "5")` on a commit that adds no delete files) survives into the final summary.
There is no reserved-key validation in `Builder.set(...)` and no javadoc warning that these keys are writer-owned.
### Is this intended?
If `set(...)` is meant to allow only non-reserved custom keys, it seems like it could either reject/drop keys colliding with the reserved metric names, or document them as off-limits. If instead occupying these keys is considered acceptable, that's fine — I mostly want to confirm the intended contract.
### Context
We hit the analogous behavior in apache/iceberg-rust and fixed the "computed metric gets overwritten / panics" part there (the part that already matched Java's overwrite-ordering intent). The residual reserved-key case described above is tracked on the Rust side in apache/iceberg-rust#2744; I'm raising it here since the Java implementation has the same characteristic and it seemed worth surfacing upstream.
Contributor guide
Research direction
Start by tracing SnapshotUpdate.set(String, String), SnapshotSummary.Builder.build(), and SnapshotProducer.updateTotal(). Confirm how custom properties and unchanged metrics flow into the final summary, then review the existing contract around reserved metric keys. Done requires a maintainer-approved decision on whether collisions are allowed, rejected, or documented, with corresponding coverage or documentation.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- data-engineering
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Needs clarification
- Newbie friendliness
- 35/100