gaborkaszab commented on code in PR #16867:
URL: https://github.com/apache/iceberg/pull/16867#discussion_r3443349921
##########
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:
As per the spec proposal data seq num is 'Optional for leaf manifests,
required for root.'. I think it's valid to have it as null here. Seems also
optional in `ManifestFile`
##########
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");
Review Comment:
nit: other adapters don't check if tracking is null. Not sure we have to
here. It's a required field anyway
##########
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(
+ 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(
+ file.manifestInfo() != null, "Cannot create manifest file: no
manifest info");
+ Preconditions.checkArgument(
Review Comment:
This checks an internal invariant for a `TrackedFile`. As per the spec
proposal a `DELETE_MANIFEST` shouldn't have `firstRowId`, so I'd assume that
this is a contract to keep when we construct a TrackedFile and set this value.
In fact this is check in the [relevant
PR](https://github.com/apache/iceberg/pull/16849/changes)
##########
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(
+ 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(
+ file.manifestInfo() != null, "Cannot create manifest file: no
manifest info");
+ Preconditions.checkArgument(
+ file.contentType() != FileContent.DELETE_MANIFEST ||
file.tracking().firstRowId() == null,
+ "Delete manifest must not have a first row ID: %s",
+ file.tracking().firstRowId());
+ this.file = file;
+ }
+
+ @Override
+ public String path() {
+ return file.location();
+ }
+
+ @Override
+ public long length() {
+ return file.fileSizeInBytes();
+ }
+
+ @Override
+ public int partitionSpecId() {
+ throw new UnsupportedOperationException(
+ "Tracked manifests are not bound to a single partition spec");
+ }
+
+ @Override
+ public ManifestContent content() {
+ return file.contentType() == FileContent.DATA_MANIFEST
+ ? ManifestContent.DATA
+ : ManifestContent.DELETES;
+ }
+
+ @Override
+ public long sequenceNumber() {
+ return file.tracking().dataSequenceNumber();
+ }
+
+ @Override
+ public long minSequenceNumber() {
+ return file.manifestInfo().minSequenceNumber();
+ }
+
+ @Override
+ public Long snapshotId() {
+ return file.tracking().snapshotId();
+ }
+
+ @Override
+ public Integer addedFilesCount() {
+ return file.manifestInfo().addedFilesCount();
+ }
+
+ @Override
+ public Long addedRowsCount() {
+ return file.manifestInfo().addedRowsCount();
+ }
+
+ @Override
+ public Integer existingFilesCount() {
+ return file.manifestInfo().existingFilesCount();
+ }
+
+ @Override
+ public Long existingRowsCount() {
+ return file.manifestInfo().existingRowsCount();
+ }
+
+ @Override
+ public Integer deletedFilesCount() {
+ return file.manifestInfo().deletedFilesCount();
+ }
+
+ @Override
+ public Long deletedRowsCount() {
+ return file.manifestInfo().deletedRowsCount();
+ }
+
+ @Override
+ // v4 does not store partition summaries on manifests, so return null.
+ public List<PartitionFieldSummary> partitions() {
+ return null;
+ }
+
+ @Override
+ public ByteBuffer keyMetadata() {
+ return file.keyMetadata();
+ }
+
+ @Override
+ public Long firstRowId() {
+ return file.tracking().firstRowId();
+ }
+
+ @Override
+ public ManifestFile copy() {
+ return new TrackedManifestFile(file.copy());
+ }
+
+ @Override
+ public boolean equals(Object other) {
Review Comment:
The other adapters don't override `equals` and `hashCode`. Is there a
particular reason it's needed here?
--
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]
