Re: [PR] Core: Optimize ExpireSnapshots by combining ancestor traversals [iceberg]

["pvary (via GitHub)" <gi...@apache.org>]

pvary commented on code in PR #15038:
URL: https://github.com/apache/iceberg/pull/15038#discussion_r2683026661


##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -296,53 +301,51 @@ private Map<String, SnapshotRef> computeRetainedRefs(Map<String, SnapshotRef> re
     return retainedRefs;
   }
 
-  private Set<Long> computeAllBranchSnapshotsToRetain(Collection<SnapshotRef> refs) {
-    Set<Long> branchSnapshotsToRetain = Sets.newHashSet();
+  private void computeAllBranchSnapshotsToRetain(
+      Collection<SnapshotRef> refs, Set<Long> idsToRetain, Set<Long> referencedSnapshotIds) {
     for (SnapshotRef ref : refs) {
       if (ref.isBranch()) {
         long expireSnapshotsOlderThan =
             ref.maxSnapshotAgeMs() != null ? now - ref.maxSnapshotAgeMs() : defaultExpireOlderThan;
         int minSnapshotsToKeep =
             ref.minSnapshotsToKeep() != null ? ref.minSnapshotsToKeep() : defaultMinNumSnapshots;
-        branchSnapshotsToRetain.addAll(
-            computeBranchSnapshotsToRetain(
-                ref.snapshotId(), expireSnapshotsOlderThan, minSnapshotsToKeep));
-      }
-    }
-
-    return branchSnapshotsToRetain;
-  }
-
-  private Set<Long> computeBranchSnapshotsToRetain(
-      long snapshot, long expireSnapshotsOlderThan, int minSnapshotsToKeep) {
-    Set<Long> idsToRetain = Sets.newHashSet();
-    for (Snapshot ancestor : SnapshotUtil.ancestorsOf(snapshot, base::snapshot)) {
-      if (idsToRetain.size() < minSnapshotsToKeep
-          || ancestor.timestampMillis() >= expireSnapshotsOlderThan) {
-        idsToRetain.add(ancestor.snapshotId());
+        computeBranchSnapshotsToRetain(
+            ref.snapshotId(),
+            expireSnapshotsOlderThan,
+            minSnapshotsToKeep,
+            idsToRetain,
+            referencedSnapshotIds);
       } else {
-        return idsToRetain;
+        referencedSnapshotIds.add(ref.snapshotId());
       }
     }
-
-    return idsToRetain;
   }
 
-  private Set<Long> unreferencedSnapshotsToRetain(Collection<SnapshotRef> refs) {
-    Set<Long> referencedSnapshots = Sets.newHashSet();
-    for (SnapshotRef ref : refs) {
-      if (ref.isBranch()) {
-        for (Snapshot snapshot : SnapshotUtil.ancestorsOf(ref.snapshotId(), base::snapshot)) {
-          referencedSnapshots.add(snapshot.snapshotId());
+  private void computeBranchSnapshotsToRetain(
+      long snapshot,
+      long expireSnapshotsOlderThan,
+      int minSnapshotsToKeep,
+      Set<Long> idsToRetain,
+      Set<Long> referencedSnapshotIds) {
+    boolean stillRetaining = true;
+
+    for (Snapshot ancestor : SnapshotUtil.ancestorsOf(snapshot, base::snapshot)) {
+      referencedSnapshotIds.add(ancestor.snapshotId());
+      if (stillRetaining) {
+        if (idsToRetain.size() < minSnapshotsToKeep
+            || ancestor.timestampMillis() >= expireSnapshotsOlderThan) {
+          idsToRetain.add(ancestor.snapshotId());
+        } else {
+          stillRetaining = false;

Review Comment:
   Why do we don't return here as it was before?



-- 
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: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org