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():
// 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.
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.) arepublic static final. InSnapshotSummary.Builder.build():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.addToonly emits the metrics that actually changed, andSnapshotProducer.updateTotalonly writes atotal-*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.