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

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

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


##########
core/src/test/java/org/apache/iceberg/TestRowDelta.java:
##########
@@ -429,29 +470,31 @@ public void testValidateNoConflictsFromSnapshot() {
         statuses(Status.ADDED));
   }
 
-  @Test
+  // @Test

Review Comment:
   Any reason this test is commented out? 



##########
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 validateTableDeleteFilesWithRef(Table tbl, String ref, DeleteFile... expectedFiles) {

Review Comment:
   validateBranchDeleteFiles?



##########
core/src/test/java/org/apache/iceberg/TestRowDelta.java:
##########
@@ -1270,11 +1332,16 @@ public void testConcurrentNonConflictingRowDeltaAndRewriteFilesWithSequenceNumbe
                 baseSnapshot.sequenceNumber())
             .validateFromSnapshot(baseSnapshot.snapshotId());
 
-    rowDelta.commit();
-    rewriteFiles.commit();
+    commit(table, rowDelta, branch);
+    commit(table, rewriteFiles, branch);
 
-    validateTableDeleteFiles(table, deleteFile1);
-    validateTableFiles(table, dataFile2);
+    if (branch == "testBranch") {

Review Comment:
   equals() instead of ==



##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -267,7 +267,26 @@ private Map<String, String> summary(TableMetadata previous) {
     }
 
     Map<String, String> previousSummary;
-    if (previous.currentSnapshot() != null) {
+    if (!targetBranch.equals(SnapshotRef.MAIN_BRANCH)) {
+      if (previous.ref(targetBranch) != null) {
+        if (previous.snapshot(previous.ref(targetBranch).snapshotId()).summary() != null) {
+          previousSummary = previous.snapshot(previous.ref(targetBranch).snapshotId()).summary();
+        } else {
+          previousSummary = ImmutableMap.of();
+        }

Review Comment:
   Could we move the value of previous.ref(targetBranch) to it's own variable for readability?



##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -267,7 +267,26 @@ private Map<String, String> summary(TableMetadata previous) {
     }
 
     Map<String, String> previousSummary;
-    if (previous.currentSnapshot() != null) {
+    if (!targetBranch.equals(SnapshotRef.MAIN_BRANCH)) {
+      if (previous.ref(targetBranch) != null) {
+        if (previous.snapshot(previous.ref(targetBranch).snapshotId()).summary() != null) {
+          previousSummary = previous.snapshot(previous.ref(targetBranch).snapshotId()).summary();
+        } else {
+          previousSummary = ImmutableMap.of();
+        }

Review Comment:
   Also I think we could probably simplify all the if/else logic here. the branch will either be main or not, and in both cases it's the same logic. Regardless of main or not main: There is either not a prev snapshot in which case default to 0, or the prev snapshot had no summary use an empty summary. or the prev snapshot had a summary so use it.



##########
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 validateTableFilesWithRef(Table tbl, String ref, DataFile... expectedFiles) {

Review Comment:
   validateBranchDataFiles?



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