gaborkaszab commented on code in PR #16689:
URL: https://github.com/apache/iceberg/pull/16689#discussion_r3387532464
##########
core/src/test/java/org/apache/iceberg/TestTrackingStruct.java:
##########
@@ -433,6 +429,43 @@ void testExistingToTerminalTransitions() {
assertThat(replaced.snapshotId()).isEqualTo(999L);
}
+ @Test
+ void testExistingPreservesSourceSnapshotId() {
+ Tracking source = sourceTracking();
+ Tracking existing = TrackingBuilder.from(source, 999L).build();
+ assertThat(existing.status()).isEqualTo(EntryStatus.EXISTING);
+
assertThat(existing.snapshotId()).isEqualTo(source.snapshotId()).isNotEqualTo(999L);
+ }
+
+ @Test
+ void testCarryForwardFromModifiedSourceChangesToExisting() {
+ // A MODIFIED entry from a prior commit carried forward without mutation
becomes EXISTING,
Review Comment:
> carried forward without mutation becomes EXISTING
nit: Maybe just my English, but I had to read this multiple times to
understand. This could be cleaner:
`A MODIFIED entry from a prior commit carried forward without mutation,
status becomes EXISTING,`
##########
core/src/test/java/org/apache/iceberg/TestTrackingStruct.java:
##########
@@ -433,6 +429,43 @@ void testExistingToTerminalTransitions() {
assertThat(replaced.snapshotId()).isEqualTo(999L);
}
+ @Test
+ void testExistingPreservesSourceSnapshotId() {
+ Tracking source = sourceTracking();
+ Tracking existing = TrackingBuilder.from(source, 999L).build();
+ assertThat(existing.status()).isEqualTo(EntryStatus.EXISTING);
+
assertThat(existing.snapshotId()).isEqualTo(source.snapshotId()).isNotEqualTo(999L);
+ }
+
+ @Test
+ void testCarryForwardFromModifiedSourceChangesToExisting() {
+ // A MODIFIED entry from a prior commit carried forward without mutation
becomes EXISTING,
+ // preserving the snapshot id of the original add (the modify commit lives
in dvSnapshotId).
Review Comment:
nit: I think this is already explained by `carried forward without mutation`
##########
core/src/test/java/org/apache/iceberg/TestTrackingStruct.java:
##########
@@ -433,6 +429,43 @@ void testExistingToTerminalTransitions() {
assertThat(replaced.snapshotId()).isEqualTo(999L);
}
+ @Test
+ void testExistingPreservesSourceSnapshotId() {
+ Tracking source = sourceTracking();
+ Tracking existing = TrackingBuilder.from(source, 999L).build();
+ assertThat(existing.status()).isEqualTo(EntryStatus.EXISTING);
+
assertThat(existing.snapshotId()).isEqualTo(source.snapshotId()).isNotEqualTo(999L);
+ }
+
+ @Test
+ void testCarryForwardFromModifiedSourceChangesToExisting() {
+ // A MODIFIED entry from a prior commit carried forward without mutation
becomes EXISTING,
+ // preserving the snapshot id of the original add (the modify commit lives
in dvSnapshotId).
+ Tracking modifiedSource = sourceTrackingWithStatus(EntryStatus.MODIFIED);
+ Tracking carried = TrackingBuilder.from(modifiedSource, 999L).build();
+ assertThat(carried.status()).isEqualTo(EntryStatus.EXISTING);
+
assertThat(carried.snapshotId()).isEqualTo(modifiedSource.snapshotId()).isNotEqualTo(999L);
+ // the modify-commit pointer and inherited fields are carried forward
unchanged
Review Comment:
nit: not sure what is `the modify-commit pointer`. The snapshot ID from the
modified instance? Anyway, I don't think this comment adds much, the asserts
are verbose enough to tell us that the tracking fields are carried forward from
the modified source entry.
--
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]
