amogh-jahagirdar commented on code in PR #16285:
URL: https://github.com/apache/iceberg/pull/16285#discussion_r4328269255
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -421,7 +438,8 @@ void
serializationRoundTrip(RoundTripSerializer<TrackedFileStruct> serializer) t
null, // DeletionVector has its own serialization tests
null, // ManifestInfo has its own serialization tests
ByteBuffer.wrap(new byte[] {1, 2, 3}),
- ImmutableList.of(50L));
+ ImmutableList.of(50L),
+ null); // ColumnFile has its own serialization tests
Review Comment:
I think we should pass actual column files here instead of null.
`ColumnFileStruct` has its own serialization tests, but the list field is owned
by `TrackedFileStruct`, so right now nothing checks that `column_files`
survives Java/Kryo serialization. Could we add a couple of real
`ColumnFileStruct`s and compare them with the `ColumnFile.schema()` comparator?
Same idea as these two comments on the original `TrackedFileStruct` PR: [real
values instead of
nulls](https://github.com/apache/iceberg/pull/15854#discussion_r3033610033) and
[serialization tests having the same validations as
copy](https://github.com/apache/iceberg/pull/15854#discussion_r3113615068).
##########
core/src/test/java/org/apache/iceberg/TestTrackingBuilder.java:
##########
@@ -280,4 +280,31 @@ void manifestDVPositionsProduceModified() {
assertThat(modified.modifiedSnapshotId()).isEqualTo(999L);
assertThat(modified.deletedPositions()).isEqualTo(deletedBytes);
}
+
+ @Test
+ void manifestPositionsWithColumnFilesUpdated() {
Review Comment:
If I'm not mistaken, this test passes for status and snapshot ID even if
`columnFilesUpdated()` does nothing, since the positions setters already set
`MODIFIED` and `modifiedSnapshotId`. Could we add a separate test that only
calls `columnFilesUpdated()` on an existing entry and checks status, snapshot
ID and both sequence numbers? Right now only the data sequence number assertion
depends on the new method.
--
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]
