laskoviymishka commented on code in PR #2667:
URL: https://github.com/apache/iceberg-rust/pull/2667#discussion_r3463146654
##########
crates/iceberg/src/transaction/expire_snapshots.rs:
##########
@@ -321,6 +321,26 @@ impl TransactionAction for ExpireSnapshotsAction {
.into_iter()
.map(|ref_name| TableUpdate::RemoveSnapshotRef { ref_name })
.collect();
+
+ // Drop the statistics metadata tied to each expired snapshot,
mirroring Java
+ // `RemoveSnapshots.removeSnapshots`. This only rewrites metadata; the
puffin files those
+ // entries point at are deleted by the higher-level file-cleanup
maintenance operation.
+ for snapshot_id in &plan.ids_to_remove {
+ if metadata.statistics_for_snapshot(*snapshot_id).is_some() {
Review Comment:
The three new tests all expire exactly one snapshot via
`retain_last`/`expire_older_than_ms`. Two cases I'd like to see (follow-up is
fine): one where a snapshot is expired purely because its sole ref aged out
(extend `test_ref_aging_drops_old_tag_and_expires_its_snapshot` with
`stats_file(1)` and assert `removed_statistics == vec![1]`), and a 3-snapshot
chain where two expire carrying stats, asserting `removed_statistics == vec![1,
2]`.
The ref-aging path is the one I most want covered — it reaches this loop
through a different `plan()` branch than the retain/expire-older cases.
##########
crates/iceberg/src/transaction/expire_snapshots.rs:
##########
@@ -321,6 +321,26 @@ impl TransactionAction for ExpireSnapshotsAction {
.into_iter()
.map(|ref_name| TableUpdate::RemoveSnapshotRef { ref_name })
.collect();
+
+ // Drop the statistics metadata tied to each expired snapshot,
mirroring Java
+ // `RemoveSnapshots.removeSnapshots`. This only rewrites metadata; the
puffin files those
+ // entries point at are deleted by the higher-level file-cleanup
maintenance operation.
+ for snapshot_id in &plan.ids_to_remove {
Review Comment:
Minor, but binding `for &snapshot_id in &plan.ids_to_remove` drops the
repeated `*snapshot_id` derefs at the use sites and reads a bit cleaner. clippy
may nudge on this too.
##########
crates/iceberg/src/transaction/expire_snapshots.rs:
##########
@@ -472,6 +493,69 @@ mod tests {
base.with_metadata(Arc::new(builder.build().unwrap().metadata))
}
+ /// Like [`table_with`], but also attaches statistics and
partition-statistics files.
+ fn table_with_stats(
Review Comment:
This duplicates the snapshot/ref wiring from `table_with` verbatim. I'd
build on top of `table_with` (or share the snapshot/ref loop) and just layer
the stats on after, so the two don't silently diverge if that wiring changes
later.
##########
crates/iceberg/src/transaction/expire_snapshots.rs:
##########
@@ -321,6 +321,26 @@ impl TransactionAction for ExpireSnapshotsAction {
.into_iter()
.map(|ref_name| TableUpdate::RemoveSnapshotRef { ref_name })
.collect();
+
+ // Drop the statistics metadata tied to each expired snapshot,
mirroring Java
+ // `RemoveSnapshots.removeSnapshots`. This only rewrites metadata; the
puffin files those
+ // entries point at are deleted by the higher-level file-cleanup
maintenance operation.
+ for snapshot_id in &plan.ids_to_remove {
+ if metadata.statistics_for_snapshot(*snapshot_id).is_some() {
+ updates.push(TableUpdate::RemoveStatistics {
+ snapshot_id: *snapshot_id,
+ });
+ }
+ if metadata
+ .partition_statistics_for_snapshot(*snapshot_id)
+ .is_some()
+ {
+ updates.push(TableUpdate::RemovePartitionStatistics {
+ snapshot_id: *snapshot_id,
+ });
+ }
+ }
Review Comment:
We end up emitting the stats removals between the `RemoveSnapshotRef`
updates and the `RemoveSnapshots` push, so the sequence reads
`[RemoveSnapshotRef*, RemoveStatistics*, RemovePartitionStatistics*,
RemoveSnapshots]`. It's correct since these touch disjoint maps, but I'd find
it cleaner to read as refs → snapshots → stats, and it's a touch safer against
a strict server that validates updates in sequence.
Moving this loop to after the `RemoveSnapshots` push gets that ordering for
free — fine as a follow-up. wdyt?
--
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]
