nastra commented on code in PR #5984:
URL: https://github.com/apache/iceberg/pull/5984#discussion_r1269458985
##########
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"))
Review Comment:
the tests are generally difficult to understand and tests on tags and
branches are mixed together. Additionally, success and failure test cases are
both mixed together. I'd suggest to split those out to make tests shorter and
easier to understand. I have refactored `testFromSnapshotInclusiveWithRef()`
and `testFromSnapshotExclusiveWithRef()` into the below code:
```
@Test
public void fromSnapshotInclusiveWithNonExistingRef() {
Assertions.assertThatThrownBy(() ->
newScan().fromSnapshotInclusive("nonExistingRef"))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot find ref: nonExistingRef");
table.newFastAppend().appendFile(FILE_A).commit();
Assertions.assertThatThrownBy(
() ->
newScan()
.fromSnapshotInclusive(table.currentSnapshot().snapshotId())
.toSnapshot("nonExistingRef"))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot find ref: nonExistingRef");
}
@Test
public void fromSnapshotInclusiveWithTag() {
table.newFastAppend().appendFile(FILE_A).commit();
long snapshotAId = table.currentSnapshot().snapshotId();
String tagSnapshotAName = "t1";
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 scanWithToSnapshot =
newScan().fromSnapshotInclusive(tagSnapshotAName).toSnapshot(tagSnapshotBName);
Assertions.assertThat(scanWithToSnapshot.planFiles()).hasSize(3);
}
@Test
public void fromSnapshotInclusiveWithBranchShouldFail() {
table.newFastAppend().appendFile(FILE_A).commit();
long snapshotAId = table.currentSnapshot().snapshotId();
String branchName = "b1";
table.manageSnapshots().createBranch(branchName, snapshotAId).commit();
Assertions.assertThatThrownBy(() ->
newScan().fromSnapshotInclusive(branchName))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage(String.format("Ref %s is not a tag", branchName));
Assertions.assertThatThrownBy(
() ->
newScan().fromSnapshotInclusive(snapshotAId).toSnapshot(branchName))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage(String.format("Ref %s is not a tag", branchName));
}
@Test
public void fromSnapshotExclusiveWithNonExistingRef() {
Assertions.assertThatThrownBy(() ->
newScan().fromSnapshotExclusive("nonExistingRef"))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot find ref: nonExistingRef");
table.newFastAppend().appendFile(FILE_A).commit();
Assertions.assertThatThrownBy(
() ->
newScan()
.fromSnapshotExclusive(table.currentSnapshot().snapshotId())
.toSnapshot("nonExistingRef"))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot find ref: nonExistingRef");
}
@Test
public void testFromSnapshotExclusiveWithTag() {
table.newFastAppend().appendFile(FILE_A).commit();
long snapshotAId = table.currentSnapshot().snapshotId();
String tagSnapshotAName = "t1";
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().fromSnapshotExclusive(tagSnapshotAName);
Assertions.assertThat(scan.planFiles()).hasSize(4);
IncrementalAppendScan scanWithToSnapshot =
newScan().fromSnapshotExclusive(tagSnapshotAName).toSnapshot(tagSnapshotBName);
Assertions.assertThat(scanWithToSnapshot.planFiles()).hasSize(2);
}
@Test
public void fromSnapshotExclusiveWithBranchShouldFail() {
table.newFastAppend().appendFile(FILE_A).commit();
long snapshotAId = table.currentSnapshot().snapshotId();
String branchName = "b1";
table.manageSnapshots().createBranch(branchName, snapshotAId).commit();
Assertions.assertThatThrownBy(() ->
newScan().fromSnapshotExclusive(branchName))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage(String.format("Ref %s is not a tag", branchName));
Assertions.assertThatThrownBy(
() ->
newScan().fromSnapshotExclusive(snapshotAId).toSnapshot(branchName))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage(String.format("Ref %s is not a tag", branchName));
}
```
--
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]
