gaborkaszab commented on code in PR #17932:
URL: https://github.com/apache/iceberg/pull/17932#discussion_r3924391650
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileAdapters.java:
##########
@@ -346,6 +366,213 @@ void dvDeleteFileAdapterRejectsNullDeletionVector() {
.hasMessage("Cannot create DV delete file: no deletion vector");
}
+ @ParameterizedTest
+ @EnumSource(
+ value = FileContent.class,
+ names = {"DATA_MANIFEST", "DELETE_MANIFEST"})
+ void manifestFileAdapterDelegation(FileContent contentType) {
+ ByteBuffer keyMetadata = ByteBuffer.wrap(new byte[] {7, 8, 9});
Review Comment:
`keyMetadata` is not specific to this test, can it be a private static
member?
##########
core/src/main/java/org/apache/iceberg/TrackedFileAdapters.java:
##########
@@ -410,6 +414,122 @@ public DeleteFile copyWithStats(Set<Integer>
requestedColumnIds) {
}
}
+ /** Adapts a TrackedFile to {@link ManifestFile}. */
+ private static class TrackedManifestFile implements ManifestFile {
+ private final TrackedFile file;
+
+ private TrackedManifestFile(TrackedFile file) {
+ Tracking tracking = file.tracking();
Review Comment:
I see that in `TrackedFileAdapter` every time we reference a member in
`Tracking`, we branch based on whether `Tracking` is null or not. Maybe to
cover if it's not projected by the read?
Shouldn't we perform that branching in this class too?
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileAdapters.java:
##########
@@ -346,6 +366,213 @@ void dvDeleteFileAdapterRejectsNullDeletionVector() {
.hasMessage("Cannot create DV delete file: no deletion vector");
}
+ @ParameterizedTest
+ @EnumSource(
+ value = FileContent.class,
+ names = {"DATA_MANIFEST", "DELETE_MANIFEST"})
+ void manifestFileAdapterDelegation(FileContent contentType) {
+ ByteBuffer keyMetadata = ByteBuffer.wrap(new byte[] {7, 8, 9});
+ TrackedFile file =
+ new TrackedFileStruct(
+ MANIFEST_TRACKING,
+ contentType,
+ FORMAT_VERSION_V4,
+ MANIFEST_FILE_LOCATION,
+ FileFormat.PARQUET,
+ 0L,
+ MANIFEST_FILE_SIZE,
+ null,
+ null,
+ null,
+ null,
+ null,
+ MANIFEST_INFO,
+ keyMetadata,
+ null,
+ null);
+
+ ManifestFile manifest = TrackedFileAdapters.asManifestFile(file);
+
+ ManifestContent expectedContent =
+ contentType == FileContent.DATA_MANIFEST ? ManifestContent.DATA :
ManifestContent.DELETES;
+ assertThat(manifest.path()).isEqualTo(MANIFEST_FILE_LOCATION);
+ assertThat(manifest.length()).isEqualTo(MANIFEST_FILE_SIZE);
+ assertThat(manifest.content()).isEqualTo(expectedContent);
+ assertThat(manifest.sequenceNumber()).isEqualTo(DATA_SEQUENCE_NUMBER);
+
assertThat(manifest.minSequenceNumber()).isEqualTo(MANIFEST_INFO.minSequenceNumber());
+ assertThat(manifest.snapshotId()).isEqualTo(SNAPSHOT_ID);
+
assertThat(manifest.addedFilesCount()).isEqualTo(MANIFEST_INFO.addedFilesCount());
+
assertThat(manifest.addedRowsCount()).isEqualTo(MANIFEST_INFO.addedRowsCount());
+
assertThat(manifest.existingFilesCount()).isEqualTo(MANIFEST_INFO.existingFilesCount());
+
assertThat(manifest.existingRowsCount()).isEqualTo(MANIFEST_INFO.existingRowsCount());
+
assertThat(manifest.deletedFilesCount()).isEqualTo(MANIFEST_INFO.deletedFilesCount());
+
assertThat(manifest.deletedRowsCount()).isEqualTo(MANIFEST_INFO.deletedRowsCount());
+ assertThat(manifest.firstRowId()).isEqualTo(FIRST_ROW_ID);
+ assertThat(manifest.keyMetadata()).isEqualTo(keyMetadata);
+
assertThat(manifest.deletionVector()).isEqualTo(ByteBuffer.wrap(MANIFEST_DV));
+ assertThat(manifest.partitions()).isNull();
+ }
+
+ @Test
+ void manifestFileAdapterPartitionSpecIdUnsupported() {
+ TrackedFile file =
+ new TrackedFileStruct(
+ MANIFEST_TRACKING,
+ FileContent.DATA_MANIFEST,
+ FORMAT_VERSION_V4,
+ MANIFEST_FILE_LOCATION,
+ FileFormat.PARQUET,
+ 0L,
+ MANIFEST_FILE_SIZE,
+ null,
+ null,
+ null,
+ null,
+ null,
+ MANIFEST_INFO,
+ null,
+ null,
+ null);
+
+ ManifestFile manifest = TrackedFileAdapters.asManifestFile(file);
+
+ assertThatThrownBy(manifest::partitionSpecId)
+ .isInstanceOf(UnsupportedOperationException.class)
+ .hasMessage("v4 manifests are not bound to a single partition spec");
Review Comment:
Is spec not allowed or optional for manifests? I know it they aren't bound
to a single partition as the comment says, but in case they happen to, then is
it still not allowed to set this?
##########
api/src/main/java/org/apache/iceberg/ManifestFile.java:
##########
@@ -126,7 +126,7 @@ static Schema schema() {
/** Returns length of the manifest file. */
long length();
- /** Returns iD of the {@link PartitionSpec} used to write the manifest file.
*/
+ /** Returns ID of the {@link PartitionSpec} used to write the manifest file.
*/
Review Comment:
Not sure about the general opinion on this, but I was asked on code reviews
multiple occasions to remove all the tiny nitpicking that are unrelated to the
PR itself. I know it seems an overkill to open a separate PR to these, but
following that logic, this and the same below should be removed from this PR.
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileAdapters.java:
##########
@@ -38,9 +38,12 @@ class TestTrackedFileAdapters {
private static final String MANIFEST_LOCATION =
"s3://bucket/table/manifest.parquet";
private static final String DATA_FILE_LOCATION =
"s3://bucket/data/file.parquet";
private static final String DV_LOCATION = "s3://bucket/puffin/dv-file.bin";
+ private static final String MANIFEST_FILE_LOCATION =
"s3://bucket/table/manifest-1.parquet";
Review Comment:
Can't we use the existing `MANIFEST_LOCATION` field?
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileAdapters.java:
##########
@@ -346,6 +366,213 @@ void dvDeleteFileAdapterRejectsNullDeletionVector() {
.hasMessage("Cannot create DV delete file: no deletion vector");
}
+ @ParameterizedTest
+ @EnumSource(
+ value = FileContent.class,
+ names = {"DATA_MANIFEST", "DELETE_MANIFEST"})
+ void manifestFileAdapterDelegation(FileContent contentType) {
+ ByteBuffer keyMetadata = ByteBuffer.wrap(new byte[] {7, 8, 9});
+ TrackedFile file =
+ new TrackedFileStruct(
+ MANIFEST_TRACKING,
+ contentType,
+ FORMAT_VERSION_V4,
+ MANIFEST_FILE_LOCATION,
+ FileFormat.PARQUET,
+ 0L,
+ MANIFEST_FILE_SIZE,
+ null,
+ null,
+ null,
+ null,
+ null,
+ MANIFEST_INFO,
+ keyMetadata,
+ null,
+ null);
+
+ ManifestFile manifest = TrackedFileAdapters.asManifestFile(file);
+
+ ManifestContent expectedContent =
+ contentType == FileContent.DATA_MANIFEST ? ManifestContent.DATA :
ManifestContent.DELETES;
+ assertThat(manifest.path()).isEqualTo(MANIFEST_FILE_LOCATION);
+ assertThat(manifest.length()).isEqualTo(MANIFEST_FILE_SIZE);
+ assertThat(manifest.content()).isEqualTo(expectedContent);
+ assertThat(manifest.sequenceNumber()).isEqualTo(DATA_SEQUENCE_NUMBER);
+
assertThat(manifest.minSequenceNumber()).isEqualTo(MANIFEST_INFO.minSequenceNumber());
+ assertThat(manifest.snapshotId()).isEqualTo(SNAPSHOT_ID);
+
assertThat(manifest.addedFilesCount()).isEqualTo(MANIFEST_INFO.addedFilesCount());
+
assertThat(manifest.addedRowsCount()).isEqualTo(MANIFEST_INFO.addedRowsCount());
+
assertThat(manifest.existingFilesCount()).isEqualTo(MANIFEST_INFO.existingFilesCount());
+
assertThat(manifest.existingRowsCount()).isEqualTo(MANIFEST_INFO.existingRowsCount());
+
assertThat(manifest.deletedFilesCount()).isEqualTo(MANIFEST_INFO.deletedFilesCount());
+
assertThat(manifest.deletedRowsCount()).isEqualTo(MANIFEST_INFO.deletedRowsCount());
+ assertThat(manifest.firstRowId()).isEqualTo(FIRST_ROW_ID);
+ assertThat(manifest.keyMetadata()).isEqualTo(keyMetadata);
+
assertThat(manifest.deletionVector()).isEqualTo(ByteBuffer.wrap(MANIFEST_DV));
+ assertThat(manifest.partitions()).isNull();
+ }
+
+ @Test
+ void manifestFileAdapterPartitionSpecIdUnsupported() {
+ TrackedFile file =
+ new TrackedFileStruct(
+ MANIFEST_TRACKING,
+ FileContent.DATA_MANIFEST,
+ FORMAT_VERSION_V4,
+ MANIFEST_FILE_LOCATION,
+ FileFormat.PARQUET,
+ 0L,
+ MANIFEST_FILE_SIZE,
+ null,
+ null,
+ null,
+ null,
+ null,
+ MANIFEST_INFO,
+ null,
+ null,
+ null);
+
+ ManifestFile manifest = TrackedFileAdapters.asManifestFile(file);
+
+ assertThatThrownBy(manifest::partitionSpecId)
+ .isInstanceOf(UnsupportedOperationException.class)
+ .hasMessage("v4 manifests are not bound to a single partition spec");
+ }
+
+ @Test
+ void manifestFileAdapterCopy() {
+ ByteBuffer keyMetadata = ByteBuffer.wrap(new byte[] {7, 8, 9});
+ TrackedFile file =
+ new TrackedFileStruct(
+ MANIFEST_TRACKING,
+ FileContent.DATA_MANIFEST,
+ FORMAT_VERSION_V4,
+ MANIFEST_FILE_LOCATION,
+ FileFormat.PARQUET,
+ 0L,
+ MANIFEST_FILE_SIZE,
+ null,
+ null,
+ null,
+ null,
+ null,
+ MANIFEST_INFO,
+ keyMetadata,
+ null,
+ null);
+
+ ManifestFile original = TrackedFileAdapters.asManifestFile(file);
+ ManifestFile copy = original.copy();
+
+ assertThat(copy.path()).isEqualTo(original.path());
+ assertThat(copy.length()).isEqualTo(original.length());
+ assertThat(copy.content()).isEqualTo(original.content());
+ assertThat(copy.sequenceNumber()).isEqualTo(original.sequenceNumber());
+
assertThat(copy.minSequenceNumber()).isEqualTo(original.minSequenceNumber());
+ assertThat(copy.snapshotId()).isEqualTo(original.snapshotId());
+ assertThat(copy.addedFilesCount()).isEqualTo(original.addedFilesCount());
+ assertThat(copy.addedRowsCount()).isEqualTo(original.addedRowsCount());
+
assertThat(copy.existingFilesCount()).isEqualTo(original.existingFilesCount());
+
assertThat(copy.existingRowsCount()).isEqualTo(original.existingRowsCount());
+
assertThat(copy.deletedFilesCount()).isEqualTo(original.deletedFilesCount());
+ assertThat(copy.deletedRowsCount()).isEqualTo(original.deletedRowsCount());
+ assertThat(copy.firstRowId()).isEqualTo(original.firstRowId());
+ assertThat(copy.partitions()).isNull();
+ assertThat(copy.keyMetadata()).isEqualTo(original.keyMetadata());
+
assertThat(copy.keyMetadata().array()).isNotSameAs(original.keyMetadata().array());
+ assertThat(copy.deletionVector()).isEqualTo(original.deletionVector());
+
assertThat(copy.deletionVector().array()).isNotSameAs(original.deletionVector().array());
Review Comment:
Shouldn't we use the mocking mechanism we do for `TrackedFile` tests to
verify that the nested struct's copy part?
##########
core/src/main/java/org/apache/iceberg/TrackedFileAdapters.java:
##########
@@ -410,6 +414,122 @@ public DeleteFile copyWithStats(Set<Integer>
requestedColumnIds) {
}
}
+ /** Adapts a TrackedFile to {@link ManifestFile}. */
+ private static class TrackedManifestFile implements ManifestFile {
+ private final TrackedFile file;
+
+ private TrackedManifestFile(TrackedFile file) {
+ Tracking tracking = file.tracking();
+ Preconditions.checkArgument(
+ tracking.dataSequenceNumber() != null, "Invalid data sequence
number: null");
+ Preconditions.checkArgument(
Review Comment:
This has been unclear for me too. I'm not sure how we guard against data
corruption on the read side and how we guarantee our internal invariants like
this. On the read side we apparently assume that these invariants are
satisfied, however, I'm not sure this is safe against some malformed data files.
Anyway, even if we want to check against these constraints, I think we
should introduce those checks on the reader, not on the adapter. Reaching this
point, I guess we should just assume that `TrackedFile` is correct and
invariants are satisfied.
So probably we are fine not checking them here.
##########
core/src/main/java/org/apache/iceberg/TrackedFileAdapters.java:
##########
@@ -410,6 +414,122 @@ public DeleteFile copyWithStats(Set<Integer>
requestedColumnIds) {
}
}
+ /** Adapts a TrackedFile to {@link ManifestFile}. */
+ private static class TrackedManifestFile implements ManifestFile {
+ private final TrackedFile file;
+
+ private TrackedManifestFile(TrackedFile file) {
+ Tracking tracking = file.tracking();
+ Preconditions.checkArgument(
+ tracking.dataSequenceNumber() != null, "Invalid data sequence
number: null");
+ Preconditions.checkArgument(
+ tracking.dataSequenceNumber().equals(tracking.fileSequenceNumber()),
+ "Manifest data and file sequence numbers must be equal, got %s and
%s",
+ tracking.dataSequenceNumber(),
+ tracking.fileSequenceNumber());
+ this.file = file;
+ }
+
+ @Override
+ public String path() {
+ return file.location();
+ }
+
+ @Override
+ public long length() {
+ return file.fileSizeInBytes();
+ }
+
+ @Override
+ public int partitionSpecId() {
+ throw new UnsupportedOperationException(
+ "v4 manifests are not bound to a single partition spec");
+ }
+
+ @Override
+ public ManifestContent content() {
+ switch (file.contentType()) {
+ case DATA_MANIFEST:
+ return ManifestContent.DATA;
+ case DELETE_MANIFEST:
+ return ManifestContent.DELETES;
+ default:
+ throw new IllegalStateException(
+ "Unsupported content type for manifests: " + file.contentType());
+ }
+ }
+
+ @Override
+ public long sequenceNumber() {
+ return file.tracking().dataSequenceNumber();
+ }
+
+ @Override
+ public long minSequenceNumber() {
+ return file.manifestInfo().minSequenceNumber();
Review Comment:
Same question as for `Tracking`: Are we sure `ManifestInfo` is not null
(part of the projection)?
--
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]
