Core: Add ManifestFile adapter for v4 tracked files #16867
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,7 +24,10 @@ | |
| import java.util.Set; | ||
| import org.apache.iceberg.relocated.com.google.common.base.Preconditions; | ||
|
|
||
| /** | ||
| * Adapts {@link TrackedFile} entries to the {@link DataFile}, {@link DeleteFile}, and {@link | ||
| * ManifestFile} APIs. | ||
| */ | ||
| class TrackedFileAdapters { | ||
|
|
||
| private TrackedFileAdapters() {} | ||
|
|
@@ -53,6 +56,15 @@ static DeleteFile asEqualityDeleteFile(TrackedFile file, Map<Integer, PartitionS | |
| return new TrackedEqualityDeleteFile(file, resolveSpec(file, specsById)); | ||
| } | ||
|
|
||
| static ManifestFile asManifestFile(TrackedFile file) { | ||
| Preconditions.checkArgument( | ||
| file.contentType() == FileContent.DATA_MANIFEST | ||
| || file.contentType() == FileContent.DELETE_MANIFEST, | ||
| "Invalid content type for ManifestFile: %s", | ||
| file.contentType()); | ||
| return new TrackedManifestFile(file); | ||
| } | ||
|
|
||
| /** Shared base for all tracked file adapters. */ | ||
|
Contributor
There was a problem hiding this comment. This is no longer true. How about "Shared base for data and delete file adapters"? |
||
| private abstract static class TrackedFileAdapter<F extends ContentFile<F>> | ||
| implements ContentFile<F> { | ||
|
|
@@ -405,6 +417,120 @@ 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) { | ||
|
stevenzwu marked this conversation as resolved.
|
||
| Preconditions.checkArgument( | ||
| file.tracking().dataSequenceNumber() != null, "Invalid data sequence number: null"); | ||
| Preconditions.checkArgument( | ||
|
Contributor
There was a problem hiding this 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
Member
Author
There was a problem hiding this comment. This PR is for root-level manifest tracking where
It is required for a manifest file:
Contributor
There was a problem hiding this comment. just to add on top of what Anoop said. this is for leaf manifest file entry (TrackedFile) in the root manifest file. Hence
Contributor
There was a problem hiding this comment. Thanks for the explanation @anoopj and @stevenzwu ! |
||
| file.tracking().snapshotId() != null, "Invalid snapshot ID: null"); | ||
| Preconditions.checkArgument( | ||
| file.manifestInfo().dv() == null, | ||
| "Cannot adapt manifest with a deletion vector to ManifestFile: %s", | ||
|
Contributor
There was a problem hiding this comment. This is correct and calls into question whether this adapter is going to be useful. We expect manifest DVs to be common and if we can't adapt a manifest with a DV back to
I think we may want to avoid adding this for now, even though it seems like a reasonable addition. In order for this to be valuable, we would need to continue using
Member
Author
There was a problem hiding this comment. OK. I will convert this into a draft and we can revisit this if it's needed as we build the reader and writer. cc @stevenzwu
Contributor
There was a problem hiding this comment. In the write path, I also proposed new additions to the
Member
Author
There was a problem hiding this comment. If we add the DV, let me know and I can revive this PR. |
||
| file.location()); | ||
| 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 UnsupportedOperationException( | ||
| "Unsupported content type for manifests: " + file.contentType()); | ||
| } | ||
| } | ||
|
|
||
| @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(); | ||
| } | ||
|
|
||
| // v4 does not store partition summaries on manifests, so return null. | ||
| @Override | ||
| public List<PartitionFieldSummary> partitions() { | ||
|
stevenzwu marked this conversation as resolved.
|
||
| 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()); | ||
| } | ||
| } | ||
|
|
||
| private static PartitionSpec resolveSpec( | ||
| TrackedFile file, Map<Integer, PartitionSpec> specsById) { | ||
| Integer specId = file.specId(); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If comments require regular maintenance when touching the file, I prefer to rewrite them to minimize churn. This doesn't need to list everything that can be produced. Maybe edit this so that it say
for example, {@link DataFile}?