gaborkaszab commented on code in PR #16867:
URL: https://github.com/apache/iceberg/pull/16867#discussion_r3458572426
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileAdapters.java:
##########
@@ -289,6 +291,268 @@ void testDVDeleteFileAdapterRejectsNullDeletionVector() {
.hasMessage("Cannot create DV delete file: no deletion vector");
}
+ @Test
+ void testManifestFileAdapterDelegation() {
+ Tracking tracking = createTracking();
+ ManifestInfo manifestInfo = createManifestInfo();
+ TrackedFileStruct file =
Review Comment:
nit: Can we hide the construction of the input TrackedFile into a separate
function? Like `dataManifestFile()` it could get `ManifestInfo`, `FileContent`,
and/or `Tracking` parameters if that matters for the test.
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileAdapters.java:
##########
@@ -289,6 +291,268 @@ void testDVDeleteFileAdapterRejectsNullDeletionVector() {
.hasMessage("Cannot create DV delete file: no deletion vector");
}
+ @Test
+ void testManifestFileAdapterDelegation() {
Review Comment:
nit: maybe parameterize the test for data + delete manifest. The
`testDeleteManifestContentMapsToDeletes` test could be dropped then
##########
core/src/main/java/org/apache/iceberg/TrackedFileAdapters.java:
##########
@@ -405,6 +418,135 @@ public DeleteFile copyWithStats(Set<Integer>
requestedColumnIds) {
}
}
+ /**
+ * Adapts a TrackedFile DATA_MANIFEST or DELETE_MANIFEST entry to the {@link
ManifestFile}
+ * interface.
+ */
+ private static class TrackedManifestFile implements ManifestFile {
+ private final TrackedFile file;
+
+ private TrackedManifestFile(TrackedFile file) {
+ Preconditions.checkArgument(
+ file.tracking().dataSequenceNumber() != null,
+ "Cannot create manifest file: no data sequence number");
+ Preconditions.checkArgument(
+ file.tracking().snapshotId() != null, "Cannot create manifest file:
no snapshot ID");
+ Preconditions.checkArgument(
Review Comment:
This also seems an internal invariant for TrackedFile. The new
TrackedFileBuilder guarantees that manifestInfo is set for manifest entries.
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileAdapters.java:
##########
@@ -289,6 +291,263 @@ void testDVDeleteFileAdapterRejectsNullDeletionVector() {
.hasMessage("Cannot create DV delete file: no deletion vector");
}
+ @Test
+ void testManifestFileAdapterDelegation() {
+ Tracking tracking = createTracking();
+ ManifestInfo manifestInfo = createManifestInfo();
+ TrackedFileStruct file =
+ new TrackedFileStruct(
+ tracking,
+ FileContent.DATA_MANIFEST,
+ WRITER_FORMAT_VERSION,
+ MANIFEST_FILE_LOCATION,
+ FileFormat.PARQUET,
+ NO_PARTITION,
+ 8L,
+ 2048L);
+ file.set(MANIFEST_INFO_ORDINAL, manifestInfo);
Review Comment:
FYI, in the meantime the builder got merged
##########
core/src/main/java/org/apache/iceberg/TrackedFileAdapters.java:
##########
@@ -405,6 +418,137 @@ public DeleteFile copyWithStats(Set<Integer>
requestedColumnIds) {
}
}
+ /**
+ * Adapts a TrackedFile DATA_MANIFEST or DELETE_MANIFEST entry to the {@link
ManifestFile}
+ * interface.
+ */
+ private static class TrackedManifestFile implements ManifestFile {
+ private final TrackedFile file;
+
+ private TrackedManifestFile(TrackedFile file) {
+ Preconditions.checkArgument(
+ file.tracking() != null, "Cannot create manifest file: no tracking");
+ Preconditions.checkArgument(
Review Comment:
Thanks for the explanation @anoopj and @stevenzwu !
--
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]
