stevenzwu commented on code in PR #16936:
URL: https://github.com/apache/iceberg/pull/16936#discussion_r3599165043
##########
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:
Done — collapsed to a single line matching the sibling count methods,
keeping the pre-v4 `null` note.
##########
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:
Done — reduced to a single line, mirroring `replacedFilesCount()`.
##########
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:
Dropped the `LEGACY_FORMAT_VERSION` reference. Kept "or 0 for pre-v4
manifests" so the sentinel value stays documented.
##########
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:
Direct answer to the question: yes, the caller decides the status — that is
unavoidable and already how the `ManifestWriter` API works today
(`add`/`existing`/`delete` encode it).
The confusing part is that this PR's adapter re-encodes that decision as six
`wrap*` methods, layered on top of `TrackingBuilder` which also encodes it.
Proposal: drop the per-status methods and have the adapter take a pre-built
`Tracking`:
```java
DataTrackedFile wrap(DataFile file, Tracking tracking)
```
`TrackingBuilder` stays the single place that builds the `Tracking`
(added/from/deleted/replaced, `dv_snapshot_id`, deleted/replaced positions);
the adapter only forwards file-derived fields (partition, stats, manifest_info,
DV). The Phase 2 v4 writer builds the `Tracking` via `TrackingBuilder` and
hands it to the adapter. Does that match what you had in mind?
##########
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:
Removed — the check enumerated all five `EntryStatus` values, so it was a
tautology. Retained the null check.
##########
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:
Switched to plain `null` for consistency with the other methods 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]
