stevenzwu commented on code in PR #17145:
URL: https://github.com/apache/iceberg/pull/17145#discussion_r3582504978
##########
core/src/test/java/org/apache/iceberg/TestTrackingBuilder.java:
##########
@@ -48,44 +55,38 @@ void testAddedWithSameCommitDvStaysAdded() {
@Test
void testExistingBuilderPreservesSourceFields() {
- Tracking source = sourceTracking();
-
- Tracking existing = TrackingBuilder.from(source, 1L).build();
+ Tracking existing = TrackingBuilder.from(SOURCE_TRACKING_ADDED,
1L).build();
assertThat(existing.status()).isEqualTo(EntryStatus.EXISTING);
- assertThat(existing.snapshotId()).isEqualTo(source.snapshotId());
-
assertThat(existing.dataSequenceNumber()).isEqualTo(source.dataSequenceNumber());
-
assertThat(existing.fileSequenceNumber()).isEqualTo(source.fileSequenceNumber());
- assertThat(existing.dvSnapshotId()).isEqualTo(source.dvSnapshotId());
- assertThat(existing.firstRowId()).isEqualTo(source.firstRowId());
+
assertThat(existing.snapshotId()).isEqualTo(SOURCE_TRACKING_ADDED.snapshotId());
Review Comment:
nit: add an assert of `isNotEqualTo(1L)` // new snapshot id
##########
core/src/test/java/org/apache/iceberg/TestTrackingBuilder.java:
##########
@@ -209,78 +211,70 @@ private static Stream<Arguments>
terminalTransitionCases() {
@MethodSource("terminalTransitionCases")
void testRejectsTransitionsFromTerminalStatus(
EntryStatus sourceStatus, Consumer<Tracking> factoryCall) {
- Tracking source = sourceTrackingWithStatus(sourceStatus);
+ Tracking source = new TrackingStruct(sourceStatus, 42L, 10L, 10L, 43L,
1000L, null, null);
+
assertThatThrownBy(() -> factoryCall.accept(source))
.isInstanceOf(IllegalStateException.class)
.hasMessage("Cannot revive non-live entry with status " +
sourceStatus);
}
@Test
void testExistingToExistingIsAllowed() {
- Tracking existingSource = sourceTrackingWithStatus(EntryStatus.EXISTING);
-
- Tracking existing = TrackingBuilder.from(existingSource, 1L).build();
+ Tracking existing = TrackingBuilder.from(SOURCE_TRACKING_EXISTING,
1L).build();
assertThat(existing.status()).isEqualTo(EntryStatus.EXISTING);
- assertThat(existing.snapshotId()).isEqualTo(existingSource.snapshotId());
+
assertThat(existing.snapshotId()).isEqualTo(SOURCE_TRACKING_EXISTING.snapshotId());
}
@Test
void testExistingToTerminalTransitions() {
- Tracking existingSource = sourceTrackingWithStatus(EntryStatus.EXISTING);
-
- Tracking deleted = TrackingBuilder.deleted(existingSource, 999L);
+ Tracking deleted = TrackingBuilder.deleted(SOURCE_TRACKING_EXISTING, 999L);
assertThat(deleted.status()).isEqualTo(EntryStatus.DELETED);
assertThat(deleted.snapshotId()).isEqualTo(999L);
- Tracking replaced = TrackingBuilder.replaced(existingSource, 999L);
+ Tracking replaced = TrackingBuilder.replaced(SOURCE_TRACKING_EXISTING,
999L);
assertThat(replaced.status()).isEqualTo(EntryStatus.REPLACED);
assertThat(replaced.snapshotId()).isEqualTo(999L);
}
@Test
void testExistingPreservesSourceSnapshotId() {
- Tracking source = sourceTracking();
- Tracking existing = TrackingBuilder.from(source, 999L).build();
+ Tracking existing = TrackingBuilder.from(SOURCE_TRACKING_ADDED,
999L).build();
assertThat(existing.status()).isEqualTo(EntryStatus.EXISTING);
-
assertThat(existing.snapshotId()).isEqualTo(source.snapshotId()).isNotEqualTo(999L);
+ assertThat(existing.snapshotId())
+ .isEqualTo(SOURCE_TRACKING_ADDED.snapshotId())
+ .isNotEqualTo(999L);
}
@Test
void testCarryForwardFromModifiedSourceChangesToExisting() {
- Tracking modifiedSource = sourceTrackingWithStatus(EntryStatus.MODIFIED);
- Tracking carried = TrackingBuilder.from(modifiedSource, 999L).build();
+ Tracking carried = TrackingBuilder.from(SOURCE_TRACKING_MODIFIED,
999L).build();
assertThat(carried.status()).isEqualTo(EntryStatus.EXISTING);
-
assertThat(carried.snapshotId()).isEqualTo(modifiedSource.snapshotId()).isNotEqualTo(999L);
-
assertThat(carried.dvSnapshotId()).isEqualTo(modifiedSource.dvSnapshotId()).isNotEqualTo(999L);
-
assertThat(carried.dataSequenceNumber()).isEqualTo(modifiedSource.dataSequenceNumber());
-
assertThat(carried.fileSequenceNumber()).isEqualTo(modifiedSource.fileSequenceNumber());
- assertThat(carried.firstRowId()).isEqualTo(modifiedSource.firstRowId());
+ assertThat(carried.snapshotId())
+ .isEqualTo(SOURCE_TRACKING_MODIFIED.snapshotId())
+ .isNotEqualTo(999L);
+ assertThat(carried.dvSnapshotId())
+ .isEqualTo(SOURCE_TRACKING_MODIFIED.dvSnapshotId())
+ .isNotEqualTo(999L);
+ assertThat(carried.dataSequenceNumber())
+ .isEqualTo(SOURCE_TRACKING_MODIFIED.dataSequenceNumber());
+ assertThat(carried.fileSequenceNumber())
+ .isEqualTo(SOURCE_TRACKING_MODIFIED.fileSequenceNumber());
+
assertThat(carried.firstRowId()).isEqualTo(SOURCE_TRACKING_MODIFIED.firstRowId());
}
@Test
void testManifestDVPositionsProduceModified() {
ByteBuffer deletedBytes = ByteBuffer.wrap(new byte[] {1, 2});
- Tracking addedSource = manifestSourceTracking();
Tracking modified =
- TrackingBuilder.from(addedSource,
999L).deletedPositions(deletedBytes).build();
+ TrackingBuilder.from(SOURCE_TRACKING_ADDED,
999L).deletedPositions(deletedBytes).build();
Review Comment:
The new code is a bit different with previous `manifestSourceTracking()`,
where the source tracking object's `dvSnapshotId` changed from `null` to `43`.
But it probably doesn't matter as Gabor pointed out as `dvSnapshotId` is
allowed for leaf manifest entry too.
--
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]
