gaborkaszab commented on code in PR #16769:
URL: https://github.com/apache/iceberg/pull/16769#discussion_r3435606766
##########
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:
In fact we didn't use this non-partitioned case for anything. It was needed
to construct a dummy `TrackedFileStruct` object through the constructor with
required fields. But in fact the partition part was also a dummy input, it was
never exercised in the test.
`Minimal file with no tracking, used by the rejection and null-tracking
tests.`
Now, I removed that constructor for required fields from
`TrackedFileStruct`, and instead of using that I construct the bare minimum
dummy object required for failure tests, and it apparently doesn't need a dummy
partition parameter.
Maybe this answers the question [you asked
here](https://github.com/apache/iceberg/pull/16769/changes#r3431902147).
##########
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:
See my answer above on `NO_PARTITION`. I removed this constructor because I
don't think it is appropriate to use one that accepts the required fields only.
I see 2 ways: 1) is to use the builder, 2) is in tests if we want to do
something that breaks internal invariant of the class then we can use the field
setters.
Here, the purpose is to construct a dummy object for failure tests where
only the content type matters. Everything else on the object seems noise.
Renamed the method to `dummyTrackedFile` to articulate that this creates an
internal state that doesn't meet invariants.
##########
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:
Thanks for walking me through the thought process!
I followed [the spec
PR](https://github.com/apache/iceberg/pull/16025/changes#r3401038042) here that
says for `sort_order_id`:
`'sort_order_id' must be null when 'content_type' is not 0.`
and for `split_offsets` it says:
`Split offsets for the data file` that I figured means only valid for
content_type=0
I saw that these are allowed fields in V3, hence I called this out with the
comment in the tests where I remove the checks on these for eq-deletes.
Let me involve @amogh-jahagirdar into the discussion. I wonder how much
intentional it was to introduce this restriction in V4 compared to V3. If it
was, then I can call this semantics change in the PR description.
##########
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:
You're right. I initially tried to implement this using the `inheritFrom`
and `setFirstRowId` functions, but I saw that some tests expect FILE_SEQ_NUM
and DATA_SEQ_NUM to be different, that is impossible to construct using these
functions, so I reverted back using the field setters.
Renamed as you suggested.
I'm not sure about a setter for `Tracking`. We can already use the `set`
method coming from `SupportsIndexProjection` as a cheat for tests, that we
already do for other fields. I wouldn't expose more functionality for this even
if `@VisibleForTesting`. WDYT?
##########
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:
Indeed. done
--
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]
