wombatu-kun commented on code in PR #17145:
URL: https://github.com/apache/iceberg/pull/17145#discussion_r3559963805
##########
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:
`manifestSourceTracking()` differed from `sourceTracking()` only in
`dvSnapshotId` being null, i.e. "no DV" (Tracking.java:98) - it was introduced
in #16408 as `sourceTracking()` with `DV_SNAPSHOT_ID` explicitly cleared. None
of the three new constants reproduce that, so its call sites (141, 150, and
here) now feed `SOURCE_TRACKING_ADDED`, which carries `dvSnapshotId = 43L`.
The tests stay green because `deletedPositions()` overwrites `dvSnapshotId`
unconditionally (TrackingBuilder.java:119), but the manifest-path tests now
start from a data-file source that already has a DV, and no test in this class
reaches `TrackingBuilder.from()` with a live null-`dvSnapshotId` source any
more (the remaining null-DV sources at 167 and 181 throw in `validateSource`
first). This is the mix rdblue asked to keep apart on #16408: "this is used
only for manifests and `dvSnapshotId` is used only for data files, so we should
ensure that they are not set on the same builder" - adding that check later
would now break these tests rather than be covered by them.
Suggest keeping a fourth constant, e.g. `SOURCE_TRACKING_MANIFEST = new
TrackingStruct(EntryStatus.ADDED, 42L, 10L, 10L, null, 1000L, null, null)`, at
those three call sites.
--
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]
