Guosmilesmile commented on code in PR #17635:
URL: https://github.com/apache/iceberg/pull/17635#discussion_r3810220231
##########
spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/data/TestVectorizedOrcDataReader.java:
##########
@@ -164,4 +172,66 @@ public void testWithFilterWithSelected() throws
IOException {
assertThat(rows).isExhausted();
}
}
+
+ @Test
+ public void testRowLineage() throws IOException {
+ Schema dataSchema = new Schema(Types.NestedField.required(1, "id",
Types.LongType.get()));
+ Schema writeSchema = MetadataColumns.schemaWithRowLineage(dataSchema);
+
+ GenericRecord record = GenericRecord.create(writeSchema);
+ ImmutableList.Builder<Record> builder = ImmutableList.builder();
+ for (int i = 0; i < 4; i++) {
Review Comment:
We currently only test within a batch. Could we add a test case where
batchOffsetInFile > 0 to cover the firstRowId + batchOffsetInFile + rowId logic?
##########
spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/data/vectorized/VectorizedSparkOrcReaders.java:
##########
@@ -488,5 +568,15 @@ public ColumnVector getChild(int ordinal) {
}
};
}
+
+ private static Map<Integer, Integer>
buildFieldIdToOrcIndex(TypeDescription orcType) {
Review Comment:
Seem the same in
orc/src/main/java/org/apache/iceberg/orc/OrcValueReaders.java . Can we extract
common part into a shared helper？
##########
spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/data/TestVectorizedOrcDataReader.java:
##########
@@ -164,4 +172,66 @@ public void testWithFilterWithSelected() throws
IOException {
assertThat(rows).isExhausted();
}
}
+
+ @Test
+ public void testRowLineage() throws IOException {
Review Comment:
We should avoid using test prefixes for newly added tests.
##########
spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/data/TestVectorizedOrcDataReader.java:
##########
@@ -164,4 +172,66 @@ public void testWithFilterWithSelected() throws
IOException {
assertThat(rows).isExhausted();
}
}
+
+ @Test
+ public void testRowLineage() throws IOException {
Review Comment:
May be package private？
##########
spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/data/vectorized/VectorizedSparkOrcReaders.java:
##########
@@ -432,16 +435,85 @@ public ColumnarMap getMap(int rowId) {
private static class StructConverter implements Converter {
private final Types.StructType structType;
+ private final Map<Integer, Integer> fieldIdToOrcIndex;
private final List<Converter> fieldConverters;
private final Map<Integer, ?> idToConstant;
private StructConverter(
Types.StructType structType,
+ TypeDescription orcType,
List<Converter> fieldConverters,
Map<Integer, ?> idToConstant) {
this.structType = structType;
+ this.fieldIdToOrcIndex = buildFieldIdToOrcIndex(orcType);
this.fieldConverters = fieldConverters;
this.idToConstant = idToConstant;
+
+ Preconditions.checkState(
+ fieldConverters.size() <= orcType.getChildren().size(),
+ "Invalid ORC schema: %s converters for %s columns",
+ fieldConverters.size(),
+ orcType.getChildren().size());
+ }
+
+ private ColumnVector storedVector(
Review Comment:
Should these private methods be moved to the bottom, after the public
methods?
##########
spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/data/vectorized/LastUpdatedSeqColumnVector.java:
##########
@@ -0,0 +1,126 @@
+/*
+ * 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.spark.data.vectorized;
+
+import org.apache.iceberg.spark.SparkSchemaUtil;
+import org.apache.iceberg.types.Types;
+import org.apache.spark.sql.types.Decimal;
+import org.apache.spark.sql.vectorized.ColumnVector;
+import org.apache.spark.sql.vectorized.ColumnarArray;
+import org.apache.spark.sql.vectorized.ColumnarMap;
+import org.apache.spark.unsafe.types.UTF8String;
+
+public class LastUpdatedSeqColumnVector extends ColumnVector {
Review Comment:
There’s quite a bit of duplicated code between `RowIdColumnVector` and
`LastUpdatedSeqColumnVector`. Could we extract the common logic into a base
class?
--
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]
