rdblue commented on code in PR #16936:
URL: https://github.com/apache/iceberg/pull/16936#discussion_r4319390812
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileAdapters.java:
##########
@@ -577,6 +640,271 @@ void unknownSpecIdThrows() {
.hasMessageContaining("Cannot find partition spec for spec ID");
}
+ @Test
+ void dataTrackedFileAdapterFromDataFile() {
+ TrackedFile result =
TrackedFileAdapters.forDataFile(TABLE_SCHEMA).wrap(DATA_FILE);
+
+ assertThat(result.tracking()).isNull();
+ assertWrappedDataFileMatchesFileFields(result, DATA_FILE);
+ }
+
+ @Test
+ void dataTrackedFileAdapterFromExistingManifestEntry() {
+ TrackedFile result =
+ TrackedFileAdapters.forDataFile(TABLE_SCHEMA)
+ .wrap(
+ newEntry()
Review Comment:
`newEntry` returns an entry that uses a partitioned schema, but `DATA_FILE`
uses the unpartitioned spec.
This is why in other test suites, I moved to using factory methods and
constants that state whether they have content stats and/or partitioning. We
should avoid mismatches here because we want test data that looks as close to
real data as possible.
Also, does it make sense to have a `newEntry()` factory method? It looks
like it's used here and in the next method. This suite is creating mismatched
values because it is difficult for a reader to understand -- you have to go
find `newEntry` to see what it's doing -- and that saves just a few lines. I
would opt to have slightly longer tests that can be read and understood in one
place.
--
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]
