[GitHub] [iceberg] amogh-jahagirdar commented on a diff in pull request #5234: Core, API: BaseRowDelta, BaseOverwrite,BaseReplacePartitions, BaseRewrite to branch Impl

[GitBox <gi...@apache.org>]

amogh-jahagirdar commented on code in PR #5234:
URL: https://github.com/apache/iceberg/pull/5234#discussion_r1071651640


##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -267,9 +267,10 @@ private Map<String, String> summary(TableMetadata previous) {
     }
 
     Map<String, String> previousSummary;
-    if (previous.currentSnapshot() != null) {
-      if (previous.currentSnapshot().summary() != null) {
-        previousSummary = previous.currentSnapshot().summary();
+    SnapshotRef previousSnapshot = previous.ref(targetBranch);

Review Comment:
   Nit: Maybe a better name instead of previousSnapshot is previousBranchHead? 



##########
core/src/test/java/org/apache/iceberg/TableTestBase.java:
##########
@@ -472,6 +484,20 @@ void validateTableDeleteFiles(Table tbl, DeleteFile... expectedFiles) {
     Assert.assertEquals("Delete files should match", expectedFilePaths, actualFilePaths);
   }
 
+  void validateBranchDeleteFiles(Table tbl, String branch, DeleteFile... expectedFiles) {
+    Set<CharSequence> expectedFilePaths = Sets.newHashSet();
+    for (DeleteFile file : expectedFiles) {
+      expectedFilePaths.add(file.path());
+    }
+    Set<CharSequence> actualFilePaths = Sets.newHashSet();
+    for (FileScanTask task : tbl.newScan().useRef(branch).planFiles()) {
+      for (DeleteFile file : task.deletes()) {
+        actualFilePaths.add(file.path());
+      }
+    }
+    Assert.assertEquals("Delete files should match", expectedFilePaths, actualFilePaths);
+  }

Review Comment:
   Same as above after this change do we still need the other validateTableDeleteFiles?



##########
core/src/test/java/org/apache/iceberg/TableTestBase.java:
##########
@@ -458,6 +458,18 @@ void validateTableFiles(Table tbl, DataFile... expectedFiles) {
     Assert.assertEquals("Files should match", expectedFilePaths, actualFilePaths);
   }
 
+  void validateBranchFiles(Table tbl, String ref, DataFile... expectedFiles) {
+    Set<CharSequence> expectedFilePaths = Sets.newHashSet();
+    for (DataFile file : expectedFiles) {
+      expectedFilePaths.add(file.path());
+    }
+    Set<CharSequence> actualFilePaths = Sets.newHashSet();
+    for (FileScanTask task : tbl.newScan().useRef(ref).planFiles()) {
+      actualFilePaths.add(task.file().path());
+    }
+    Assert.assertEquals("Files should match", expectedFilePaths, actualFilePaths);
+  }

Review Comment:
   Do we still need the other validateTableFiles after this change? I don't think we should.



-- 
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