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: [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]
