nastra commented on code in PR #5984:
URL: https://github.com/apache/iceberg/pull/5984#discussion_r1269462822
##########
core/src/test/java/org/apache/iceberg/TestBaseIncrementalAppendScan.java:
##########
@@ -51,6 +51,131 @@ public void testFromSnapshotInclusive() {
Assert.assertEquals(3, Iterables.size(scanWithToSnapshot.planFiles()));
}
+ @Test
+ public void testFromSnapshotInclusiveWithRef() {
+ table.newFastAppend().appendFile(FILE_A).commit();
+ long snapshotAId = table.currentSnapshot().snapshotId();
+
+ String branchName = "b1";
+ String tagSnapshotAName = "t1";
+ table.manageSnapshots().createBranch(branchName, snapshotAId).commit();
+ table.manageSnapshots().createTag(tagSnapshotAName, snapshotAId).commit();
+
+ String tagSnapshotBName = "t2";
+ table.newFastAppend().appendFile(FILE_B).appendFile(FILE_B).commit();
+ long snapshotBId = table.currentSnapshot().snapshotId();
+ table.manageSnapshots().createTag(tagSnapshotBName, snapshotBId).commit();
+ table.newFastAppend().appendFile(FILE_C).appendFile(FILE_C).commit();
+ long snapshotCId = table.currentSnapshot().snapshotId();
+
+ IncrementalAppendScan scan =
newScan().fromSnapshotInclusive(tagSnapshotAName);
+ Assertions.assertThat(scan.planFiles()).hasSize(5);
+
+ IncrementalAppendScan scan3 =
+
newScan().fromSnapshotInclusive(tagSnapshotAName).toSnapshot(tagSnapshotBName);
+ Assertions.assertThat(scan3.planFiles()).hasSize(3);
+
+ Assertions.assertThatThrownBy(() ->
newScan().fromSnapshotInclusive(branchName))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage(String.format("Ref %s is not a tag", branchName));
+
+ Assertions.assertThatThrownBy(() ->
newScan().fromSnapshotInclusive("notExistTag"))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("Cannot find ref");
+ }
+
+ @Test
+ public void testFromSnapshotExclusiveWithRef() {
+ table.newFastAppend().appendFile(FILE_A).commit();
+ long snapshotAId = table.currentSnapshot().snapshotId();
+
+ String branchName = "b1";
+ String tagSnapshotAName = "t1";
+ table.manageSnapshots().createBranch(branchName, snapshotAId).commit();
+ table.manageSnapshots().createTag(tagSnapshotAName, snapshotAId).commit();
+
+ String tagSnapshotBName = "t2";
+ table.newFastAppend().appendFile(FILE_B).appendFile(FILE_B).commit();
+ long snapshotBId = table.currentSnapshot().snapshotId();
+ table.manageSnapshots().createTag(tagSnapshotBName, snapshotBId).commit();
+ table.newFastAppend().appendFile(FILE_C).appendFile(FILE_C).commit();
+ long snapshotCId = table.currentSnapshot().snapshotId();
+
+ IncrementalAppendScan scan2 =
newScan().fromSnapshotExclusive(tagSnapshotAName);
+ Assertions.assertThat(scan2.planFiles()).hasSize(4);
+
+ IncrementalAppendScan scan3 =
+
newScan().fromSnapshotExclusive(tagSnapshotAName).toSnapshot(tagSnapshotBName);
+ Assertions.assertThat(scan3.planFiles()).hasSize(2);
+
+ Assertions.assertThatThrownBy(() ->
newScan().fromSnapshotExclusive(branchName))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage(String.format("Ref %s is not a tag", branchName));
+
+ Assertions.assertThatThrownBy(() ->
newScan().fromSnapshotExclusive("notExistTag"))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("Cannot find ref");
+ }
+
+ @Test
+ public void testUseBranch() {
+ table.newFastAppend().appendFile(FILE_A).commit();
+ long snapshotAId = table.currentSnapshot().snapshotId();
+
+ String branchName = "b1";
+ String tagSnapshotAName = "t1";
+ table.manageSnapshots().createBranch(branchName, snapshotAId).commit();
+ table.manageSnapshots().createTag(tagSnapshotAName, snapshotAId).commit();
+
+ String tagName2 = "t2";
+ table.newFastAppend().appendFile(FILE_B).appendFile(FILE_B).commit();
+ long snapshotMainBId = table.currentSnapshot().snapshotId();
+ table.manageSnapshots().createTag(tagName2, snapshotMainBId).commit();
+
+ table.newFastAppend().appendFile(FILE_B).appendFile(FILE_B).commit();
+
+ table.newFastAppend().appendFile(FILE_C).toBranch(branchName).commit();
+ long snapshotBranchBId = table.snapshot(branchName).snapshotId();
+
+ table.newFastAppend().appendFile(FILE_C).toBranch(branchName).commit();
+ long snapshotBranchCId = table.snapshot(branchName).snapshotId();
+
+ IncrementalAppendScan scan =
newScan().fromSnapshotInclusive(tagSnapshotAName);
+ Assertions.assertThat(scan.planFiles()).hasSize(5);
+
+ IncrementalAppendScan scan2 =
+
newScan().fromSnapshotInclusive(tagSnapshotAName).useBranch(branchName);
+ Assertions.assertThat(scan2.planFiles()).hasSize(3);
+
+ IncrementalAppendScan scan3 =
newScan().toSnapshot(snapshotBranchBId).useBranch(branchName);
+ Assertions.assertThat(scan3.planFiles()).hasSize(2);
+
+ IncrementalAppendScan scan4 =
newScan().toSnapshot(snapshotBranchCId).useBranch(branchName);
+ Assertions.assertThat(scan4.planFiles()).hasSize(3);
+
+ IncrementalAppendScan scan5 =
+ newScan()
+ .fromSnapshotExclusive(tagSnapshotAName)
+ .toSnapshot(snapshotBranchBId)
+ .useBranch(branchName);
+ Assertions.assertThat(scan5.planFiles()).hasSize(1);
+
+ Assertions.assertThatThrownBy(
Review Comment:
it's quite difficult to follow this entire test, since it requires mental
note-taking to understand which branch/tag has how many files and such. I would
suggest:
1) maybe add a few comments to
`Assertions.assertThat(scan2.planFiles()).hasSize(3);` mentioning which files
are exactly expected here
2) Failure scenarios should be split out and tested separately (similar to
what I suggested in my previous comment
--
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]
