gaborkaszab commented on code in PR #17145:
URL: https://github.com/apache/iceberg/pull/17145#discussion_r3568875168
##########
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:
Doesn't make any difference for the sake of these tests. About
`dvSnapshotId` being only allowed for data files and not manifests: It's not
true anymore, we decided to allow `dvSnapshotId` for manifests too when setting
`deletedPositions` or `replacedPositions`.
The purpose of `manifestSourceTracking()` in these tests was to serve as the
source for `from()` calls and it should look like a real manifest `Tracking`.
This is still true and the test suite seems way cleaner this way.
Unrelated, but I frankly find these seemingly LLM-generated, extra verbose,
overly explaining comments distracting. This comment could have been "We might
loose test coverage by dropping the manifest source `Tracking` because we now
have no source that doesn't have a dv. Is that intentional, do we loose
anything here?"
--
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]
