gaborkaszab commented on code in PR #16936:
URL: https://github.com/apache/iceberg/pull/16936#discussion_r3499550208
##########
api/src/main/java/org/apache/iceberg/ManifestFile.java:
##########
@@ -186,6 +192,26 @@ default boolean hasDeletedFiles() {
/** Returns the total number of rows in all files with status DELETED in the
manifest file. */
Long deletedRowsCount();
+ /**
Review Comment:
This seems over-explained compared to other function in this class. I think
it enough to simply say:
`/** Returns the number of files with status REPLACED in the manifest file.
*/`
We might want to add `, or null for pre-v4 manifests` but I'm not convinced
on this.
##########
api/src/main/java/org/apache/iceberg/ManifestFile.java:
##########
@@ -186,6 +192,26 @@ default boolean hasDeletedFiles() {
/** Returns the total number of rows in all files with status DELETED in the
manifest file. */
Long deletedRowsCount();
+ /**
+ * Returns the number of files with status REPLACED in the manifest file, or
null if not tracked.
+ *
+ * <p>REPLACED files are the prior-state entries of v4 REPLACED/MODIFIED
pairs and are not live.
+ * Returns null for manifest files written by pre-v4 writers.
+ */
+ default Integer replacedFilesCount() {
+ return null;
+ }
+
+ /**
+ * Returns the total number of rows in all files with status REPLACED in the
manifest file, or
+ * null if not tracked.
+ *
+ * <p>Returns null for manifest files written by pre-v4 writers.
Review Comment:
nit: this seems redundant, would merge with above similarly as I commented
for the other function in this file.
##########
api/src/main/java/org/apache/iceberg/ManifestFile.java:
##########
@@ -210,6 +236,19 @@ default Long firstRowId() {
return null;
}
+ /** Returns the number of records in the manifest file, or {@code null} for
pre-v4 manifests. */
Review Comment:
nit: is `{@code null}` needed instead of simply `null`?
##########
api/src/main/java/org/apache/iceberg/ManifestFile.java:
##########
@@ -210,6 +236,19 @@ default Long firstRowId() {
return null;
}
+ /** Returns the number of records in the manifest file, or {@code null} for
pre-v4 manifests. */
+ default Long recordCount() {
+ return null;
+ }
+
+ /**
Review Comment:
`LEGACY_FORMAT_VERSION` is an implementation detail IMO. Simply this?
`/** Returns the format version of the manifest file. */`
##########
core/src/main/java/org/apache/iceberg/TrackedFileAdapters.java:
##########
@@ -425,4 +482,980 @@ private static PartitionSpec resolveSpec(
throw new IllegalArgumentException(
"Cannot find unpartitioned spec in specs: " + specsById.keySet());
}
+
+ // Content_entry on-disk schema field count and per-position ordering,
mirroring
+ // TrackedFileStruct.BASE_TYPE and TrackedFileStruct.getByPos so the
wrapper's StructLike view
+ // matches what the Parquet writer expects when it iterates fields by
ordinal.
+ private static final int CONTENT_ENTRY_FIELD_COUNT = 16;
+
+ /** Shared base for content-file (DATA / EQUALITY_DELETES) write-direction
wrappers. */
+ abstract static class ContentTrackedFile<F extends ContentFile<F>>
+ implements TrackedFile, StructLike {
+ private final int formatVersion;
+ private final Schema tableSchema;
+ private final Map<Integer, Type> primitiveTypesById;
+ private final Types.StructType partitionType;
+ private final WrappedTracking tracking = new WrappedTracking();
+ private F file;
+ private StructProjection partition;
+ private ContentStats stats;
+
+ ContentTrackedFile(int formatVersion, Schema tableSchema, Types.StructType
partitionType) {
+ Preconditions.checkArgument(
+ formatVersion >=
TableMetadata.MIN_FORMAT_VERSION_ADAPTIVE_MANIFEST_TREE,
+ "Invalid format version for adaptive manifest tree: %s (must be >=
%s)",
+ formatVersion,
+ TableMetadata.MIN_FORMAT_VERSION_ADAPTIVE_MANIFEST_TREE);
+ Preconditions.checkArgument(tableSchema != null, "Invalid table schema:
null");
+ Preconditions.checkArgument(partitionType != null, "Invalid partition
type: null");
+ this.formatVersion = formatVersion;
+ this.tableSchema = tableSchema;
+ this.primitiveTypesById = primitiveTypesFor(tableSchema);
+ this.partitionType = partitionType;
+ }
+
+ final void wrapWithTracking(
+ F newFile, EntryStatus status, Long snapshotId, Long dataSeq, Long
fileSeq) {
+ wrapWithTracking(newFile, status, snapshotId, dataSeq, fileSeq, null);
+ }
+
+ final void wrapWithTracking(
+ F newFile,
+ EntryStatus status,
+ Long snapshotId,
+ Long dataSeq,
+ Long fileSeq,
+ Long dvSnapshotId) {
+ Preconditions.checkArgument(newFile != null, "Invalid file: null");
+ Preconditions.checkArgument(status != null, "Invalid status: null");
+ validateContent(newFile);
+ Preconditions.checkArgument(
+ status == EntryStatus.ADDED
Review Comment:
This lists all the enum valid values
##########
core/src/main/java/org/apache/iceberg/TrackedFileAdapters.java:
##########
@@ -425,4 +482,980 @@ private static PartitionSpec resolveSpec(
throw new IllegalArgumentException(
"Cannot find unpartitioned spec in specs: " + specsById.keySet());
}
+
+ // Content_entry on-disk schema field count and per-position ordering,
mirroring
+ // TrackedFileStruct.BASE_TYPE and TrackedFileStruct.getByPos so the
wrapper's StructLike view
+ // matches what the Parquet writer expects when it iterates fields by
ordinal.
+ private static final int CONTENT_ENTRY_FIELD_COUNT = 16;
+
+ /** Shared base for content-file (DATA / EQUALITY_DELETES) write-direction
wrappers. */
+ abstract static class ContentTrackedFile<F extends ContentFile<F>>
+ implements TrackedFile, StructLike {
+ private final int formatVersion;
+ private final Schema tableSchema;
+ private final Map<Integer, Type> primitiveTypesById;
+ private final Types.StructType partitionType;
+ private final WrappedTracking tracking = new WrappedTracking();
+ private F file;
+ private StructProjection partition;
+ private ContentStats stats;
+
+ ContentTrackedFile(int formatVersion, Schema tableSchema, Types.StructType
partitionType) {
+ Preconditions.checkArgument(
+ formatVersion >=
TableMetadata.MIN_FORMAT_VERSION_ADAPTIVE_MANIFEST_TREE,
+ "Invalid format version for adaptive manifest tree: %s (must be >=
%s)",
+ formatVersion,
+ TableMetadata.MIN_FORMAT_VERSION_ADAPTIVE_MANIFEST_TREE);
+ Preconditions.checkArgument(tableSchema != null, "Invalid table schema:
null");
+ Preconditions.checkArgument(partitionType != null, "Invalid partition
type: null");
+ this.formatVersion = formatVersion;
+ this.tableSchema = tableSchema;
+ this.primitiveTypesById = primitiveTypesFor(tableSchema);
+ this.partitionType = partitionType;
+ }
+
+ final void wrapWithTracking(
+ F newFile, EntryStatus status, Long snapshotId, Long dataSeq, Long
fileSeq) {
+ wrapWithTracking(newFile, status, snapshotId, dataSeq, fileSeq, null);
+ }
+
+ final void wrapWithTracking(
Review Comment:
There is a lot of loci in TrackingBuilder to take care of status
transitions, and setting `dvSnapshotId`. I figured that the point of that
builder is to use it on the write path when creating a new entry or using an
existing one as a source, so that we don't have to deal with setting all these
manually.
I'm a bit confused now about the usage of that builder. And in general, I
don't really see the usage of these new adapters either. Do we expect the
caller to figure out itself what status it wants for the entries and call the
respective `wrapExisting`, `wrapAdded` etc. function?
--
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]
