stevenzwu commented on code in PR #16285:
URL: https://github.com/apache/iceberg/pull/16285#discussion_r4255026175
##########
core/src/main/java/org/apache/iceberg/TrackedFileStruct.java:
##########
@@ -152,6 +159,14 @@ private TrackedFileStruct(TrackedFileStruct toCopy,
Set<Integer> statsIds) {
toCopy.splitOffsets != null
? Arrays.copyOf(toCopy.splitOffsets, toCopy.splitOffsets.length)
: null;
+ if (toCopy.columnFiles != null) {
+ this.columnFiles =
Lists.newArrayListWithCapacity(toCopy.columnFiles.size());
+ for (ColumnFile columnFile : toCopy.columnFiles) {
+ this.columnFiles.add(columnFile != null ? columnFile.copy() : null);
Review Comment:
the list element shouldn't be null, right?
##########
core/src/main/java/org/apache/iceberg/ColumnFile.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * 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.types.Types;
+
+interface ColumnFile {
+ Types.NestedField LOCATION =
+ Types.NestedField.required(
+ 160, "location", Types.StringType.get(), "Location of the column
file");
+ Types.NestedField FIELD_IDS =
+ Types.NestedField.required(
+ 161,
+ "field_ids",
+ Types.ListType.ofRequired(162, Types.IntegerType.get()),
+ "Live field IDs in this column file");
+ Types.NestedField FILE_FORMAT =
+ Types.NestedField.required(
+ 163,
+ "file_format",
+ Types.StringType.get(),
+ "String file format name for this column file");
Review Comment:
[#16025](https://github.com/apache/iceberg/pull/16025) documents this as
"String file format name: avro, orc, or parquet", which is also the doc on
`TrackedFile.FILE_FORMAT`.
##########
core/src/test/java/org/apache/iceberg/TestColumnFileStruct.java:
##########
@@ -0,0 +1,210 @@
+/*
+ * 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.Comparators;
+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 String LOCATION = "s3://bucket/data/column.parquet";
+ private static final List<Integer> FIELD_IDS = Lists.newArrayList(1, 2, 3);
+ 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});
+
+ @Test
+ void fieldAccess() {
+ ColumnFile columnFile =
+ new ColumnFileStruct(LOCATION, FIELD_IDS, FILE_FORMAT,
FILE_SIZE_IN_BYTES, KEY_METADATA);
+
+ assertThat(columnFile.location()).isEqualTo(LOCATION);
+ assertThat(columnFile.fieldIds()).containsExactlyElementsOf(FIELD_IDS);
+ assertThat(columnFile.fileFormat()).isEqualTo(FILE_FORMAT);
+ assertThat(columnFile.fileSizeInBytes()).isEqualTo(FILE_SIZE_IN_BYTES);
+ assertThat(columnFile.keyMetadata()).isEqualTo(KEY_METADATA);
+ }
+
+ @Test
+ void copy() {
+ ColumnFile columnFile =
+ new ColumnFileStruct(LOCATION, FIELD_IDS, FILE_FORMAT,
FILE_SIZE_IN_BYTES, KEY_METADATA);
+
+ ColumnFile copy = columnFile.copy();
+
+ assertThat(copy.location()).isEqualTo(LOCATION);
Review Comment:
use `Comparators.forType(Types.StructType)` to simplify the assertions.
##########
core/src/main/java/org/apache/iceberg/TrackingBuilder.java:
##########
@@ -115,6 +118,19 @@ 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;
+ }
+ // Reset to null to inherit from the new snapshot sequence number. It is
safe to bump up the
+ // dataSequenceNumber as writers are required to rewrite v2 equality and
position deletes to DVs
+ // when applying column update.
+ this.dataSequenceNumber = null;
Review Comment:
The null reset matches the inheritance rule in
[#16025](https://github.com/apache/iceberg/pull/16025): the data sequence
number is inherited for ADDED, and for MODIFIED when a column file is added.
This comment still says writers must rewrite v2 equality and position
deletes into DVs before a column update. The spec PR does not state that
requirement. Moving the data sequence number forward without it lets those
deletes stop applying.
--
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]
