gaborkaszab commented on code in PR #16285:
URL: https://github.com/apache/iceberg/pull/16285#discussion_r3740558462
##########
core/src/main/java/org/apache/iceberg/TrackingBuilder.java:
##########
@@ -111,10 +115,27 @@ TrackingBuilder dvUpdated() {
return this;
}
+ /** Indicates that the column files list has been updated for the new
Tracking. */
+ TrackingBuilder columnFilesUpdated() {
+ Preconditions.checkState(
+ deletedPositions == null && replacedPositions == null,
+ "Cannot mark column files updated on a manifest entry
(deleted/replaced positions are set)");
+ this.latestColumnFileSnapshotId = newSnapshotId;
+ if (status == EntryStatus.EXISTING) {
+ this.status = EntryStatus.MODIFIED;
+ }
+ // Bumping 'dataSequenceNumber' to avoid having both equality deletes and
column files.
+ this.dataSequenceNumber = null;
Review Comment:
Thanks for the comment suggestion! Added
##########
core/src/main/java/org/apache/iceberg/TrackingBuilder.java:
##########
@@ -115,6 +118,17 @@ TrackingBuilder dvUpdated() {
return this;
}
+ /** Indicates that the column files list has been updated for the new
Tracking. */
+ TrackingBuilder columnFilesUpdated() {
+ this.latestColumnFileSnapshotId = newSnapshotId;
+ if (status == EntryStatus.EXISTING) {
+ this.status = EntryStatus.MODIFIED;
+ }
+ // Bumping 'dataSequenceNumber' to avoid having both equality deletes and
column files.
Review Comment:
Thanks for the suggestion! Steven also had one, I went with that.
##########
core/src/test/java/org/apache/iceberg/TestTrackingStruct.java:
##########
@@ -175,10 +204,22 @@ void doNotInheritSequenceNumberForModifiedEntries() {
assertThat(tracking.fileSequenceNumber()).isEqualTo(6L);
}
+ @Test
+ void inheritDataSequenceNumberAfterColumnFilesChange() {
+ // Adding column files should set data sequence number to null
Review Comment:
It's not redundant with the other inheritance tests, because the other ones
test either other status than MODIFIED, or MODIFIED with non null sequence
numbers (where we expect no inheritance). While this one tests that MODIFIED
with null data sequence number triggers inheritance.
Additionally, I recall we said we want to avoid using the Builder in these
test suites for setting up objects, rather we use the all-param constructor.
##########
core/src/main/java/org/apache/iceberg/ColumnFileStruct.java:
##########
@@ -0,0 +1,271 @@
+/*
+ * 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.io.Serializable;
+import java.nio.ByteBuffer;
+import java.util.Arrays;
+import java.util.List;
+import org.apache.iceberg.avro.SupportsIndexProjection;
+import org.apache.iceberg.relocated.com.google.common.base.MoreObjects;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+import org.apache.iceberg.types.Types;
+import org.apache.iceberg.util.ArrayUtil;
+import org.apache.iceberg.util.ByteBuffers;
+
+/** Mutable {@link StructLike} implementation of {@link ColumnFile}. */
+class ColumnFileStruct extends SupportsIndexProjection implements ColumnFile,
Serializable {
+ private static final Types.StructType BASE_TYPE =
+ Types.StructType.of(
+ ColumnFile.FORMAT_VERSION,
+ ColumnFile.FIELD_IDS,
+ ColumnFile.LOCATION,
+ ColumnFile.FILE_FORMAT,
+ ColumnFile.FILE_SIZE_IN_BYTES,
+ ColumnFile.KEY_METADATA,
+ ColumnFile.SPLIT_OFFSETS);
+
+ private int formatVersion = -1;
+ private int[] fieldIds = null;
+ private String location = null;
+ private FileFormat fileFormat = null;
+ private long fileSizeInBytes = -1L;
+ private byte[] keyMetadata = null;
+ private long[] splitOffsets = null;
+
+ /** Used by internal readers to instantiate this class with a projection
schema. */
+ ColumnFileStruct(Types.StructType projection) {
+ super(BASE_TYPE, projection);
+ }
+
+ ColumnFileStruct(
+ int formatVersion,
+ List<Integer> fieldIds,
+ String location,
+ FileFormat fileFormat,
+ long fileSizeInBytes,
+ ByteBuffer keyMetadata,
+ List<Long> splitOffsets) {
+ super(BASE_TYPE.fields().size());
+ this.formatVersion = formatVersion;
+ this.fieldIds = ArrayUtil.toIntArray(fieldIds);
+ this.location = location;
+ this.fileFormat = fileFormat;
+ this.fileSizeInBytes = fileSizeInBytes;
+ this.keyMetadata = ByteBuffers.toByteArray(keyMetadata);
+ this.splitOffsets = ArrayUtil.toLongArray(splitOffsets);
+ }
+
+ /** Copy constructor. */
+ private ColumnFileStruct(ColumnFileStruct toCopy) {
+ super(toCopy);
+ this.formatVersion = toCopy.formatVersion;
+ this.fieldIds =
+ toCopy.fieldIds != null ? Arrays.copyOf(toCopy.fieldIds,
toCopy.fieldIds.length) : null;
+ this.location = toCopy.location;
+ this.fileFormat = toCopy.fileFormat;
+ this.fileSizeInBytes = toCopy.fileSizeInBytes;
+ this.keyMetadata =
+ toCopy.keyMetadata != null
+ ? Arrays.copyOf(toCopy.keyMetadata, toCopy.keyMetadata.length)
+ : null;
+ this.splitOffsets =
+ toCopy.splitOffsets != null
+ ? Arrays.copyOf(toCopy.splitOffsets, toCopy.splitOffsets.length)
+ : null;
+ }
+
+ /** Constructor for Java serialization. */
+ ColumnFileStruct() {
+ super(BASE_TYPE.fields().size());
+ }
+
+ @Override
+ public int formatVersion() {
+ return formatVersion;
+ }
+
+ @Override
+ public List<Integer> fieldIds() {
+ return fieldIds != null ? ArrayUtil.toUnmodifiableIntList(fieldIds) : null;
+ }
+
+ @Override
+ public String location() {
+ return location;
+ }
+
+ @Override
+ public FileFormat fileFormat() {
+ return fileFormat;
+ }
+
+ @Override
+ public long fileSizeInBytes() {
+ return fileSizeInBytes;
+ }
+
+ @Override
+ public ByteBuffer keyMetadata() {
+ return keyMetadata != null ? ByteBuffer.wrap(keyMetadata) : null;
+ }
+
+ @Override
+ public List<Long> splitOffsets() {
+ return splitOffsets != null ?
ArrayUtil.toUnmodifiableLongList(splitOffsets) : null;
+ }
+
+ @Override
+ public ColumnFile copy() {
+ return new ColumnFileStruct(this);
+ }
+
+ @Override
+ protected <T> T internalGet(int pos, Class<T> javaClass) {
+ return javaClass.cast(getByPos(pos));
+ }
+
+ private Object getByPos(int pos) {
+ return switch (pos) {
+ case 0 -> formatVersion;
+ case 1 -> fieldIds();
+ case 2 -> location;
+ case 3 -> fileFormat != null ? fileFormat.toString() : null;
+ case 4 -> fileSizeInBytes;
+ case 5 -> keyMetadata();
+ case 6 -> splitOffsets();
+ default -> throw new UnsupportedOperationException("Unknown field
ordinal: " + pos);
+ };
+ }
+
+ @Override
+ @SuppressWarnings("unchecked")
+ protected <T> void internalSet(int pos, T value) {
+ switch (pos) {
+ case 0 -> this.formatVersion = (int) value;
+ case 1 -> this.fieldIds = ArrayUtil.toIntArray((List<Integer>) value);
+ // always coerce to String for Serializable
Review Comment:
Just tried, that breaks the spotless checks
##########
core/src/test/java/org/apache/iceberg/TestColumnFileStruct.java:
##########
@@ -0,0 +1,280 @@
+/*
+ * 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 static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.util.List;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.MethodSource;
+
+class TestColumnFileStruct {
+
+ private static final int FORMAT_VERSION = 4;
+ private static final List<Integer> FIELD_IDS = Lists.newArrayList(1, 2, 3);
+ private static final String LOCATION = "s3://bucket/data/column.parquet";
+ private static final FileFormat FILE_FORMAT = FileFormat.PARQUET;
+ private static final long FILE_SIZE_IN_BYTES = 1024L;
+ private static final ByteBuffer KEY_METADATA = ByteBuffer.wrap(new byte[]
{1, 2, 3});
+ private static final List<Long> SPLIT_OFFSETS = Lists.newArrayList(0L, 512L);
+
+ @Test
+ void fieldAccess() {
+ ColumnFile columnFile =
+ new ColumnFileStruct(
+ FORMAT_VERSION,
+ FIELD_IDS,
+ LOCATION,
+ FILE_FORMAT,
+ FILE_SIZE_IN_BYTES,
+ KEY_METADATA,
+ SPLIT_OFFSETS);
+
+ assertThat(columnFile.formatVersion()).isEqualTo(FORMAT_VERSION);
+ assertThat(columnFile.fieldIds()).containsExactlyElementsOf(FIELD_IDS);
+ assertThat(columnFile.location()).isEqualTo(LOCATION);
+ assertThat(columnFile.fileFormat()).isEqualTo(FILE_FORMAT);
+ assertThat(columnFile.fileSizeInBytes()).isEqualTo(FILE_SIZE_IN_BYTES);
+ assertThat(columnFile.keyMetadata()).isEqualTo(KEY_METADATA);
+
assertThat(columnFile.splitOffsets()).containsExactlyElementsOf(SPLIT_OFFSETS);
+ }
+
+ @Test
+ void copy() {
+ ColumnFile columnFile =
+ new ColumnFileStruct(
+ FORMAT_VERSION,
+ FIELD_IDS,
+ LOCATION,
+ FILE_FORMAT,
+ FILE_SIZE_IN_BYTES,
+ KEY_METADATA,
+ SPLIT_OFFSETS);
+
+ ColumnFile copy = columnFile.copy();
+
+ assertThat(copy.formatVersion()).isEqualTo(FORMAT_VERSION);
+ assertThat(copy.fieldIds()).containsExactlyElementsOf(FIELD_IDS);
+ assertThat(copy.location()).isEqualTo(LOCATION);
+ assertThat(copy.fileFormat()).isEqualTo(FILE_FORMAT);
+ assertThat(copy.fileSizeInBytes()).isEqualTo(FILE_SIZE_IN_BYTES);
+ assertThat(copy.keyMetadata()).isEqualTo(KEY_METADATA);
+ assertThat(copy.splitOffsets()).containsExactlyElementsOf(SPLIT_OFFSETS);
+ }
+
+ @Test
+ void structLikeSize() {
+ ColumnFileStruct columnFile = new ColumnFileStruct();
+ assertThat(columnFile.size()).isEqualTo(7);
+ }
+
+ @Test
+ void setFieldsByOrdinals() {
+ ColumnFileStruct columnFile = new ColumnFileStruct();
+
+ columnFile.set(0, FORMAT_VERSION);
+ columnFile.set(1, FIELD_IDS);
+ columnFile.set(2, LOCATION);
+ columnFile.set(3, FILE_FORMAT.toString());
+ columnFile.set(4, FILE_SIZE_IN_BYTES);
+ columnFile.set(5, KEY_METADATA);
+ columnFile.set(6, SPLIT_OFFSETS);
+
+ assertThat(columnFile.formatVersion()).isEqualTo(FORMAT_VERSION);
+ assertThat(columnFile.fieldIds()).containsExactlyElementsOf(FIELD_IDS);
+ assertThat(columnFile.location()).isEqualTo(LOCATION);
+ assertThat(columnFile.fileFormat()).isEqualTo(FILE_FORMAT);
+ assertThat(columnFile.fileSizeInBytes()).isEqualTo(FILE_SIZE_IN_BYTES);
+ assertThat(columnFile.keyMetadata()).isEqualTo(KEY_METADATA);
+
assertThat(columnFile.splitOffsets()).containsExactlyElementsOf(SPLIT_OFFSETS);
+ }
+
+ @Test
+ void getFieldsByOrdinals() {
+ ColumnFileStruct columnFile =
+ new ColumnFileStruct(
+ FORMAT_VERSION,
+ FIELD_IDS,
+ LOCATION,
+ FILE_FORMAT,
+ FILE_SIZE_IN_BYTES,
+ KEY_METADATA,
+ SPLIT_OFFSETS);
+
+ assertThat(columnFile.get(0, Integer.class)).isEqualTo(FORMAT_VERSION);
+ assertThat(columnFile.get(1,
List.class)).containsExactlyElementsOf(FIELD_IDS);
+ assertThat(columnFile.get(2, String.class)).isEqualTo(LOCATION);
+ assertThat(columnFile.get(3,
String.class)).isEqualTo(FILE_FORMAT.toString());
+ assertThat(columnFile.get(4, Long.class)).isEqualTo(FILE_SIZE_IN_BYTES);
+ assertThat(columnFile.get(5, ByteBuffer.class)).isEqualTo(KEY_METADATA);
+ assertThat(columnFile.get(6,
List.class)).containsExactlyElementsOf(SPLIT_OFFSETS);
+ }
+
+ @Test
+ void projectedStructLike() {
+ Types.StructType projection =
+ Types.StructType.of(ColumnFile.LOCATION,
ColumnFile.FILE_SIZE_IN_BYTES);
+
+ ColumnFileStruct columnFile = new ColumnFileStruct(projection);
+ assertThat(columnFile.size()).isEqualTo(2);
+
+ // projected position 0 maps to internal position of location
+ // projected position 1 maps to internal position of file_size_in_bytes
+ columnFile.set(0, LOCATION);
+ columnFile.set(1, 1024L);
+
+ assertThat(columnFile.location()).isEqualTo(LOCATION);
+ assertThat(columnFile.fileSizeInBytes()).isEqualTo(1024L);
+ assertThat(columnFile.get(0, String.class)).isEqualTo(LOCATION);
+ assertThat(columnFile.get(1, Long.class)).isEqualTo(1024L);
+ }
+
+ @ParameterizedTest
+ @MethodSource("org.apache.iceberg.TestHelpers#serializers")
+ void serializationRoundTrip(TestHelpers.RoundTripSerializer<ColumnFile>
roundTripSerializer)
+ throws IOException, ClassNotFoundException {
+ ColumnFile columnFile =
+ new ColumnFileStruct(
+ FORMAT_VERSION,
+ FIELD_IDS,
+ LOCATION,
+ FILE_FORMAT,
+ FILE_SIZE_IN_BYTES,
+ KEY_METADATA,
+ SPLIT_OFFSETS);
+
+ ColumnFile deserialized = roundTripSerializer.apply(columnFile);
+
+ assertThat(deserialized.formatVersion()).isEqualTo(FORMAT_VERSION);
Review Comment:
Other tests do the comparison field by field, however, your suggestion seems
a good simplification. Applied
##########
core/src/main/java/org/apache/iceberg/ColumnFileStruct.java:
##########
@@ -0,0 +1,271 @@
+/*
+ * 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.io.Serializable;
+import java.nio.ByteBuffer;
+import java.util.Arrays;
+import java.util.List;
+import org.apache.iceberg.avro.SupportsIndexProjection;
+import org.apache.iceberg.relocated.com.google.common.base.MoreObjects;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+import org.apache.iceberg.types.Types;
+import org.apache.iceberg.util.ArrayUtil;
+import org.apache.iceberg.util.ByteBuffers;
+
+/** Mutable {@link StructLike} implementation of {@link ColumnFile}. */
+class ColumnFileStruct extends SupportsIndexProjection implements ColumnFile,
Serializable {
+ private static final Types.StructType BASE_TYPE =
+ Types.StructType.of(
+ ColumnFile.FORMAT_VERSION,
+ ColumnFile.FIELD_IDS,
+ ColumnFile.LOCATION,
+ ColumnFile.FILE_FORMAT,
+ ColumnFile.FILE_SIZE_IN_BYTES,
+ ColumnFile.KEY_METADATA,
+ ColumnFile.SPLIT_OFFSETS);
+
+ private int formatVersion = -1;
+ private int[] fieldIds = null;
+ private String location = null;
+ private FileFormat fileFormat = null;
+ private long fileSizeInBytes = -1L;
+ private byte[] keyMetadata = null;
+ private long[] splitOffsets = null;
+
+ /** Used by internal readers to instantiate this class with a projection
schema. */
+ ColumnFileStruct(Types.StructType projection) {
+ super(BASE_TYPE, projection);
+ }
+
+ ColumnFileStruct(
+ int formatVersion,
+ List<Integer> fieldIds,
+ String location,
+ FileFormat fileFormat,
+ long fileSizeInBytes,
+ ByteBuffer keyMetadata,
+ List<Long> splitOffsets) {
+ super(BASE_TYPE.fields().size());
+ this.formatVersion = formatVersion;
+ this.fieldIds = ArrayUtil.toIntArray(fieldIds);
+ this.location = location;
+ this.fileFormat = fileFormat;
+ this.fileSizeInBytes = fileSizeInBytes;
+ this.keyMetadata = ByteBuffers.toByteArray(keyMetadata);
+ this.splitOffsets = ArrayUtil.toLongArray(splitOffsets);
+ }
+
+ /** Copy constructor. */
+ private ColumnFileStruct(ColumnFileStruct toCopy) {
+ super(toCopy);
+ this.formatVersion = toCopy.formatVersion;
+ this.fieldIds =
+ toCopy.fieldIds != null ? Arrays.copyOf(toCopy.fieldIds,
toCopy.fieldIds.length) : null;
+ this.location = toCopy.location;
+ this.fileFormat = toCopy.fileFormat;
+ this.fileSizeInBytes = toCopy.fileSizeInBytes;
+ this.keyMetadata =
+ toCopy.keyMetadata != null
+ ? Arrays.copyOf(toCopy.keyMetadata, toCopy.keyMetadata.length)
+ : null;
+ this.splitOffsets =
+ toCopy.splitOffsets != null
+ ? Arrays.copyOf(toCopy.splitOffsets, toCopy.splitOffsets.length)
+ : null;
+ }
+
+ /** Constructor for Java serialization. */
+ ColumnFileStruct() {
+ super(BASE_TYPE.fields().size());
+ }
+
+ @Override
+ public int formatVersion() {
+ return formatVersion;
+ }
+
+ @Override
+ public List<Integer> fieldIds() {
+ return fieldIds != null ? ArrayUtil.toUnmodifiableIntList(fieldIds) : null;
+ }
+
+ @Override
+ public String location() {
+ return location;
+ }
+
+ @Override
+ public FileFormat fileFormat() {
+ return fileFormat;
+ }
+
+ @Override
+ public long fileSizeInBytes() {
+ return fileSizeInBytes;
+ }
+
+ @Override
+ public ByteBuffer keyMetadata() {
+ return keyMetadata != null ? ByteBuffer.wrap(keyMetadata) : null;
+ }
+
+ @Override
+ public List<Long> splitOffsets() {
+ return splitOffsets != null ?
ArrayUtil.toUnmodifiableLongList(splitOffsets) : null;
+ }
+
+ @Override
+ public ColumnFile copy() {
+ return new ColumnFileStruct(this);
+ }
+
+ @Override
+ protected <T> T internalGet(int pos, Class<T> javaClass) {
+ return javaClass.cast(getByPos(pos));
+ }
+
+ private Object getByPos(int pos) {
+ return switch (pos) {
+ case 0 -> formatVersion;
+ case 1 -> fieldIds();
+ case 2 -> location;
+ case 3 -> fileFormat != null ? fileFormat.toString() : null;
+ case 4 -> fileSizeInBytes;
+ case 5 -> keyMetadata();
+ case 6 -> splitOffsets();
+ default -> throw new UnsupportedOperationException("Unknown field
ordinal: " + pos);
+ };
+ }
+
+ @Override
+ @SuppressWarnings("unchecked")
+ protected <T> void internalSet(int pos, T value) {
+ switch (pos) {
+ case 0 -> this.formatVersion = (int) value;
+ case 1 -> this.fieldIds = ArrayUtil.toIntArray((List<Integer>) value);
+ // always coerce to String for Serializable
+ case 2 -> this.location = value.toString();
+ case 3 -> this.fileFormat = FileFormat.fromString(value.toString());
+ case 4 -> this.fileSizeInBytes = (long) value;
+ case 5 -> this.keyMetadata = ByteBuffers.toByteArray((ByteBuffer) value);
+ case 6 -> this.splitOffsets = ArrayUtil.toLongArray((List<Long>) value);
+ default -> {
+ // ignore the object, it must be from a newer version of the format
Review Comment:
This comment is inline with the same in `TrackedFileStruct` and
`TrackingStruct`. I'd rather keep consistency with these.
##########
core/src/main/java/org/apache/iceberg/TrackingStruct.java:
##########
@@ -130,11 +135,13 @@ void inheritFrom(Tracking manifestTracking) {
manifestTracking.dataSequenceNumber(),
manifestTracking.fileSequenceNumber());
- if (status == EntryStatus.ADDED) {
+ if (status == EntryStatus.ADDED || status == EntryStatus.MODIFIED) {
if (dataSequenceNumber == null) {
this.dataSequenceNumber = manifestTracking.fileSequenceNumber();
}
+ }
+ if (status == EntryStatus.ADDED) {
Review Comment:
It depends on how we'll use this. Note, that this guard has been there
before this PR, so I haven't changed anything here. What changed is that before
ADDED triggered inheriting both data and file seq nums, now ADDED triggers
inheriting file seq num, while ADDED or MODIFIED triggers inheriting data seq
num.
If we want to scratch file seq num, I'd do that separately to this PR,
because I wouldn't introduce unrelated changes into this one.
##########
core/src/test/java/org/apache/iceberg/TestTrackingBuilder.java:
##########
@@ -272,15 +287,69 @@ void manifestDVPositionsProduceModified() {
assertThat(modified.deletedPositions()).isEqualTo(deletedBytes);
}
+ @Test
+ void manifestPositionsWithColumnFilesUpdated() {
+ ByteBuffer deletedBytes = ByteBuffer.wrap(new byte[] {1});
+ Tracking withDeletedPositions =
+ TrackingBuilder.from(manifestSourceTracking(), 999L)
+ .columnFilesUpdated()
+ .deletedPositions(deletedBytes)
+ .build();
+
+ assertThat(withDeletedPositions.status()).isEqualTo(EntryStatus.MODIFIED);
+
assertThat(withDeletedPositions.latestColumnFileSnapshotId()).isEqualTo(999L);
+ assertThat(withDeletedPositions.dvSnapshotId()).isEqualTo(999L);
+
assertThat(withDeletedPositions.deletedPositions()).isEqualTo(deletedBytes);
+
Review Comment:
Done
##########
core/src/test/java/org/apache/iceberg/TestTrackingBuilder.java:
##########
@@ -272,15 +287,69 @@ void manifestDVPositionsProduceModified() {
assertThat(modified.deletedPositions()).isEqualTo(deletedBytes);
}
+ @Test
+ void manifestPositionsWithColumnFilesUpdated() {
+ ByteBuffer deletedBytes = ByteBuffer.wrap(new byte[] {1});
+ Tracking withDeletedPositions =
+ TrackingBuilder.from(manifestSourceTracking(), 999L)
+ .columnFilesUpdated()
+ .deletedPositions(deletedBytes)
Review Comment:
I don't think technically we want to avoid providing deleted/replaced
positions together with column files. I just wanted to pin this down with a
test.
Giving this some further thought, I think you're right: Such a `Tracking`
that has these positions is an entry in the root manifest pointing to a leaf
manifest. I don't think we plan to add column files for leaf manifest at this
point, but it seems too strict to reject such a setting.
Could such a test remain? WDYT @stevenzwu ?
##########
core/src/test/java/org/apache/iceberg/TestTrackingBuilder.java:
##########
@@ -272,15 +287,69 @@ void manifestDVPositionsProduceModified() {
assertThat(modified.deletedPositions()).isEqualTo(deletedBytes);
}
+ @Test
+ void manifestPositionsWithColumnFilesUpdated() {
+ ByteBuffer deletedBytes = ByteBuffer.wrap(new byte[] {1});
+ Tracking withDeletedPositions =
+ TrackingBuilder.from(manifestSourceTracking(), 999L)
+ .columnFilesUpdated()
+ .deletedPositions(deletedBytes)
+ .build();
+
+ assertThat(withDeletedPositions.status()).isEqualTo(EntryStatus.MODIFIED);
+
assertThat(withDeletedPositions.latestColumnFileSnapshotId()).isEqualTo(999L);
+ assertThat(withDeletedPositions.dvSnapshotId()).isEqualTo(999L);
+
assertThat(withDeletedPositions.deletedPositions()).isEqualTo(deletedBytes);
+
+ ByteBuffer replacedBytes = ByteBuffer.wrap(new byte[] {2});
+ Tracking withReplacedPositions =
+ TrackingBuilder.from(manifestSourceTracking(), 999L)
+ .columnFilesUpdated()
+ .replacedPositions(replacedBytes)
+ .build();
+
+ assertThat(withReplacedPositions.status()).isEqualTo(EntryStatus.MODIFIED);
+
assertThat(withReplacedPositions.latestColumnFileSnapshotId()).isEqualTo(999L);
+ assertThat(withReplacedPositions.dvSnapshotId()).isEqualTo(999L);
+
assertThat(withReplacedPositions.replacedPositions()).isEqualTo(replacedBytes);
+ }
+
+ @Test
+ void columnFilesUpdatedWithManifestPositions() {
Review Comment:
Here I wanted to test the opposite order of adding deleted positions and
column files. Earlier I did have some extra checks around that code, but I
don't see the point to have this coverage now. Removed this test.
##########
core/src/test/java/org/apache/iceberg/TestColumnFileStruct.java:
##########
@@ -0,0 +1,280 @@
+/*
+ * 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 static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.util.List;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.MethodSource;
+
+class TestColumnFileStruct {
+
+ private static final int FORMAT_VERSION = 4;
+ private static final List<Integer> FIELD_IDS = Lists.newArrayList(1, 2, 3);
+ private static final String LOCATION = "s3://bucket/data/column.parquet";
+ private static final FileFormat FILE_FORMAT = FileFormat.PARQUET;
+ private static final long FILE_SIZE_IN_BYTES = 1024L;
+ private static final ByteBuffer KEY_METADATA = ByteBuffer.wrap(new byte[]
{1, 2, 3});
+ private static final List<Long> SPLIT_OFFSETS = Lists.newArrayList(0L, 512L);
+
+ @Test
+ void fieldAccess() {
+ ColumnFile columnFile =
+ new ColumnFileStruct(
+ FORMAT_VERSION,
+ FIELD_IDS,
+ LOCATION,
+ FILE_FORMAT,
+ FILE_SIZE_IN_BYTES,
+ KEY_METADATA,
+ SPLIT_OFFSETS);
+
+ assertThat(columnFile.formatVersion()).isEqualTo(FORMAT_VERSION);
+ assertThat(columnFile.fieldIds()).containsExactlyElementsOf(FIELD_IDS);
+ assertThat(columnFile.location()).isEqualTo(LOCATION);
+ assertThat(columnFile.fileFormat()).isEqualTo(FILE_FORMAT);
+ assertThat(columnFile.fileSizeInBytes()).isEqualTo(FILE_SIZE_IN_BYTES);
+ assertThat(columnFile.keyMetadata()).isEqualTo(KEY_METADATA);
+
assertThat(columnFile.splitOffsets()).containsExactlyElementsOf(SPLIT_OFFSETS);
+ }
+
+ @Test
+ void copy() {
+ ColumnFile columnFile =
+ new ColumnFileStruct(
+ FORMAT_VERSION,
+ FIELD_IDS,
+ LOCATION,
+ FILE_FORMAT,
+ FILE_SIZE_IN_BYTES,
+ KEY_METADATA,
+ SPLIT_OFFSETS);
+
+ ColumnFile copy = columnFile.copy();
+
+ assertThat(copy.formatVersion()).isEqualTo(FORMAT_VERSION);
+ assertThat(copy.fieldIds()).containsExactlyElementsOf(FIELD_IDS);
+ assertThat(copy.location()).isEqualTo(LOCATION);
+ assertThat(copy.fileFormat()).isEqualTo(FILE_FORMAT);
+ assertThat(copy.fileSizeInBytes()).isEqualTo(FILE_SIZE_IN_BYTES);
+ assertThat(copy.keyMetadata()).isEqualTo(KEY_METADATA);
+ assertThat(copy.splitOffsets()).containsExactlyElementsOf(SPLIT_OFFSETS);
+ }
+
+ @Test
+ void structLikeSize() {
+ ColumnFileStruct columnFile = new ColumnFileStruct();
+ assertThat(columnFile.size()).isEqualTo(7);
+ }
+
+ @Test
+ void setFieldsByOrdinals() {
+ ColumnFileStruct columnFile = new ColumnFileStruct();
+
+ columnFile.set(0, FORMAT_VERSION);
+ columnFile.set(1, FIELD_IDS);
+ columnFile.set(2, LOCATION);
+ columnFile.set(3, FILE_FORMAT.toString());
+ columnFile.set(4, FILE_SIZE_IN_BYTES);
+ columnFile.set(5, KEY_METADATA);
+ columnFile.set(6, SPLIT_OFFSETS);
+
+ assertThat(columnFile.formatVersion()).isEqualTo(FORMAT_VERSION);
+ assertThat(columnFile.fieldIds()).containsExactlyElementsOf(FIELD_IDS);
+ assertThat(columnFile.location()).isEqualTo(LOCATION);
+ assertThat(columnFile.fileFormat()).isEqualTo(FILE_FORMAT);
+ assertThat(columnFile.fileSizeInBytes()).isEqualTo(FILE_SIZE_IN_BYTES);
+ assertThat(columnFile.keyMetadata()).isEqualTo(KEY_METADATA);
+
assertThat(columnFile.splitOffsets()).containsExactlyElementsOf(SPLIT_OFFSETS);
+ }
+
+ @Test
+ void getFieldsByOrdinals() {
+ ColumnFileStruct columnFile =
+ new ColumnFileStruct(
+ FORMAT_VERSION,
+ FIELD_IDS,
+ LOCATION,
+ FILE_FORMAT,
+ FILE_SIZE_IN_BYTES,
+ KEY_METADATA,
+ SPLIT_OFFSETS);
+
+ assertThat(columnFile.get(0, Integer.class)).isEqualTo(FORMAT_VERSION);
+ assertThat(columnFile.get(1,
List.class)).containsExactlyElementsOf(FIELD_IDS);
+ assertThat(columnFile.get(2, String.class)).isEqualTo(LOCATION);
+ assertThat(columnFile.get(3,
String.class)).isEqualTo(FILE_FORMAT.toString());
+ assertThat(columnFile.get(4, Long.class)).isEqualTo(FILE_SIZE_IN_BYTES);
+ assertThat(columnFile.get(5, ByteBuffer.class)).isEqualTo(KEY_METADATA);
+ assertThat(columnFile.get(6,
List.class)).containsExactlyElementsOf(SPLIT_OFFSETS);
+ }
+
+ @Test
+ void projectedStructLike() {
+ Types.StructType projection =
+ Types.StructType.of(ColumnFile.LOCATION,
ColumnFile.FILE_SIZE_IN_BYTES);
+
+ ColumnFileStruct columnFile = new ColumnFileStruct(projection);
+ assertThat(columnFile.size()).isEqualTo(2);
+
+ // projected position 0 maps to internal position of location
+ // projected position 1 maps to internal position of file_size_in_bytes
+ columnFile.set(0, LOCATION);
+ columnFile.set(1, 1024L);
Review Comment:
You're right. Replaced with the field. Found other occurrence, did there the
replace there too.
--
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]
