stevenzwu commented on code in PR #16769:
URL: https://github.com/apache/iceberg/pull/16769#discussion_r3431317677
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileAdapters.java:
##########
@@ -56,13 +56,9 @@ class TestTrackedFileAdapters {
.identity("category")
.withSpecId(PARTITIONED_SPEC_ID)
.build();
-
- // Passed for unpartitioned test files, where there is no partition tuple.
- private static final PartitionData NO_PARTITION = null;
Review Comment:
previous coverage also include non partitioned cases. this changed it.
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileAdapters.java:
##########
@@ -396,20 +382,18 @@ private static PartitionData partition(String category) {
/** Minimal file with no tracking, used by the rejection and null-tracking
tests. */
private static TrackedFileStruct trackedFile(FileContent contentType) {
- return new TrackedFileStruct(
- null, contentType, "s3://bucket/file", FileFormat.PARQUET,
NO_PARTITION, 1L, 1L);
Review Comment:
why change this one?
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileAdapters.java:
##########
@@ -146,41 +137,36 @@ void testDataFileAdapterRejectsNonDataContent(FileContent
contentType) {
@Test
void testEqualityDeleteFileAdapterDelegation() {
- PartitionData partition = partition("books");
-
TrackedFileStruct file =
- new TrackedFileStruct(
- createTracking(),
- FileContent.EQUALITY_DELETES,
- "s3://bucket/eq-delete.avro",
- FileFormat.AVRO,
- partition,
- 50L,
- 512L);
- file.set(SPEC_ID_ORDINAL, PARTITIONED_SPEC_ID);
- file.set(CONTENT_STATS_ORDINAL, createContentStats());
- file.set(SORT_ORDER_ID_ORDINAL, 5);
- file.set(KEY_METADATA_ORDINAL, ByteBuffer.wrap(new byte[] {4, 5}));
- file.set(SPLIT_OFFSETS_ORDINAL, ImmutableList.of(200L));
- file.set(EQUALITY_IDS_ORDINAL, ImmutableList.of(1, 2, 3));
+ (TrackedFileStruct)
Review Comment:
why don't we change the variable type to the `TrackedFile` interface
(instead of casting here)?
`populateDerivedAndInheritedFields` also takes the interface as an arg.
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileAdapters.java:
##########
@@ -396,20 +382,18 @@ private static PartitionData partition(String category) {
/** Minimal file with no tracking, used by the rejection and null-tracking
tests. */
private static TrackedFileStruct trackedFile(FileContent contentType) {
- return new TrackedFileStruct(
- null, contentType, "s3://bucket/file", FileFormat.PARQUET,
NO_PARTITION, 1L, 1L);
+ TrackedFileStruct file = new TrackedFileStruct();
+ file.set(CONTENT_TYPE_ORDINAL, contentType.id());
+ return file;
}
- private static TrackingStruct createTracking() {
- TrackingStruct tracking = new TrackingStruct();
- tracking.set(STATUS_ORDINAL, EntryStatus.ADDED.id());
- tracking.set(SNAPSHOT_ID_ORDINAL, 42L);
+ private static void populateDerivedAndInheritedFields(TrackedFile file) {
Review Comment:
this method name is confusing tom me. we are not inheriting values here. sth
like `populateTrackfingFields` might be more intuitive.
I am also wondering if we should expose a setter of `Tracking` object in the
builder. I know normal production code probably doesn't need it. we can mark
the package private method with `@VisibleForTesting`, which is a common pattern
we do in other classes.
##########
core/src/main/java/org/apache/iceberg/TrackedFileBuilder.java:
##########
@@ -0,0 +1,352 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import java.nio.ByteBuffer;
+import java.util.List;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+class TrackedFileBuilder {
+ private final long snapshotId;
+ private final FileContent contentType;
+
+ // Required fields
+ // TODO gaborkaszab: Include writer_format_version once merged
+ private String location = null;
+ private FileFormat fileFormat = null;
+ private Long recordCount = null;
+ private Long fileSizeInBytes = null;
+ private PartitionData partitionData = null;
+
+ // optional fields
+ private Integer specId = null;
+ private ContentStats contentStats = null;
+ private Integer sortOrderId = null;
+ private DeletionVector deletionVector = null;
+ private ManifestInfo manifestInfo = null;
+ private ByteBuffer keyMetadata = null;
+ private List<Long> splitOffsets = null;
+ private List<Integer> equalityIds = null;
+
+ // tracking-related fields
+ private Tracking sourceTracking = null;
+ private boolean dvUpdated = false;
+ private ByteBuffer deletedPositions = null;
+ private ByteBuffer replacedPositions = null;
+
+ /**
+ * Creates a builder for a newly added data file entry.
+ *
+ * @param newSnapshotId the snapshot ID in which the new tracked file will
be committed
+ */
+ static TrackedFileBuilder data(long newSnapshotId) {
+ return new TrackedFileBuilder(FileContent.DATA, newSnapshotId);
+ }
+
+ /**
+ * Creates a builder for a newly added equality delete file entry.
+ *
+ * @param newSnapshotId the snapshot ID in which the new tracked file will
be committed
+ */
+ static TrackedFileBuilder equalityDelete(long newSnapshotId) {
+ return new TrackedFileBuilder(FileContent.EQUALITY_DELETES, newSnapshotId);
+ }
+
+ /**
+ * Creates a builder for a newly added data manifest entry.
+ *
+ * @param newSnapshotId the snapshot ID in which the new tracked file will
be committed
+ */
+ static TrackedFileBuilder dataManifest(long newSnapshotId) {
+ return new TrackedFileBuilder(FileContent.DATA_MANIFEST, newSnapshotId);
+ }
+
+ /**
+ * Creates a builder for a newly added delete manifest entry.
+ *
+ * @param newSnapshotId the snapshot ID in which the new tracked file will
be committed
+ */
+ static TrackedFileBuilder deleteManifest(long newSnapshotId) {
+ return new TrackedFileBuilder(FileContent.DELETE_MANIFEST, newSnapshotId);
+ }
+
+ /**
+ * Creates a builder for a tracked file derived from {@code newSource}.
+ *
+ * @param source source tracked file to copy fields from
+ * @param newSnapshotId the snapshot ID in which the new tracked file will
be committed
+ */
+ static TrackedFileBuilder from(TrackedFile source, long newSnapshotId) {
+ Preconditions.checkArgument(source != null, "Invalid source: null");
+ return new TrackedFileBuilder(source, newSnapshotId);
+ }
+
+ /**
+ * Returns a DELETED tracked file derived from {@code source}.
+ *
+ * @param source source tracked file
+ * @param newSnapshotId the snapshot ID in which the new tracked file will
be committed
+ */
+ static TrackedFile deleted(TrackedFile source, long newSnapshotId) {
+ Preconditions.checkArgument(source != null, "Invalid source: null");
+ return terminal(source, TrackingBuilder.deleted(source.tracking(),
newSnapshotId));
+ }
+
+ /**
+ * Returns a REPLACED tracked file derived from {@code source}.
+ *
+ * <p>Manifest entries cannot transition to REPLACED.
+ *
+ * @param source source tracked file
+ * @param newSnapshotId the snapshot ID in which the new tracked file will
be committed
+ */
+ static TrackedFile replaced(TrackedFile source, long newSnapshotId) {
+ Preconditions.checkArgument(source != null, "Invalid source: null");
+ Preconditions.checkArgument(
+ source.contentType() != FileContent.DATA_MANIFEST
+ && source.contentType() != FileContent.DELETE_MANIFEST,
+ "Manifest entries cannot transition to REPLACED, but entry type is:
%s",
+ source.contentType());
+ return terminal(source, TrackingBuilder.replaced(source.tracking(),
newSnapshotId));
+ }
+
+ private static TrackedFile terminal(TrackedFile source, Tracking tracking) {
+ return new TrackedFileStruct(
+ tracking,
+ source.contentType(),
+ source.location(),
+ source.fileFormat(),
+ (PartitionData) source.partition(),
+ source.recordCount(),
+ source.fileSizeInBytes(),
+ source.specId(),
+ source.contentStats(),
+ source.sortOrderId(),
+ source.deletionVector(),
+ source.manifestInfo(),
+ source.keyMetadata(),
+ source.splitOffsets(),
+ source.equalityIds());
+ }
+
+ private TrackedFileBuilder(FileContent contentType, long snapshotId) {
+ this.contentType = contentType;
+ this.snapshotId = snapshotId;
+ }
+
+ private TrackedFileBuilder(TrackedFile source, long snapshotId) {
+ this.contentType = source.contentType();
+ this.snapshotId = snapshotId;
+ this.location = source.location();
+ this.fileFormat = source.fileFormat();
+ this.recordCount = source.recordCount();
+ this.fileSizeInBytes = source.fileSizeInBytes();
+ this.partitionData = (PartitionData) source.partition();
+ this.specId = source.specId();
+ this.contentStats = source.contentStats();
+ this.sortOrderId = source.sortOrderId();
+ this.deletionVector = source.deletionVector();
+ this.manifestInfo = source.manifestInfo();
+ this.keyMetadata = source.keyMetadata();
+ this.splitOffsets = source.splitOffsets();
+ this.equalityIds = source.equalityIds();
+ this.sourceTracking = source.tracking();
+ }
+
+ TrackedFileBuilder location(String newLocation) {
+ Preconditions.checkArgument(newLocation != null, "Invalid location: null");
+ this.location = newLocation;
+ return this;
+ }
+
+ TrackedFileBuilder fileFormat(FileFormat newFileFormat) {
+ Preconditions.checkArgument(newFileFormat != null, "Invalid file format:
null");
+ this.fileFormat = newFileFormat;
+ return this;
+ }
+
+ TrackedFileBuilder recordCount(long newRecordCount) {
+ Preconditions.checkArgument(
+ newRecordCount >= 0, "Invalid record count: %s (must be >= 0)",
newRecordCount);
+ this.recordCount = newRecordCount;
+ return this;
+ }
+
+ TrackedFileBuilder fileSizeInBytes(long newFileSizeInBytes) {
+ Preconditions.checkArgument(
+ newFileSizeInBytes >= 0,
+ "Invalid file size in bytes: %s (must be >= 0)",
+ newFileSizeInBytes);
+ this.fileSizeInBytes = newFileSizeInBytes;
+ return this;
+ }
+
+ TrackedFileBuilder specId(int newSpecId) {
+ Preconditions.checkArgument(newSpecId >= 0, "Invalid spec ID: %s (must be
>= 0)", newSpecId);
+ this.specId = newSpecId;
+ return this;
+ }
+
+ TrackedFileBuilder partition(PartitionData newPartitionData) {
+ Preconditions.checkArgument(newPartitionData != null, "Invalid partition:
null");
+ this.partitionData = newPartitionData;
+ return this;
+ }
+
+ TrackedFileBuilder contentStats(ContentStats newContentStats) {
+ Preconditions.checkArgument(newContentStats != null, "Invalid content
stats: null");
+ this.contentStats = newContentStats;
+ return this;
+ }
+
+ TrackedFileBuilder sortOrderId(int newSortOrderId) {
+ Preconditions.checkArgument(
+ contentType == FileContent.DATA,
Review Comment:
The `contentType == FileContent.DATA` restriction on `sortOrderId` (here)
and `splitOffsets` (line 263-271) is a **new invariant introduced by this PR**,
not an inherited one. It diverges from the spec and from existing v3 writer
behavior:
**Spec** ([format/spec.md, data_file
fields](https://github.com/apache/iceberg/blob/main/format/spec.md#data-file-fields))
marks both fields `_optional_ | _optional_ | _optional_` across v1/v2/v3 with
no per-content-type restriction:
```
| _optional_ | _optional_ | _optional_ | 132 split_offsets | list<long>
| Split offsets for the data file ...
| _optional_ | _optional_ | _optional_ | 140 sort_order_id | int
| ID representing sort order for this file
```
**v3 writer behavior** — `EqualityDeleteWriter` actively sets BOTH on
equality delete files
([EqualityDeleteWriter.java:80-92](https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/deletes/EqualityDeleteWriter.java#L80-L92)):
```java
FileMetadata.deleteFileBuilder(spec)
.ofEqualityDeletes(equalityFieldIds)
...
.withSplitOffsets(appender.splitOffsets())
.withSortOrder(sortOrder)
.build();
```
And `FileMetadata.Builder.build()`
([FileMetadata.java:274-286](https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/FileMetadata.java#L274-L286))
explicitly defaults `sortOrderId` to `SortOrder.unsorted().orderId()` for
EQUALITY_DELETES if not set:
```java
switch (content) {
case POSITION_DELETES:
Preconditions.checkArgument(sortOrderId == null, "Position delete file
should not have sort order");
break;
case EQUALITY_DELETES:
if (sortOrderId == null) {
sortOrderId = SortOrder.unsorted().orderId();
}
break;
...
}
```
So in v3 / spec terms, **POSITION_DELETES** is the only content type where
`sort_order_id` is prohibited; equality deletes always carry one. The v4 schema
(`TrackedFile.SORT_ORDER_ID`, `TrackedFile.SPLIT_OFFSETS`) doesn't gate by
content type either, and `TrackedEqualityDeleteFile`
([TrackedFileAdapters.java:156-167](https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/TrackedFileAdapters.java#L156-L167))
blindly delegates both fields — meaning a v4 EQ-delete entry that does carry
them will surface them on the legacy `DeleteFile`.
This restriction also drove the assertion deletion in
`TestTrackedFileAdapters.testEqualityDeleteFileAdapterDelegation` (your reply:
https://github.com/apache/iceberg/pull/16769#discussion_r3427966408). That
removal is locally consistent with the builder's contract but rests on the
unstated premise that the contract itself is correct — which the spec doesn't
support.
Two options:
1. **Relax** the restriction to match the spec — allow `sortOrderId` on
`DATA | EQUALITY_DELETES` (gate against POSITION_DELETES if anything), and drop
the content-type check on `splitOffsets` entirely. Then the deleted adapter
assertions can come back.
2. If there's a deliberate v4 design decision to drop these fields from
EQ-deletes, please call it out in the PR description (currently empty) and link
to wherever it's discussed — it's a content-type-semantics change, not a
builder ergonomics one, and shouldn't be silent.
Same applies to the analogous check on `splitOffsets` at line 263-271.
--
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]
