You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "RussellSpitzer (via GitHub)" <gi...@apache.org> on 2025/09/09 21:54:34 UTC
[PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
RussellSpitzer opened a new pull request, #14040:
URL: https://github.com/apache/iceberg/pull/14040
Adds support to InternalReader Parquet for Support custom types.
New StructLike Reader added to ParquetValueReaders which can optionally be
used by BaseParquetReaders and InternalReader to produce any struct like object
with a constructor.
Tests for this are added to a new suite in Core because the existing tests are currently
split between TestInternalAvro and TestInternalParquet but I can move those if necessary.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2334910315
##########
core/src/test/java/org/apache/iceberg/TestInternalData.java:
##########
@@ -0,0 +1,247 @@
+/*
+ * 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 java.io.IOException;
+import java.nio.file.Path;
+import java.util.Arrays;
+import java.util.List;
+import org.apache.iceberg.data.GenericRecord;
+import org.apache.iceberg.data.Record;
+import org.apache.iceberg.io.CloseableIterable;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.io.FileIO;
+import org.apache.iceberg.io.InputFile;
+import org.apache.iceberg.io.OutputFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.io.TempDir;
+
+@ExtendWith(ParameterizedTestExtension.class)
+public class TestInternalData {
Review Comment:
Currently we have some tests for these code paths in TestInternalAvro and TestInternalParquet but those suites mostly function by piping everything through a "writeAndValidate" method which is not conducive to adding new functionality. It currently is used to test reuse containers and projections but I think we may want to consider separating those tests out as well into tests that run in core if possible.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2356839499
##########
parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java:
##########
@@ -1161,6 +1160,61 @@ public static class ReadBuilder implements InternalData.ReadBuilder {
private NameMapping nameMapping = null;
private ByteBuffer fileEncryptionKey = null;
private ByteBuffer fileAADPrefix = null;
+ private Class<? extends StructLike> rootType = null;
+ private Map<Integer, Class<? extends StructLike>> customTypes = Maps.newHashMap();
+
+ public interface ReaderFunction {
Review Comment:
This is my replacement for "readerFunc and readerFuncWithSchema" which we can continue using if we want to add more parameters to the readerFunc without adding additional private fields.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2371794254
##########
core/src/test/java/org/apache/iceberg/TestInternalData.java:
##########
@@ -0,0 +1,158 @@
+/*
+ * 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 java.io.IOException;
+import java.nio.file.Path;
+import java.util.Arrays;
+import java.util.List;
+import org.apache.iceberg.data.Record;
+import org.apache.iceberg.io.CloseableIterable;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.io.FileIO;
+import org.apache.iceberg.io.InputFile;
+import org.apache.iceberg.io.OutputFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.io.TempDir;
+
+@ExtendWith(ParameterizedTestExtension.class)
+public class TestInternalData {
+
+ @Parameter(index = 0)
+ private FileFormat format;
+
+ @Parameters(name = " format = {0}")
+ protected static List<Object> parameters() {
Review Comment:
```
@Parameters(name = " format = {0}")
protected static List<FileFormat> parameters() {
return Arrays.asList(FileFormat.AVRO, FileFormat.PARQUET);
}
```
this is slightly more readable and doesn't require the additional layer of Object[]
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "shangxinli (via GitHub)" <gi...@apache.org>.
shangxinli commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2373866109
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetValueReaders.java:
##########
@@ -1132,6 +1144,46 @@ private TripleIterator<?> firstNonNullColumn(List<TripleIterator<?>> columns) {
}
}
+ private static class StructLikeReader<T extends StructLike> extends StructReader<T, T> {
+ private final Types.StructType struct;
+ private final DynConstructors.Ctor<T> ctor;
+
+ StructLikeReader(
+ List<ParquetValueReader<?>> readers, Types.StructType struct, Class<T> structLikeClass) {
+ super(readers);
+ this.struct = struct;
+ this.ctor =
Review Comment:
DynConstructors.build() returns a constructor wrapper even if no matching constructors are found. The failure only occurs later when ctor.newInstance(struct) is called in newStructData() method (line 1167), potentially causing confusing runtime errors instead of clear validation errors during reader setup.
Can we do?
try {
this.ctor = DynConstructors.builder(StructLike.class)
.hiddenImpl(structLikeClass, Types.StructType.class)
.hiddenImpl(structLikeClass)
.build();
// Validate constructor works by testing instantiation
ctor.newInstance(struct);
} catch (Exception e) {
throw new IllegalArgumentException(
"Custom type " + structLikeClass.getName() +
" must have a constructor accepting Types.StructType or a no-args constructor", e);
}
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2338001999
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/InternalReader.java:
##########
@@ -30,27 +31,45 @@
public class InternalReader<T extends StructLike> extends BaseParquetReaders<T> {
+ private Class<? extends StructLike> rootType = Record.class;
+ private Map<Integer, Class<? extends StructLike>> typesById = new java.util.HashMap<>();
Review Comment:
nit: use `Maps.newHashMap()`?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#issuecomment-3272440080
I'm interested if anyone has an alternate way of wiring this together, I'm not extremely happy with the current setup where Parquet.java has so many different ways of attaching functions or mappings.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2336883696
##########
parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java:
##########
@@ -1281,12 +1304,24 @@ public ReadBuilder withNameMapping(NameMapping newNameMapping) {
@Override
public ReadBuilder setRootType(Class<? extends StructLike> rootClass) {
- throw new UnsupportedOperationException("Custom types are not yet supported");
+ Preconditions.checkArgument(
+ this.internalReader != null, "Cannot set root type without using an Internal Reader");
+ Preconditions.checkArgument(
+ this.readerFunc == null && this.readerFuncWithSchema == null,
+ "Setting root type is not compatible with setting a reader function");
+ internalReader.setRootType(rootClass);
Review Comment:
Why not store the rootClass, and the custom types in the Parquet.ReadBuilder directly?
If we do so, we might not need the whole internal reader redirections.
Also, this is the way how this is solved in Avro
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2359548074
##########
parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java:
##########
@@ -1220,35 +1274,43 @@ public ReadBuilder createReaderFunc(
this.batchedReaderFunc == null,
"Cannot set reader function: batched reader function already set");
Preconditions.checkArgument(
- this.readerFuncWithSchema == null,
- "Cannot set reader function: 2-argument reader function already set");
- this.readerFunc = newReaderFunction;
+ this.readerFunction == null, "Cannot set reader function: reader function already set");
+ this.readerFunction = new UnaryReaderFunction(newReaderFunction);
return this;
}
public ReadBuilder createReaderFunc(
BiFunction<Schema, MessageType, ParquetValueReader<?>> newReaderFunction) {
- Preconditions.checkArgument(
- this.readerFunc == null,
- "Cannot set 2-argument reader function: reader function already set");
Preconditions.checkArgument(
this.batchedReaderFunc == null,
- "Cannot set 2-argument reader function: batched reader function already set");
- this.readerFuncWithSchema = newReaderFunction;
+ "Cannot set reader function: batched reader function already set");
+ Preconditions.checkArgument(
+ this.readerFunction == null, "Cannot set reader function: reader function already set");
+ this.readerFunction = new BinaryReaderFunction(newReaderFunction);
return this;
}
public ReadBuilder createBatchedReaderFunc(Function<MessageType, VectorizedReader<?>> func) {
Preconditions.checkArgument(
- this.readerFunc == null,
- "Cannot set batched reader function: reader function already set");
+ this.batchedReaderFunc == null,
+ "Cannot set batched reader function: batched reader function already set");
Preconditions.checkArgument(
- this.readerFuncWithSchema == null,
- "Cannot set batched reader function: 2-argument reader function already set");
+ this.readerFunction == null,
+ "Cannot set batched reader function: ReaderFunction already set");
this.batchedReaderFunc = func;
return this;
}
+ public ReadBuilder createReaderFunc(ReaderFunction reader) {
Review Comment:
Shall we deprecate the other reader function methods?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2356604062
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/BaseParquetReaders.java:
##########
@@ -75,6 +78,16 @@ protected ParquetValueReader<T> createReader(
}
}
+ /**
+ * This method can be overridden to provide a custom implementation which also uses the fieldId of
+ * the Schema when creating the struct reader
+ */
+ protected ParquetValueReader<T> createStructReader(
Review Comment:
The complaint is that while we are source compatible it "may" be binary incompatible
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2363284301
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/BaseParquetReaders.java:
##########
@@ -75,8 +78,26 @@ protected ParquetValueReader<T> createReader(
}
}
- protected abstract ParquetValueReader<T> createStructReader(
- List<ParquetValueReader<?>> fieldReaders, Types.StructType structType);
+ /**
+ * This method can be overridden to provide a custom implementation which also uses the fieldId of
+ * the Schema when creating the struct reader
+ */
+ protected ParquetValueReader<T> createStructReader(
+ List<ParquetValueReader<?>> fieldReaders, Types.StructType structType, Integer fieldId) {
+ // Fallback to the signature without fieldId if not overridden
+ return createStructReader(fieldReaders, structType);
+ }
+
+ /**
+ * @deprecated will be removed in 1.11.0. Subclasses should override {@link
+ * #createStructReader(List, Types.StructType, Integer)} instead
+ */
+ @Deprecated
+ protected ParquetValueReader<T> createStructReader(
Review Comment:
With release of 1.10.0, should this method be removed in 1.12.0?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2371739946
##########
.palantir/revapi.yml:
##########
@@ -1333,6 +1333,44 @@ acceptedBreaks:
\ java.util.List<org.apache.iceberg.PositionDeletesScanTask>)"
justification: "Removing deprecations for 1.10.0"
org.apache.iceberg:iceberg-parquet:
+ - code: "java.method.parameterTypeChanged"
Review Comment:
I don't think any of these RevAPI changes are actually needed anymore. I've removed them locally and things passed
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2371748551
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/BaseParquetReaders.java:
##########
@@ -75,8 +78,26 @@ protected ParquetValueReader<T> createReader(
}
}
- protected abstract ParquetValueReader<T> createStructReader(
- List<ParquetValueReader<?>> fieldReaders, Types.StructType structType);
+ /**
+ * This method can be overridden to provide a custom implementation which also uses the fieldId of
+ * the Schema when creating the struct reader
+ */
+ protected ParquetValueReader<T> createStructReader(
+ List<ParquetValueReader<?>> fieldReaders, Types.StructType structType, Integer fieldId) {
+ // Fallback to the signature without fieldId if not overridden
+ return createStructReader(fieldReaders, structType);
+ }
+
+ /**
+ * @deprecated will be removed in 1.11.0. Subclasses should override {@link
+ * #createStructReader(List, Types.StructType, Integer)} instead
+ */
+ @Deprecated
+ protected ParquetValueReader<T> createStructReader(
Review Comment:
nit: might want to move this method to where it originally was to reduce the diff
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2356840817
##########
parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java:
##########
@@ -1161,6 +1160,61 @@ public static class ReadBuilder implements InternalData.ReadBuilder {
private NameMapping nameMapping = null;
private ByteBuffer fileEncryptionKey = null;
private ByteBuffer fileAADPrefix = null;
+ private Class<? extends StructLike> rootType = null;
+ private Map<Integer, Class<? extends StructLike>> customTypes = Maps.newHashMap();
+
+ public interface ReaderFunction {
+ Function<MessageType, ParquetValueReader<?>> apply();
+
+ default ReaderFunction withRootType(Class<? extends StructLike> rootType) {
+ return this;
+ }
+
+ default ReaderFunction withCustomTypes(
+ Map<Integer, Class<? extends StructLike>> customTypes) {
+ return this;
+ }
+
+ default ReaderFunction withSchema(Schema schema) {
+ return this;
+ }
+ }
+
+ private static class UnaryReaderFunction implements ReaderFunction {
Review Comment:
This is an instance of the original readerFunc which takes only messageType as an arg.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2361186348
##########
core/src/test/java/org/apache/iceberg/TestInternalData.java:
##########
@@ -0,0 +1,157 @@
+/*
+ * 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 java.io.IOException;
+import java.nio.file.Path;
+import java.util.Arrays;
+import java.util.List;
+import org.apache.iceberg.data.Record;
+import org.apache.iceberg.io.CloseableIterable;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.io.FileIO;
+import org.apache.iceberg.io.InputFile;
+import org.apache.iceberg.io.OutputFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.io.TempDir;
+
+@ExtendWith(ParameterizedTestExtension.class)
+public class TestInternalData {
+
+ @Parameter(index = 0)
+ private FileFormat format;
+
+ @Parameters(name = " format = {0}")
+ protected static List<Object> parameters() {
+ return Arrays.asList(new Object[] {FileFormat.AVRO}, new Object[] {FileFormat.PARQUET});
+ }
+
+ private static final Schema SIMPLE_SCHEMA =
+ new Schema(
+ Types.NestedField.required(1, "id", Types.LongType.get()),
+ Types.NestedField.optional(2, "name", Types.StringType.get()));
+
+ private static final Schema NESTED_SCHEMA =
+ new Schema(
+ Types.NestedField.required(1, "outer_id", Types.LongType.get()),
+ Types.NestedField.optional(
+ 2,
+ "nested_struct",
+ Types.StructType.of(
+ Types.NestedField.optional(3, "inner_id", Types.LongType.get()),
+ Types.NestedField.optional(4, "inner_name", Types.StringType.get()))));
+
+ @TempDir private Path tempDir;
+
+ private final FileIO fileIO = new TestTables.LocalFileIO();
+
Review Comment:
got it. resolving the thread
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2371795434
##########
core/src/test/java/org/apache/iceberg/TestInternalData.java:
##########
@@ -0,0 +1,158 @@
+/*
+ * 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 java.io.IOException;
+import java.nio.file.Path;
+import java.util.Arrays;
+import java.util.List;
+import org.apache.iceberg.data.Record;
+import org.apache.iceberg.io.CloseableIterable;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.io.FileIO;
+import org.apache.iceberg.io.InputFile;
+import org.apache.iceberg.io.OutputFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.io.TempDir;
+
+@ExtendWith(ParameterizedTestExtension.class)
+public class TestInternalData {
+
+ @Parameter(index = 0)
+ private FileFormat format;
+
+ @Parameters(name = " format = {0}")
+ protected static List<Object> parameters() {
+ return Arrays.asList(new Object[] {FileFormat.AVRO}, new Object[] {FileFormat.PARQUET});
+ }
+
+ private static final Schema SIMPLE_SCHEMA =
+ new Schema(
+ Types.NestedField.required(1, "id", Types.LongType.get()),
+ Types.NestedField.optional(2, "name", Types.StringType.get()));
+
+ private static final Schema NESTED_SCHEMA =
+ new Schema(
+ Types.NestedField.required(1, "outer_id", Types.LongType.get()),
+ Types.NestedField.optional(
+ 2,
+ "nested_struct",
+ Types.StructType.of(
+ Types.NestedField.optional(3, "inner_id", Types.LongType.get()),
+ Types.NestedField.optional(4, "inner_name", Types.StringType.get()))));
+
+ @TempDir private Path tempDir;
+
+ private final FileIO fileIO = new TestTables.LocalFileIO();
+
+ @TestTemplate
+ public void testCustomRootType() throws IOException {
+ OutputFile outputFile = fileIO.newOutputFile(tempDir.resolve("test." + format).toString());
+
+ List<Record> testData = RandomInternalData.generate(SIMPLE_SCHEMA, 1000, 1L);
+
+ try (FileAppender<Record> appender =
+ InternalData.write(format, outputFile).schema(SIMPLE_SCHEMA).build()) {
+ appender.addAll(testData);
+ }
+
+ InputFile inputFile = fileIO.newInputFile(outputFile.location());
+ List<PartitionData> readRecords = Lists.newArrayList();
+
+ try (CloseableIterable<PartitionData> reader =
+ InternalData.read(format, inputFile)
+ .project(SIMPLE_SCHEMA)
+ .setRootType(PartitionData.class)
+ .build()) {
+ for (PartitionData record : reader) {
+ readRecords.add(record);
+ }
+ }
+
+ assertThat(readRecords).hasSize(testData.size());
Review Comment:
```suggestion
assertThat(readRecords).hasSameSizeAs(testData);
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2373094205
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/BaseParquetReaders.java:
##########
@@ -75,8 +78,26 @@ protected ParquetValueReader<T> createReader(
}
}
- protected abstract ParquetValueReader<T> createStructReader(
- List<ParquetValueReader<?>> fieldReaders, Types.StructType structType);
+ /**
+ * This method can be overridden to provide a custom implementation which also uses the fieldId of
+ * the Schema when creating the struct reader
+ */
+ protected ParquetValueReader<T> createStructReader(
+ List<ParquetValueReader<?>> fieldReaders, Types.StructType structType, Integer fieldId) {
+ // Fallback to the signature without fieldId if not overridden
+ return createStructReader(fieldReaders, structType);
+ }
+
+ /**
+ * @deprecated will be removed in 1.11.0. Subclasses should override {@link
+ * #createStructReader(List, Types.StructType, Integer)} instead
+ */
+ @Deprecated
+ protected ParquetValueReader<T> createStructReader(
+ List<ParquetValueReader<?>> fieldReaders, Types.StructType structType) {
+ throw new UnsupportedOperationException(
Review Comment:
No, the method was "abstract" before so they would have had to have implemented this method. I made it concerete here so I could remove the implementations from the two child classes we currently have rather than marking it deprecated there as well.
If you check the 3 arg version, it calls the subclasses 2 arg version if not defined so existing behavior should be the same.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2373094205
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/BaseParquetReaders.java:
##########
@@ -75,8 +78,26 @@ protected ParquetValueReader<T> createReader(
}
}
- protected abstract ParquetValueReader<T> createStructReader(
- List<ParquetValueReader<?>> fieldReaders, Types.StructType structType);
+ /**
+ * This method can be overridden to provide a custom implementation which also uses the fieldId of
+ * the Schema when creating the struct reader
+ */
+ protected ParquetValueReader<T> createStructReader(
+ List<ParquetValueReader<?>> fieldReaders, Types.StructType structType, Integer fieldId) {
+ // Fallback to the signature without fieldId if not overridden
+ return createStructReader(fieldReaders, structType);
+ }
+
+ /**
+ * @deprecated will be removed in 1.11.0. Subclasses should override {@link
+ * #createStructReader(List, Types.StructType, Integer)} instead
+ */
+ @Deprecated
+ protected ParquetValueReader<T> createStructReader(
+ List<ParquetValueReader<?>> fieldReaders, Types.StructType structType) {
+ throw new UnsupportedOperationException(
Review Comment:
No, the method was "abstract" before so they would have had to have implemented this method. I made it concrete here so I could remove the implementations from the two child classes we currently have rather than marking it deprecated there as well.
If you check the 3 arg version, it calls the subclasses 2 arg version if not defined so existing behavior should be the same.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2334926982
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/InternalReader.java:
##########
@@ -30,27 +31,45 @@
public class InternalReader<T extends StructLike> extends BaseParquetReaders<T> {
Review Comment:
I've changed this into a stateful reader so that Parquet.java can set custom classes for the reader to use. I'm definitely open to other ways around this. The previous approach was for Parquet to statically used the create method to generate a reader for "readerFunc", this had the problem that each call to set custom types would happen after the reader func was set.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2334934635
##########
parquet/src/main/java/org/apache/iceberg/InternalParquet.java:
##########
@@ -37,6 +37,6 @@ private static Parquet.WriteBuilder writeInternal(OutputFile outputFile) {
}
private static Parquet.ReadBuilder readInternal(InputFile inputFile) {
- return Parquet.read(inputFile).createReaderFunc(InternalReader::create);
+ return Parquet.read(inputFile).useInternalReader(new InternalReader<>());
Review Comment:
We cannot create a reader function at this point because we don't know whether or not we will have custom classes to apply by future calls to returned object from this 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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2337196442
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/BaseParquetReaders.java:
##########
@@ -119,7 +133,8 @@ public ParquetValueReader<?> struct(
}
}
- return createStructReader(newFields, expected);
+ Integer id = struct.getId() != null ? struct.getId().intValue() : null;
Review Comment:
This is worth a separate method.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2338009166
##########
parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java:
##########
@@ -1281,12 +1304,24 @@ public ReadBuilder withNameMapping(NameMapping newNameMapping) {
@Override
public ReadBuilder setRootType(Class<? extends StructLike> rootClass) {
- throw new UnsupportedOperationException("Custom types are not yet supported");
+ Preconditions.checkArgument(
+ this.internalReader != null, "Cannot set root type without using an Internal Reader");
+ Preconditions.checkArgument(
+ this.readerFunc == null && this.readerFuncWithSchema == null,
+ "Setting root type is not compatible with setting a reader function");
+ internalReader.setRootType(rootClass);
+ return this;
}
@Override
public ReadBuilder setCustomType(int fieldId, Class<? extends StructLike> structClass) {
- throw new UnsupportedOperationException("Custom types are not yet supported");
+ Preconditions.checkArgument(
+ this.internalReader != null, "Cannot set a custom type without using an Internal Reader");
+ Preconditions.checkArgument(
+ this.readerFunc == null && this.readerFuncWithSchema == null,
+ "Setting root type is not compatible with setting a reader function");
Review Comment:
nit: typo? `Setting custom type`?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#issuecomment-3272662883
@RussellSpitzer is this still the uber PR for the Parquet manifest support?
https://github.com/apache/iceberg/pull/13769/
If yes, it will be great t0 rebase that PR on top of this one to help understand the integration.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2335424814
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetValueReaders.java:
##########
@@ -1132,6 +1144,46 @@ private TripleIterator<?> firstNonNullColumn(List<TripleIterator<?>> columns) {
}
}
+ private static class StructLikeReader<T extends StructLike> extends StructReader<T, T> {
+ private final Types.StructType struct;
+ private final DynConstructors.Ctor<T> ctor;
+
+ StructLikeReader(
+ List<ParquetValueReader<?>> readers, Types.StructType struct, Class<T> structLikeClass) {
+ super(readers);
+ this.struct = struct;
+ this.ctor =
+ DynConstructors.builder(StructLike.class)
+ .hiddenImpl(structLikeClass, Types.StructType.class)
+ .hiddenImpl(structLikeClass)
Review Comment:
should default/empty constructor be used here? I though we would require a struct type.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2335503954
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/InternalReader.java:
##########
@@ -72,4 +91,12 @@ protected ParquetValueReader<?> timeReader(ColumnDescriptor desc) {
protected ParquetValueReader<?> timestampReader(ColumnDescriptor desc, boolean isAdjustedToUTC) {
return ParquetValueReaders.timestamps(desc);
}
+
+ public void setCustomType(int fieldId, Class<? extends StructLike> type) {
+ this.typesById.put(fieldId, type);
+ }
+
+ public void setRootType(Class<? extends StructLike> rootClass) {
+ setCustomType(ROOT_ID, rootClass);
Review Comment:
do we need the `ROOT_ID`? why not just
```
this.rootType = rootClass;
```
##########
parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java:
##########
@@ -1245,10 +1252,26 @@ public ReadBuilder createBatchedReaderFunc(Function<MessageType, VectorizedReade
Preconditions.checkArgument(
this.readerFuncWithSchema == null,
"Cannot set batched reader function: 2-argument reader function already set");
+ Preconditions.checkArgument(
+ this.internalReader == null,
+ "Cannot set batched reader function: internal reader already set");
this.batchedReaderFunc = func;
return this;
}
+ public ReadBuilder useInternalReader(InternalReader<?> reader) {
+ Preconditions.checkArgument(
+ this.readerFunc == null, "Cannot set internal reader: reader function already set");
+ Preconditions.checkArgument(
+ this.readerFuncWithSchema == null,
+ "Cannot set internal reader: 2-argument reader function already set");
+ Preconditions.checkArgument(
+ this.internalReader == null, "Cannot set internal reader: internal reader already set");
Review Comment:
I think you meant `batchedReaderFunc` here
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/InternalReader.java:
##########
@@ -72,4 +91,12 @@ protected ParquetValueReader<?> timeReader(ColumnDescriptor desc) {
protected ParquetValueReader<?> timestampReader(ColumnDescriptor desc, boolean isAdjustedToUTC) {
return ParquetValueReaders.timestamps(desc);
}
+
+ public void setCustomType(int fieldId, Class<? extends StructLike> type) {
Review Comment:
have this class implements the `SupportsCustomTypes` interface like Avro InternalReader
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2353766565
##########
parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java:
##########
@@ -1281,12 +1304,24 @@ public ReadBuilder withNameMapping(NameMapping newNameMapping) {
@Override
public ReadBuilder setRootType(Class<? extends StructLike> rootClass) {
- throw new UnsupportedOperationException("Custom types are not yet supported");
+ Preconditions.checkArgument(
+ this.internalReader != null, "Cannot set root type without using an Internal Reader");
+ Preconditions.checkArgument(
+ this.readerFunc == null && this.readerFuncWithSchema == null,
+ "Setting root type is not compatible with setting a reader function");
+ internalReader.setRootType(rootClass);
Review Comment:
Sorry, I followed what you were saying I was just wondering if we shouldn't be making an interface but making a ReaderFunctionFactory class itself
```
class ParquetReaderFunction {
Function<Message, ParquetValueReaderBuilder> build()
ParquetReaderFunction withSchema(Schema schema) {
// Overload if we are a readerFuncWithSchema
return this
}
ParquetReaderFunction withCustomTypes<Map<Integer, Class<T extends StructLike> {
// Overload if we are using CustomTypes
return this
}
}
```
This is a bigger overall refactor, but I'm wondering if we can replace the currently 3 (will be 4 after this) setters we currently have with just 2
ParquetReaderFunction
And
VectorizedReaderFunction
And we rely on implementations to decide whether they do something with withSchema or withCustomTypes\
We could be doing this with "InternalReader" as an example of this class and avoid making createReader and ReaderWithSchema and ReaderWithSchemaAndType
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2356620992
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/BaseParquetReaders.java:
##########
@@ -119,7 +133,8 @@ public ParquetValueReader<?> struct(
}
}
- return createStructReader(newFields, expected);
+ Integer id = struct.getId() != null ? struct.getId().intValue() : null;
Review Comment:
Good call, let me extract that
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2356649727
##########
parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java:
##########
@@ -1245,10 +1252,26 @@ public ReadBuilder createBatchedReaderFunc(Function<MessageType, VectorizedReade
Preconditions.checkArgument(
this.readerFuncWithSchema == null,
"Cannot set batched reader function: 2-argument reader function already set");
+ Preconditions.checkArgument(
+ this.internalReader == null,
+ "Cannot set batched reader function: internal reader already set");
this.batchedReaderFunc = func;
return this;
}
+ public ReadBuilder useInternalReader(InternalReader<?> reader) {
+ Preconditions.checkArgument(
+ this.readerFunc == null, "Cannot set internal reader: reader function already set");
+ Preconditions.checkArgument(
+ this.readerFuncWithSchema == null,
+ "Cannot set internal reader: 2-argument reader function already set");
+ Preconditions.checkArgument(
+ this.internalReader == null, "Cannot set internal reader: internal reader already set");
Review Comment:
Fixed, also changed this structure so now there is only a ReaderFunction and a batchedReaderFunc so we don't have all these preconditions. In the future I think we probably should reduce the interface again so there is only a single field ReaderFunction and we parameterize it some how so we can still get this to work. Like
```
ReaderFunction<T> and
ParquetReaderFunction extends ReaderFunction<ParquetValueReader>
BatchedReaderFunction extends ReaderFunction<VectorizedReader>
```
Or something like that?
We can talk about that later 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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2356661819
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetValueReaders.java:
##########
@@ -1132,6 +1144,46 @@ private TripleIterator<?> firstNonNullColumn(List<TripleIterator<?>> columns) {
}
}
+ private static class StructLikeReader<T extends StructLike> extends StructReader<T, T> {
+ private final Types.StructType struct;
+ private final DynConstructors.Ctor<T> ctor;
+
+ StructLikeReader(
+ List<ParquetValueReader<?>> readers, Types.StructType struct, Class<T> structLikeClass) {
+ super(readers);
+ this.struct = struct;
+ this.ctor =
+ DynConstructors.builder(StructLike.class)
+ .hiddenImpl(structLikeClass, Types.StructType.class)
+ .hiddenImpl(structLikeClass)
Review Comment:
I'm mimicking the Avro code here although we can't actually have empty structs here here ...
https://github.com/apache/iceberg/blob/7eef46da0d00edb79b20d24cb7806f6ea8da4000/core/src/main/java/org/apache/iceberg/avro/InternalReaders.java#L55-L60
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2356627705
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/InternalReader.java:
##########
@@ -72,4 +91,12 @@ protected ParquetValueReader<?> timeReader(ColumnDescriptor desc) {
protected ParquetValueReader<?> timestampReader(ColumnDescriptor desc, boolean isAdjustedToUTC) {
return ParquetValueReaders.timestamps(desc);
}
+
+ public void setCustomType(int fieldId, Class<? extends StructLike> type) {
Review Comment:
I've taken Peter's suggestion and moved this back out into the Parquet class, and removed public access to the constructor here.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2360025639
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/InternalReader.java:
##########
@@ -20,37 +20,87 @@
import java.util.List;
import java.util.Map;
+import java.util.function.Function;
import org.apache.iceberg.Schema;
import org.apache.iceberg.StructLike;
+import org.apache.iceberg.data.Record;
+import org.apache.iceberg.parquet.Parquet;
import org.apache.iceberg.parquet.ParquetValueReader;
import org.apache.iceberg.parquet.ParquetValueReaders;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
import org.apache.iceberg.types.Types.StructType;
import org.apache.parquet.column.ColumnDescriptor;
import org.apache.parquet.schema.MessageType;
public class InternalReader<T extends StructLike> extends BaseParquetReaders<T> {
+ private final Map<Integer, Class<? extends StructLike>> typesById = Maps.newHashMap();
+
private static final InternalReader<?> INSTANCE = new InternalReader<>();
private InternalReader() {}
@SuppressWarnings("unchecked")
public static <T extends StructLike> ParquetValueReader<T> create(
- Schema expectedSchema, MessageType fileSchema) {
- return (ParquetValueReader<T>) INSTANCE.createReader(expectedSchema, fileSchema);
+ Schema expectedSchema, MessageType fileSchema, Map<Integer, ?> idToConstant) {
+ return (ParquetValueReader<T>) INSTANCE.createReader(expectedSchema, fileSchema, idToConstant);
}
@SuppressWarnings("unchecked")
public static <T extends StructLike> ParquetValueReader<T> create(
- Schema expectedSchema, MessageType fileSchema, Map<Integer, ?> idToConstant) {
- return (ParquetValueReader<T>) INSTANCE.createReader(expectedSchema, fileSchema, idToConstant);
+ Schema expectedSchema, MessageType fileSchema) {
+ return (ParquetValueReader<T>) INSTANCE.createReader(expectedSchema, fileSchema);
+ }
+
+ public static Parquet.ReadBuilder.ReaderFunction readerFunction() {
+ InternalReader<?> reader = new InternalReader<>();
+
+ return new Parquet.ReadBuilder.ReaderFunction() {
+ private Schema schema;
+
+ @Override
+ public Function<MessageType, ParquetValueReader<?>> apply() {
+ return messageType -> reader.createReader(schema, messageType);
+ }
+
+ @Override
+ public Parquet.ReadBuilder.ReaderFunction withSchema(Schema schema) {
+ this.schema = schema;
+ return this;
+ }
+
+ @Override
+ public Parquet.ReadBuilder.ReaderFunction withCustomTypes(
+ Map<Integer, Class<? extends StructLike>> customTypes) {
+ reader.typesById.putAll(customTypes);
+ return this;
+ }
+
+ @Override
+ public Parquet.ReadBuilder.ReaderFunction withRootType(Class<? extends StructLike> rootType) {
+ if (rootType != null) {
+ reader.typesById.put(ROOT_ID, rootType);
+ }
+ return this;
+ }
+ };
}
@Override
- @SuppressWarnings("unchecked")
protected ParquetValueReader<T> createStructReader(
List<ParquetValueReader<?>> fieldReaders, StructType structType) {
- return (ParquetValueReader<T>) ParquetValueReaders.recordReader(fieldReaders, structType);
+ throw new UnsupportedOperationException(
Review Comment:
Why not remove this function?
If not removing, maybe we should deprecate it at least
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2371747334
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/BaseParquetReaders.java:
##########
@@ -75,6 +78,16 @@ protected ParquetValueReader<T> createReader(
}
}
+ /**
+ * This method can be overridden to provide a custom implementation which also uses the fieldId of
+ * the Schema when creating the struct reader
+ */
+ protected ParquetValueReader<T> createStructReader(
Review Comment:
I tested this locally and RevAPI shouldn't be complaining anymore, meaning that the RevAPI changes can be reverted
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2336700233
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/InternalReader.java:
##########
@@ -72,4 +91,12 @@ protected ParquetValueReader<?> timeReader(ColumnDescriptor desc) {
protected ParquetValueReader<?> timestampReader(ColumnDescriptor desc, boolean isAdjustedToUTC) {
return ParquetValueReaders.timestamps(desc);
}
+
+ public void setCustomType(int fieldId, Class<? extends StructLike> type) {
+ this.typesById.put(fieldId, type);
Review Comment:
nit: Are we using `this` in these cases?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2339561722
##########
parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java:
##########
@@ -1281,12 +1304,24 @@ public ReadBuilder withNameMapping(NameMapping newNameMapping) {
@Override
public ReadBuilder setRootType(Class<? extends StructLike> rootClass) {
- throw new UnsupportedOperationException("Custom types are not yet supported");
+ Preconditions.checkArgument(
+ this.internalReader != null, "Cannot set root type without using an Internal Reader");
+ Preconditions.checkArgument(
+ this.readerFunc == null && this.readerFuncWithSchema == null,
+ "Setting root type is not compatible with setting a reader function");
+ internalReader.setRootType(rootClass);
Review Comment:
How about this solution: https://github.com/apache/iceberg/pull/14049/commits/01afda90b99f6fa31bc9a3bb6d9b42e6722a3bfd?
Basically this suggest to store the types map in the `Parquet.ReadBuilder`, and create a new `readerFuncWithTypes` which have a ` Map<Integer, Class<? extends StructLike>>` input parameter
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2356833601
##########
api/src/test/java/org/apache/iceberg/TestHelpers.java:
##########
@@ -390,6 +391,14 @@ public static CustomRow of(Object... values) {
return new CustomRow(values);
}
Review Comment:
Constructors so we can use Custom Row as a CustomType in internal Data
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2359548074
##########
parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java:
##########
@@ -1220,35 +1274,43 @@ public ReadBuilder createReaderFunc(
this.batchedReaderFunc == null,
"Cannot set reader function: batched reader function already set");
Preconditions.checkArgument(
- this.readerFuncWithSchema == null,
- "Cannot set reader function: 2-argument reader function already set");
- this.readerFunc = newReaderFunction;
+ this.readerFunction == null, "Cannot set reader function: reader function already set");
+ this.readerFunction = new UnaryReaderFunction(newReaderFunction);
return this;
}
public ReadBuilder createReaderFunc(
BiFunction<Schema, MessageType, ParquetValueReader<?>> newReaderFunction) {
- Preconditions.checkArgument(
- this.readerFunc == null,
- "Cannot set 2-argument reader function: reader function already set");
Preconditions.checkArgument(
this.batchedReaderFunc == null,
- "Cannot set 2-argument reader function: batched reader function already set");
- this.readerFuncWithSchema = newReaderFunction;
+ "Cannot set reader function: batched reader function already set");
+ Preconditions.checkArgument(
+ this.readerFunction == null, "Cannot set reader function: reader function already set");
+ this.readerFunction = new BinaryReaderFunction(newReaderFunction);
return this;
}
public ReadBuilder createBatchedReaderFunc(Function<MessageType, VectorizedReader<?>> func) {
Preconditions.checkArgument(
- this.readerFunc == null,
- "Cannot set batched reader function: reader function already set");
+ this.batchedReaderFunc == null,
+ "Cannot set batched reader function: batched reader function already set");
Preconditions.checkArgument(
- this.readerFuncWithSchema == null,
- "Cannot set batched reader function: 2-argument reader function already set");
+ this.readerFunction == null,
+ "Cannot set batched reader function: ReaderFunction already set");
this.batchedReaderFunc = func;
return this;
}
+ public ReadBuilder createReaderFunc(ReaderFunction reader) {
Review Comment:
Shall we deprecate the other reader function methods?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2359647446
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/InternalReader.java:
##########
@@ -20,37 +20,87 @@
import java.util.List;
import java.util.Map;
+import java.util.function.Function;
import org.apache.iceberg.Schema;
import org.apache.iceberg.StructLike;
+import org.apache.iceberg.data.Record;
+import org.apache.iceberg.parquet.Parquet;
import org.apache.iceberg.parquet.ParquetValueReader;
import org.apache.iceberg.parquet.ParquetValueReaders;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
import org.apache.iceberg.types.Types.StructType;
import org.apache.parquet.column.ColumnDescriptor;
import org.apache.parquet.schema.MessageType;
public class InternalReader<T extends StructLike> extends BaseParquetReaders<T> {
+ private final Map<Integer, Class<? extends StructLike>> typesById = Maps.newHashMap();
+
private static final InternalReader<?> INSTANCE = new InternalReader<>();
private InternalReader() {}
@SuppressWarnings("unchecked")
public static <T extends StructLike> ParquetValueReader<T> create(
- Schema expectedSchema, MessageType fileSchema) {
- return (ParquetValueReader<T>) INSTANCE.createReader(expectedSchema, fileSchema);
+ Schema expectedSchema, MessageType fileSchema, Map<Integer, ?> idToConstant) {
+ return (ParquetValueReader<T>) INSTANCE.createReader(expectedSchema, fileSchema, idToConstant);
}
@SuppressWarnings("unchecked")
public static <T extends StructLike> ParquetValueReader<T> create(
- Schema expectedSchema, MessageType fileSchema, Map<Integer, ?> idToConstant) {
- return (ParquetValueReader<T>) INSTANCE.createReader(expectedSchema, fileSchema, idToConstant);
+ Schema expectedSchema, MessageType fileSchema) {
+ return (ParquetValueReader<T>) INSTANCE.createReader(expectedSchema, fileSchema);
+ }
+
+ public static Parquet.ReadBuilder.ReaderFunction readerFunction() {
+ InternalReader<?> reader = new InternalReader<>();
+
+ return new Parquet.ReadBuilder.ReaderFunction() {
+ private Schema schema;
+
+ @Override
+ public Function<MessageType, ParquetValueReader<?>> apply() {
+ return messageType -> reader.createReader(schema, messageType);
+ }
+
+ @Override
+ public Parquet.ReadBuilder.ReaderFunction withSchema(Schema schema) {
+ this.schema = schema;
+ return this;
+ }
+
+ @Override
+ public Parquet.ReadBuilder.ReaderFunction withCustomTypes(
+ Map<Integer, Class<? extends StructLike>> customTypes) {
+ reader.typesById.putAll(customTypes);
+ return this;
+ }
+
+ @Override
+ public Parquet.ReadBuilder.ReaderFunction withRootType(Class<? extends StructLike> rootType) {
+ if (rootType != null) {
+ reader.typesById.put(ROOT_ID, rootType);
+ }
Review Comment:
nit: newline
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2359928975
##########
core/src/test/java/org/apache/iceberg/TestInternalData.java:
##########
@@ -0,0 +1,157 @@
+/*
+ * 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 java.io.IOException;
+import java.nio.file.Path;
+import java.util.Arrays;
+import java.util.List;
+import org.apache.iceberg.data.Record;
+import org.apache.iceberg.io.CloseableIterable;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.io.FileIO;
+import org.apache.iceberg.io.InputFile;
+import org.apache.iceberg.io.OutputFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.io.TempDir;
+
+@ExtendWith(ParameterizedTestExtension.class)
+public class TestInternalData {
+
+ @Parameter(index = 0)
+ private FileFormat format;
+
+ @Parameters(name = " format = {0}")
+ protected static List<Object> parameters() {
+ return Arrays.asList(new Object[] {FileFormat.AVRO}, new Object[] {FileFormat.PARQUET});
+ }
+
+ private static final Schema SIMPLE_SCHEMA =
+ new Schema(
+ Types.NestedField.required(1, "id", Types.LongType.get()),
+ Types.NestedField.optional(2, "name", Types.StringType.get()));
+
+ private static final Schema NESTED_SCHEMA =
+ new Schema(
+ Types.NestedField.required(1, "outer_id", Types.LongType.get()),
+ Types.NestedField.optional(
+ 2,
+ "nested_struct",
+ Types.StructType.of(
+ Types.NestedField.optional(3, "inner_id", Types.LongType.get()),
+ Types.NestedField.optional(4, "inner_name", Types.StringType.get()))));
+
+ @TempDir private Path tempDir;
+
+ private final FileIO fileIO = new TestTables.LocalFileIO();
+
Review Comment:
We could, this is where things are a little weird, we have a TestInternalParquet test that extends some of the other parquet tests. It uses WriteAndValidate as the extension point which covers Record reading and writing, re-use of containers and projection. I couldn't figure out a good way to add in custom type mapping there at the same time, it's also looking a bit messy. That said, we have very good coverage on the Record usage in that file.
https://github.com/apache/iceberg/blob/main/parquet/src/test/java/org/apache/iceberg/parquet/TestInternalParquet.java#L76-L120
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2360168576
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/InternalReader.java:
##########
@@ -20,37 +20,87 @@
import java.util.List;
import java.util.Map;
+import java.util.function.Function;
import org.apache.iceberg.Schema;
import org.apache.iceberg.StructLike;
+import org.apache.iceberg.data.Record;
+import org.apache.iceberg.parquet.Parquet;
import org.apache.iceberg.parquet.ParquetValueReader;
import org.apache.iceberg.parquet.ParquetValueReaders;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
import org.apache.iceberg.types.Types.StructType;
import org.apache.parquet.column.ColumnDescriptor;
import org.apache.parquet.schema.MessageType;
public class InternalReader<T extends StructLike> extends BaseParquetReaders<T> {
+ private final Map<Integer, Class<? extends StructLike>> typesById = Maps.newHashMap();
+
private static final InternalReader<?> INSTANCE = new InternalReader<>();
private InternalReader() {}
@SuppressWarnings("unchecked")
public static <T extends StructLike> ParquetValueReader<T> create(
- Schema expectedSchema, MessageType fileSchema) {
- return (ParquetValueReader<T>) INSTANCE.createReader(expectedSchema, fileSchema);
+ Schema expectedSchema, MessageType fileSchema, Map<Integer, ?> idToConstant) {
+ return (ParquetValueReader<T>) INSTANCE.createReader(expectedSchema, fileSchema, idToConstant);
}
@SuppressWarnings("unchecked")
public static <T extends StructLike> ParquetValueReader<T> create(
- Schema expectedSchema, MessageType fileSchema, Map<Integer, ?> idToConstant) {
- return (ParquetValueReader<T>) INSTANCE.createReader(expectedSchema, fileSchema, idToConstant);
+ Schema expectedSchema, MessageType fileSchema) {
+ return (ParquetValueReader<T>) INSTANCE.createReader(expectedSchema, fileSchema);
+ }
+
+ public static Parquet.ReadBuilder.ReaderFunction readerFunction() {
+ InternalReader<?> reader = new InternalReader<>();
+
+ return new Parquet.ReadBuilder.ReaderFunction() {
+ private Schema schema;
+
+ @Override
+ public Function<MessageType, ParquetValueReader<?>> apply() {
+ return messageType -> reader.createReader(schema, messageType);
+ }
+
+ @Override
+ public Parquet.ReadBuilder.ReaderFunction withSchema(Schema schema) {
+ this.schema = schema;
+ return this;
+ }
+
+ @Override
+ public Parquet.ReadBuilder.ReaderFunction withCustomTypes(
+ Map<Integer, Class<? extends StructLike>> customTypes) {
+ reader.typesById.putAll(customTypes);
+ return this;
+ }
+
+ @Override
+ public Parquet.ReadBuilder.ReaderFunction withRootType(Class<? extends StructLike> rootType) {
+ if (rootType != null) {
+ reader.typesById.put(ROOT_ID, rootType);
+ }
+ return this;
+ }
+ };
}
@Override
- @SuppressWarnings("unchecked")
protected ParquetValueReader<T> createStructReader(
List<ParquetValueReader<?>> fieldReaders, StructType structType) {
- return (ParquetValueReader<T>) ParquetValueReaders.recordReader(fieldReaders, structType);
+ throw new UnsupportedOperationException(
Review Comment:
You know what, I think maybe we should just pull this completely.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2371756709
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/BaseParquetReaders.java:
##########
@@ -75,8 +78,26 @@ protected ParquetValueReader<T> createReader(
}
}
- protected abstract ParquetValueReader<T> createStructReader(
- List<ParquetValueReader<?>> fieldReaders, Types.StructType structType);
+ /**
+ * This method can be overridden to provide a custom implementation which also uses the fieldId of
+ * the Schema when creating the struct reader
+ */
+ protected ParquetValueReader<T> createStructReader(
+ List<ParquetValueReader<?>> fieldReaders, Types.StructType structType, Integer fieldId) {
+ // Fallback to the signature without fieldId if not overridden
+ return createStructReader(fieldReaders, structType);
+ }
+
+ /**
+ * @deprecated will be removed in 1.11.0. Subclasses should override {@link
+ * #createStructReader(List, Types.StructType, Integer)} instead
+ */
+ @Deprecated
+ protected ParquetValueReader<T> createStructReader(
+ List<ParquetValueReader<?>> fieldReaders, Types.StructType structType) {
+ throw new UnsupportedOperationException(
Review Comment:
I'm not sure we can actually throw here, isn't that a behavioral change for when people upgrade their Iceberg version and they were in some place using `createStructReader(fieldReaders, structType)`?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2373126717
##########
core/src/test/java/org/apache/iceberg/TestInternalData.java:
##########
@@ -0,0 +1,158 @@
+/*
+ * 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 java.io.IOException;
+import java.nio.file.Path;
+import java.util.Arrays;
+import java.util.List;
+import org.apache.iceberg.data.Record;
+import org.apache.iceberg.io.CloseableIterable;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.io.FileIO;
+import org.apache.iceberg.io.InputFile;
+import org.apache.iceberg.io.OutputFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.io.TempDir;
+
+@ExtendWith(ParameterizedTestExtension.class)
+public class TestInternalData {
+
+ @Parameter(index = 0)
+ private FileFormat format;
+
+ @Parameters(name = " format = {0}")
+ protected static List<Object> parameters() {
Review Comment:
Thanks I always forget what is actually allowed with this annotation
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "gaborkaszab (via GitHub)" <gi...@apache.org>.
gaborkaszab commented on PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#issuecomment-3390629297
Hey @stevenzwu ,
Let me cc you as well on this. See my above comment. I'd appreciate any early feedback, and whether you think this makes sense. Thank you!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2335144289
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/BaseParquetReaders.java:
##########
@@ -75,6 +78,16 @@ protected ParquetValueReader<T> createReader(
}
}
+ /**
+ * This method can be overridden to provide a custom implementation which also uses the fieldId of
+ * the Schema when creating the struct reader
+ */
+ protected ParquetValueReader<T> createStructReader(
Review Comment:
A little weird that revapi complains about API change for non-public 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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer merged PR #14040:
URL: https://github.com/apache/iceberg/pull/14040
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2384318600
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetValueReaders.java:
##########
@@ -1132,6 +1144,46 @@ private TripleIterator<?> firstNonNullColumn(List<TripleIterator<?>> columns) {
}
}
+ private static class StructLikeReader<T extends StructLike> extends StructReader<T, T> {
+ private final Types.StructType struct;
+ private final DynConstructors.Ctor<T> ctor;
+
+ StructLikeReader(
+ List<ParquetValueReader<?>> readers, Types.StructType struct, Class<T> structLikeClass) {
+ super(readers);
+ this.struct = struct;
+ this.ctor =
Review Comment:
I think you may be misreading the code for DynConstructors, if it fails to find a constructor it will fail see
```
@SuppressWarnings("unchecked")
public <C> Ctor<C> build() {
if (ctor != null) {
return (Ctor<C>) ctor;
}
throw buildRuntimeException(baseClass, problems);
}
```
I also think this is pretty deep in the code and if we do have an exception here a user probably won't be able to fix.
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetValueReaders.java:
##########
@@ -1132,6 +1144,46 @@ private TripleIterator<?> firstNonNullColumn(List<TripleIterator<?>> columns) {
}
}
+ private static class StructLikeReader<T extends StructLike> extends StructReader<T, T> {
+ private final Types.StructType struct;
+ private final DynConstructors.Ctor<T> ctor;
+
+ StructLikeReader(
+ List<ParquetValueReader<?>> readers, Types.StructType struct, Class<T> structLikeClass) {
+ super(readers);
+ this.struct = struct;
+ this.ctor =
Review Comment:
I think you may be misreading the code for DynConstructors, if it fails to find a constructor it will fail see
```
@SuppressWarnings("unchecked")
public <C> Ctor<C> build() {
if (ctor != null) {
return (Ctor<C>) ctor;
}
throw buildRuntimeException(baseClass, problems);
}
```
I also think this is pretty deep in the code and if we do have an exception here a user probably won't be able to fix it.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2338001336
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/BaseParquetReaders.java:
##########
@@ -53,6 +53,9 @@
import org.apache.parquet.schema.Type;
abstract class BaseParquetReaders<T> {
+ // Root ID is used for the top-level struct in the Parquet Schema
+ protected static final int ROOT_ID = 1;
Review Comment:
+1 to use `-1` instead
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2361038783
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/BaseParquetReaders.java:
##########
@@ -75,8 +78,26 @@ protected ParquetValueReader<T> createReader(
}
}
- protected abstract ParquetValueReader<T> createStructReader(
- List<ParquetValueReader<?>> fieldReaders, Types.StructType structType);
+ /**
+ * This method can be overridden to provide a custom implementation which also uses the fieldId of
+ * the Schema when creating the struct reader
+ */
+ protected ParquetValueReader<T> createStructReader(
+ List<ParquetValueReader<?>> fieldReaders, Types.StructType structType, Integer fieldId) {
+ // Fallback to the signature without fieldId if not overridden
+ return createStructReader(fieldReaders, structType);
+ }
+
+ /**
+ * @deprecated will be removed in 1.11.0. Subclasses should override {@link
+ * #createStructReader(List, Types.StructType, Integer)} instead
+ */
+ @Deprecated
+ protected ParquetValueReader<T> createStructReader(
Review Comment:
@pvary I made this Deprecated, no longer abstract, and removed it from the child classes GenericParquetReaders and InternalReader.
This should mean we are still source compatible if someone was extending it but we no longer have any usages in library code
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2359958260
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/InternalReader.java:
##########
@@ -20,37 +20,87 @@
import java.util.List;
import java.util.Map;
+import java.util.function.Function;
import org.apache.iceberg.Schema;
import org.apache.iceberg.StructLike;
+import org.apache.iceberg.data.Record;
+import org.apache.iceberg.parquet.Parquet;
import org.apache.iceberg.parquet.ParquetValueReader;
import org.apache.iceberg.parquet.ParquetValueReaders;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
import org.apache.iceberg.types.Types.StructType;
import org.apache.parquet.column.ColumnDescriptor;
import org.apache.parquet.schema.MessageType;
public class InternalReader<T extends StructLike> extends BaseParquetReaders<T> {
+ private final Map<Integer, Class<? extends StructLike>> typesById = Maps.newHashMap();
+
private static final InternalReader<?> INSTANCE = new InternalReader<>();
private InternalReader() {}
@SuppressWarnings("unchecked")
public static <T extends StructLike> ParquetValueReader<T> create(
- Schema expectedSchema, MessageType fileSchema) {
- return (ParquetValueReader<T>) INSTANCE.createReader(expectedSchema, fileSchema);
+ Schema expectedSchema, MessageType fileSchema, Map<Integer, ?> idToConstant) {
Review Comment:
Ah let me switch them back. I rewrote this a few dozen times trying to figure out the best way to fit this together
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2360008171
##########
parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java:
##########
@@ -1220,35 +1274,43 @@ public ReadBuilder createReaderFunc(
this.batchedReaderFunc == null,
"Cannot set reader function: batched reader function already set");
Preconditions.checkArgument(
- this.readerFuncWithSchema == null,
- "Cannot set reader function: 2-argument reader function already set");
- this.readerFunc = newReaderFunction;
+ this.readerFunction == null, "Cannot set reader function: reader function already set");
Review Comment:
Hmm, it is a behavior change. I could allow the old behavior and allow resetting if the class of the ReaderFunction matches the previous class but failing if it's different.
Ie:
If this.readerFunction.class == readerFunction.class
this.readerFunction = readerFunction
So it wouldn't be a huge deal, but I think it's a weird behavior but it is an established one... Maybe we fix it in 2.0
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2371762289
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/InternalReader.java:
##########
@@ -46,11 +52,48 @@ public static <T extends StructLike> ParquetValueReader<T> create(
return (ParquetValueReader<T>) INSTANCE.createReader(expectedSchema, fileSchema, idToConstant);
}
+ public static Parquet.ReadBuilder.ReaderFunction readerFunction() {
+ InternalReader<?> reader = new InternalReader<>();
+
+ return new Parquet.ReadBuilder.ReaderFunction() {
+ private Schema schema;
+
+ @Override
+ public Function<MessageType, ParquetValueReader<?>> apply() {
+ return messageType -> reader.createReader(schema, messageType);
+ }
+
+ @Override
+ public Parquet.ReadBuilder.ReaderFunction withSchema(Schema schema) {
+ this.schema = schema;
+ return this;
+ }
+
+ @Override
+ public Parquet.ReadBuilder.ReaderFunction withCustomTypes(
Review Comment:
I actually like this naming you have here better, but just wanted to point out that in the Avro reader we use `setCustomTypes` / `setRootType` / `setSchema`
EDIT: nvm, there isn't any direct relation between the Avro and Parquet internal readers, so having different naming here should be fine
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2373139462
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/InternalReader.java:
##########
@@ -46,11 +52,48 @@ public static <T extends StructLike> ParquetValueReader<T> create(
return (ParquetValueReader<T>) INSTANCE.createReader(expectedSchema, fileSchema, idToConstant);
}
+ public static Parquet.ReadBuilder.ReaderFunction readerFunction() {
+ InternalReader<?> reader = new InternalReader<>();
+
+ return new Parquet.ReadBuilder.ReaderFunction() {
+ private Schema schema;
+
+ @Override
+ public Function<MessageType, ParquetValueReader<?>> apply() {
+ return messageType -> reader.createReader(schema, messageType);
+ }
+
+ @Override
+ public Parquet.ReadBuilder.ReaderFunction withSchema(Schema schema) {
+ this.schema = schema;
+ return this;
+ }
+
+ @Override
+ public Parquet.ReadBuilder.ReaderFunction withCustomTypes(
Review Comment:
🤷 Yeah I'm not sure it's worth standardizing since this is only called by Parquet.java anyway
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "shangxinli (via GitHub)" <gi...@apache.org>.
shangxinli commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2373862105
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/BaseParquetReaders.java:
##########
@@ -53,6 +53,9 @@
import org.apache.parquet.schema.Type;
abstract class BaseParquetReaders<T> {
+ // Root ID is used for the top-level struct in the Parquet Schema
+ protected static final int ROOT_ID = -1;
Review Comment:
Magic number could use documentation
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2384318600
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetValueReaders.java:
##########
@@ -1132,6 +1144,46 @@ private TripleIterator<?> firstNonNullColumn(List<TripleIterator<?>> columns) {
}
}
+ private static class StructLikeReader<T extends StructLike> extends StructReader<T, T> {
+ private final Types.StructType struct;
+ private final DynConstructors.Ctor<T> ctor;
+
+ StructLikeReader(
+ List<ParquetValueReader<?>> readers, Types.StructType struct, Class<T> structLikeClass) {
+ super(readers);
+ this.struct = struct;
+ this.ctor =
Review Comment:
I think you may be misreading the code for DynConstructors, if it fails to find a constructor it will fail see
```
@SuppressWarnings("unchecked")
public <C> Ctor<C> build() {
if (ctor != null) {
return (Ctor<C>) ctor;
}
throw buildRuntimeException(baseClass, problems);
}
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2371796004
##########
core/src/test/java/org/apache/iceberg/TestInternalData.java:
##########
@@ -0,0 +1,158 @@
+/*
+ * 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 java.io.IOException;
+import java.nio.file.Path;
+import java.util.Arrays;
+import java.util.List;
+import org.apache.iceberg.data.Record;
+import org.apache.iceberg.io.CloseableIterable;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.io.FileIO;
+import org.apache.iceberg.io.InputFile;
+import org.apache.iceberg.io.OutputFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.io.TempDir;
+
+@ExtendWith(ParameterizedTestExtension.class)
+public class TestInternalData {
+
+ @Parameter(index = 0)
+ private FileFormat format;
+
+ @Parameters(name = " format = {0}")
+ protected static List<Object> parameters() {
+ return Arrays.asList(new Object[] {FileFormat.AVRO}, new Object[] {FileFormat.PARQUET});
+ }
+
+ private static final Schema SIMPLE_SCHEMA =
+ new Schema(
+ Types.NestedField.required(1, "id", Types.LongType.get()),
+ Types.NestedField.optional(2, "name", Types.StringType.get()));
+
+ private static final Schema NESTED_SCHEMA =
+ new Schema(
+ Types.NestedField.required(1, "outer_id", Types.LongType.get()),
+ Types.NestedField.optional(
+ 2,
+ "nested_struct",
+ Types.StructType.of(
+ Types.NestedField.optional(3, "inner_id", Types.LongType.get()),
+ Types.NestedField.optional(4, "inner_name", Types.StringType.get()))));
+
+ @TempDir private Path tempDir;
+
+ private final FileIO fileIO = new TestTables.LocalFileIO();
+
+ @TestTemplate
+ public void testCustomRootType() throws IOException {
+ OutputFile outputFile = fileIO.newOutputFile(tempDir.resolve("test." + format).toString());
+
+ List<Record> testData = RandomInternalData.generate(SIMPLE_SCHEMA, 1000, 1L);
+
+ try (FileAppender<Record> appender =
+ InternalData.write(format, outputFile).schema(SIMPLE_SCHEMA).build()) {
+ appender.addAll(testData);
+ }
+
+ InputFile inputFile = fileIO.newInputFile(outputFile.location());
+ List<PartitionData> readRecords = Lists.newArrayList();
+
+ try (CloseableIterable<PartitionData> reader =
+ InternalData.read(format, inputFile)
+ .project(SIMPLE_SCHEMA)
+ .setRootType(PartitionData.class)
+ .build()) {
+ for (PartitionData record : reader) {
+ readRecords.add(record);
+ }
+ }
+
+ assertThat(readRecords).hasSize(testData.size());
+
+ for (int i = 0; i < testData.size(); i++) {
+ Record expected = testData.get(i);
+ PartitionData actual = readRecords.get(i);
+
+ assertThat(actual.get(0, Long.class)).isEqualTo(expected.get(0, Long.class));
+ assertThat(actual.get(1, String.class)).isEqualTo(expected.get(1, String.class));
+ }
+ }
+
+ @TestTemplate
+ public void testCustomTypeForNestedField() throws IOException {
+ OutputFile outputFile = fileIO.newOutputFile(tempDir.resolve("test." + format).toString());
+
+ List<Record> testData = RandomInternalData.generate(NESTED_SCHEMA, 1000, 1L);
+
+ try (FileAppender<Record> appender =
+ InternalData.write(format, outputFile).schema(NESTED_SCHEMA).build()) {
+ appender.addAll(testData);
+ }
+
+ InputFile inputFile = fileIO.newInputFile(outputFile.location());
+ List<Record> readRecords = Lists.newArrayList();
+
+ try (CloseableIterable<Record> reader =
+ InternalData.read(format, inputFile)
+ .project(NESTED_SCHEMA)
+ .setCustomType(2, TestHelpers.CustomRow.class)
+ .build()) {
+ for (Record record : reader) {
+ readRecords.add(record);
+ }
+ }
+
+ assertThat(readRecords).hasSize(testData.size());
Review Comment:
```suggestion
assertThat(readRecords).hasSameSizeAs(testData);
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#issuecomment-3275603987
> @RussellSpitzer is this still the uber PR for the Parquet manifest support? #13769
>
> If yes, it will be great t0 rebase that PR on top of this one to help understand the integration.
This is a small part of the PR for manifest support. This is required to load the fields into specific structLike classes.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2334907095
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/BaseParquetReaders.java:
##########
@@ -75,6 +78,16 @@ protected ParquetValueReader<T> createReader(
}
}
+ /**
+ * This method can be overridden to provide a custom implementation which also uses the fieldId of
+ * the Schema when creating the struct reader
+ */
+ protected ParquetValueReader<T> createStructReader(
Review Comment:
This is key for allowing InternalParquet to use the field Id to lookup what structlike class should be used for a particular field. This also is the reason for the RevAPI breaks
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2344616108
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/BaseParquetReaders.java:
##########
@@ -53,6 +53,9 @@
import org.apache.parquet.schema.Type;
abstract class BaseParquetReaders<T> {
+ // Root ID is used for the top-level struct in the Parquet Schema
+ protected static final int ROOT_ID = 1;
Review Comment:
yep, copy paste error from the other pr
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2342075453
##########
parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java:
##########
@@ -1281,12 +1304,24 @@ public ReadBuilder withNameMapping(NameMapping newNameMapping) {
@Override
public ReadBuilder setRootType(Class<? extends StructLike> rootClass) {
- throw new UnsupportedOperationException("Custom types are not yet supported");
+ Preconditions.checkArgument(
+ this.internalReader != null, "Cannot set root type without using an Internal Reader");
+ Preconditions.checkArgument(
+ this.readerFunc == null && this.readerFuncWithSchema == null,
+ "Setting root type is not compatible with setting a reader function");
+ internalReader.setRootType(rootClass);
Review Comment:
I'm not sure that's materially that different than using the InternalReader itself except we have to define another new interface. I'm fine with going in that direction it just doesn't seem any simpler to me
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2356971417
##########
core/src/test/java/org/apache/iceberg/TestInternalData.java:
##########
@@ -0,0 +1,157 @@
+/*
+ * 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 java.io.IOException;
+import java.nio.file.Path;
+import java.util.Arrays;
+import java.util.List;
+import org.apache.iceberg.data.Record;
+import org.apache.iceberg.io.CloseableIterable;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.io.FileIO;
+import org.apache.iceberg.io.InputFile;
+import org.apache.iceberg.io.OutputFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.io.TempDir;
+
+@ExtendWith(ParameterizedTestExtension.class)
+public class TestInternalData {
+
+ @Parameter(index = 0)
+ private FileFormat format;
+
+ @Parameters(name = " format = {0}")
+ protected static List<Object> parameters() {
+ return Arrays.asList(new Object[] {FileFormat.AVRO}, new Object[] {FileFormat.PARQUET});
+ }
+
+ private static final Schema SIMPLE_SCHEMA =
+ new Schema(
+ Types.NestedField.required(1, "id", Types.LongType.get()),
+ Types.NestedField.optional(2, "name", Types.StringType.get()));
+
+ private static final Schema NESTED_SCHEMA =
+ new Schema(
+ Types.NestedField.required(1, "outer_id", Types.LongType.get()),
+ Types.NestedField.optional(
+ 2,
+ "nested_struct",
+ Types.StructType.of(
+ Types.NestedField.optional(3, "inner_id", Types.LongType.get()),
+ Types.NestedField.optional(4, "inner_name", Types.StringType.get()))));
+
+ @TempDir private Path tempDir;
+
+ private final FileIO fileIO = new TestTables.LocalFileIO();
+
+ @TestTemplate
+ public void testCustomRootType() throws IOException {
+ OutputFile outputFile = fileIO.newOutputFile(tempDir.resolve("test." + format).toString());
+
+ List<Record> testData = RandomInternalData.generate(SIMPLE_SCHEMA, 1000, 1L);
+
+ try (FileAppender<Record> appender =
+ InternalData.write(format, outputFile).schema(SIMPLE_SCHEMA).build()) {
+ appender.addAll(testData);
+ }
+
+ InputFile inputFile = fileIO.newInputFile(outputFile.location());
+ List<PartitionData> readRecords = Lists.newArrayList();
+
+ try (CloseableIterable<PartitionData> reader =
+ InternalData.read(format, inputFile)
+ .project(SIMPLE_SCHEMA)
+ .setRootType(PartitionData.class)
+ .build()) {
+ for (PartitionData record : reader) {
+ readRecords.add(record);
+ }
+ }
+
+ assertThat(readRecords).hasSize(testData.size());
+
+ for (int i = 0; i < testData.size(); i++) {
+ Record expected = testData.get(i);
+ PartitionData actual = readRecords.get(i);
+
+ assertThat(actual.get(0, Long.class)).isEqualTo(expected.get(0, Long.class));
+ assertThat(actual.get(1, String.class)).isEqualTo(expected.get(1, String.class));
+ }
+ }
+
+ @TestTemplate
+ public void testCustomTypeForNestedField() throws IOException {
+ OutputFile outputFile = fileIO.newOutputFile(tempDir.resolve("test." + format).toString());
+
+ List<Record> testData = RandomInternalData.generate(NESTED_SCHEMA, 1000, 1L);
+
+ try (FileAppender<Record> appender =
+ InternalData.write(format, outputFile).schema(NESTED_SCHEMA).build()) {
+ appender.addAll(testData);
+ }
+
+ InputFile inputFile = fileIO.newInputFile(outputFile.location());
+ List<Record> readRecords = Lists.newArrayList();
+
+ try (CloseableIterable<Record> reader =
+ InternalData.read(format, inputFile)
+ .project(NESTED_SCHEMA)
+ .setCustomType(2, TestHelpers.CustomRow.class)
+ .build()) {
+ for (Record record : reader) {
+ readRecords.add(record);
+ }
+ }
+
+ assertThat(readRecords).hasSize(testData.size());
+
+ for (int i = 0; i < testData.size(); i++) {
+ Record expected = testData.get(i);
+ Record actual = readRecords.get(i);
+
+ assertThat(actual.get(0, Long.class)).isEqualTo(expected.get(0, Long.class));
+
+ Object expectedNested = expected.get(1);
+ Object actualNested = actual.get(1);
+
+ if (expectedNested == null && actualNested == null) {
+ continue;
+ }
+
+ if (actualNested != null) {
+ assertThat(actualNested)
+ .as("Custom type should be TestCustomRow, but was: " + actualNested.getClass())
Review Comment:
nit: error msg for `TestCustomRow` should be updated `TestHelpers.CustomRow`
##########
parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java:
##########
@@ -1220,35 +1274,43 @@ public ReadBuilder createReaderFunc(
this.batchedReaderFunc == null,
"Cannot set reader function: batched reader function already set");
Preconditions.checkArgument(
- this.readerFuncWithSchema == null,
- "Cannot set reader function: 2-argument reader function already set");
- this.readerFunc = newReaderFunction;
+ this.readerFunction == null, "Cannot set reader function: reader function already set");
Review Comment:
there is a slight behavior change here. Previously, it is ok to set the unary reader function multiple times. With the combining unary and binary reader function, this is not allowed anymore. But I am ok with the slight behavior change, as I don't think it is necessary to allow set the same reader function type multiple times.
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/InternalReader.java:
##########
@@ -20,37 +20,87 @@
import java.util.List;
import java.util.Map;
+import java.util.function.Function;
import org.apache.iceberg.Schema;
import org.apache.iceberg.StructLike;
+import org.apache.iceberg.data.Record;
+import org.apache.iceberg.parquet.Parquet;
import org.apache.iceberg.parquet.ParquetValueReader;
import org.apache.iceberg.parquet.ParquetValueReaders;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
import org.apache.iceberg.types.Types.StructType;
import org.apache.parquet.column.ColumnDescriptor;
import org.apache.parquet.schema.MessageType;
public class InternalReader<T extends StructLike> extends BaseParquetReaders<T> {
+ private final Map<Integer, Class<? extends StructLike>> typesById = Maps.newHashMap();
+
private static final InternalReader<?> INSTANCE = new InternalReader<>();
private InternalReader() {}
@SuppressWarnings("unchecked")
public static <T extends StructLike> ParquetValueReader<T> create(
- Schema expectedSchema, MessageType fileSchema) {
- return (ParquetValueReader<T>) INSTANCE.createReader(expectedSchema, fileSchema);
+ Schema expectedSchema, MessageType fileSchema, Map<Integer, ?> idToConstant) {
Review Comment:
nit: it seems that we switched the order of these two public static factory methods
##########
parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java:
##########
@@ -1161,6 +1160,61 @@ public static class ReadBuilder implements InternalData.ReadBuilder {
private NameMapping nameMapping = null;
private ByteBuffer fileEncryptionKey = null;
private ByteBuffer fileAADPrefix = null;
+ private Class<? extends StructLike> rootType = null;
+ private Map<Integer, Class<? extends StructLike>> customTypes = Maps.newHashMap();
+
+ public interface ReaderFunction {
Review Comment:
I really like this new elegant design. much cleaner.
##########
core/src/test/java/org/apache/iceberg/TestInternalData.java:
##########
@@ -0,0 +1,157 @@
+/*
+ * 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 java.io.IOException;
+import java.nio.file.Path;
+import java.util.Arrays;
+import java.util.List;
+import org.apache.iceberg.data.Record;
+import org.apache.iceberg.io.CloseableIterable;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.io.FileIO;
+import org.apache.iceberg.io.InputFile;
+import org.apache.iceberg.io.OutputFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.io.TempDir;
+
+@ExtendWith(ParameterizedTestExtension.class)
+public class TestInternalData {
+
+ @Parameter(index = 0)
+ private FileFormat format;
+
+ @Parameters(name = " format = {0}")
+ protected static List<Object> parameters() {
+ return Arrays.asList(new Object[] {FileFormat.AVRO}, new Object[] {FileFormat.PARQUET});
+ }
+
+ private static final Schema SIMPLE_SCHEMA =
+ new Schema(
+ Types.NestedField.required(1, "id", Types.LongType.get()),
+ Types.NestedField.optional(2, "name", Types.StringType.get()));
+
+ private static final Schema NESTED_SCHEMA =
+ new Schema(
+ Types.NestedField.required(1, "outer_id", Types.LongType.get()),
+ Types.NestedField.optional(
+ 2,
+ "nested_struct",
+ Types.StructType.of(
+ Types.NestedField.optional(3, "inner_id", Types.LongType.get()),
+ Types.NestedField.optional(4, "inner_name", Types.StringType.get()))));
+
+ @TempDir private Path tempDir;
+
+ private final FileIO fileIO = new TestTables.LocalFileIO();
+
Review Comment:
should we add a test for default `Record` type?
##########
core/src/test/java/org/apache/iceberg/TestInternalData.java:
##########
@@ -0,0 +1,245 @@
+/*
+ * 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 java.io.IOException;
+import java.nio.file.Path;
+import java.util.Arrays;
+import java.util.List;
+import org.apache.iceberg.data.GenericRecord;
+import org.apache.iceberg.data.Record;
+import org.apache.iceberg.io.CloseableIterable;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.io.FileIO;
+import org.apache.iceberg.io.InputFile;
+import org.apache.iceberg.io.OutputFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.io.TempDir;
+
+@ExtendWith(ParameterizedTestExtension.class)
+public class TestInternalData {
+
+ @Parameter(index = 0)
+ private String format;
+
+ @Parameters(name = "format={0}")
+ public static Object[][] parameters() {
+ return new Object[][] {{"avro"}, {"parquet"}};
+ }
+
+ private static final Schema NESTED_SCHEMA =
+ new Schema(
+ Types.NestedField.required(1, "outer_id", Types.LongType.get()),
+ Types.NestedField.optional(
+ 2,
+ "nested_struct",
+ Types.StructType.of(
+ Types.NestedField.optional(3, "inner_id", Types.LongType.get()),
+ Types.NestedField.optional(4, "inner_name", Types.StringType.get()))));
+
+ @TempDir private Path tempDir;
+
+ private final FileIO fileIO = new TestTables.LocalFileIO();
+
+ @TestTemplate
+ public void testCustomRootType() throws IOException {
+ FileFormat fileFormat = FileFormat.fromString(format);
+ OutputFile outputFile = fileIO.newOutputFile(tempDir.resolve("test." + format).toString());
+
+ List<Record> testData = createSimpleTestRecords();
+
+ try (FileAppender<Record> appender =
+ InternalData.write(fileFormat, outputFile).schema(simpleSchema()).build()) {
+ appender.addAll(testData);
+ }
+
+ InputFile inputFile = fileIO.newInputFile(outputFile.location());
+ List<PartitionData> readRecords = Lists.newArrayList();
+
+ try (CloseableIterable<PartitionData> reader =
+ InternalData.read(fileFormat, inputFile)
+ .project(simpleSchema())
+ .setRootType(PartitionData.class)
+ .build()) {
+ for (PartitionData record : reader) {
+ readRecords.add(record);
+ }
+ }
+
+ assertThat(readRecords).hasSize(testData.size());
+
+ for (int i = 0; i < testData.size(); i++) {
+ Record expected = testData.get(i);
+ PartitionData actual = readRecords.get(i);
+
+ assertThat(actual.get(0, Long.class)).isEqualTo(expected.get(0, Long.class));
+ assertThat(actual.get(1, String.class)).isEqualTo(expected.get(1, String.class));
+ }
+ }
+
+ @TestTemplate
+ public void testCustomTypeForNestedField() throws IOException {
+ FileFormat fileFormat = FileFormat.fromString(format);
+ OutputFile outputFile = fileIO.newOutputFile(tempDir.resolve("test." + format).toString());
+
+ List<Record> testData = createNestedTestRecords();
+
+ try (FileAppender<Record> appender =
+ InternalData.write(fileFormat, outputFile).schema(NESTED_SCHEMA).build()) {
+ appender.addAll(testData);
+ }
+
+ InputFile inputFile = fileIO.newInputFile(outputFile.location());
+ List<Record> readRecords = Lists.newArrayList();
+
+ try (CloseableIterable<Record> reader =
+ InternalData.read(fileFormat, inputFile)
+ .project(NESTED_SCHEMA)
+ .setCustomType(2, TestCustomRow.class)
+ .build()) {
+ for (Record record : reader) {
+ readRecords.add(record);
+ }
+ }
+
+ assertThat(readRecords).hasSize(testData.size());
+
+ for (int i = 0; i < testData.size(); i++) {
+ Record expected = testData.get(i);
+ Record actual = readRecords.get(i);
+
+ assertThat(actual.get(0, Long.class)).isEqualTo(expected.get(0, Long.class));
+
+ Object expectedNested = expected.get(1);
+ Object actualNested = actual.get(1);
+
+ if (expectedNested == null && actualNested == null) {
+ continue;
+ }
+
+ if (actualNested != null) {
Review Comment:
maybe just replace line 139-141 with an assertion that either both are null or both are not null.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2360117010
##########
parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java:
##########
@@ -1220,35 +1274,43 @@ public ReadBuilder createReaderFunc(
this.batchedReaderFunc == null,
"Cannot set reader function: batched reader function already set");
Preconditions.checkArgument(
- this.readerFuncWithSchema == null,
- "Cannot set reader function: 2-argument reader function already set");
- this.readerFunc = newReaderFunction;
+ this.readerFunction == null, "Cannot set reader function: reader function already set");
Review Comment:
Maybe with the File Format API we can remove the possibility of setting the reader function from the external API.
That would naturally fix this.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2356532680
##########
core/src/test/java/org/apache/iceberg/TestInternalData.java:
##########
@@ -0,0 +1,245 @@
+/*
+ * 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 java.io.IOException;
+import java.nio.file.Path;
+import java.util.Arrays;
+import java.util.List;
+import org.apache.iceberg.data.GenericRecord;
+import org.apache.iceberg.data.Record;
+import org.apache.iceberg.io.CloseableIterable;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.io.FileIO;
+import org.apache.iceberg.io.InputFile;
+import org.apache.iceberg.io.OutputFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.io.TempDir;
+
+@ExtendWith(ParameterizedTestExtension.class)
+public class TestInternalData {
+
+ @Parameter(index = 0)
+ private String format;
+
+ @Parameters(name = "format={0}")
+ public static Object[][] parameters() {
+ return new Object[][] {{"avro"}, {"parquet"}};
+ }
+
+ private static final Schema NESTED_SCHEMA =
+ new Schema(
+ Types.NestedField.required(1, "outer_id", Types.LongType.get()),
+ Types.NestedField.optional(
+ 2,
+ "nested_struct",
+ Types.StructType.of(
+ Types.NestedField.optional(3, "inner_id", Types.LongType.get()),
+ Types.NestedField.optional(4, "inner_name", Types.StringType.get()))));
+
+ @TempDir private Path tempDir;
+
+ private final FileIO fileIO = new TestTables.LocalFileIO();
+
+ @TestTemplate
+ public void testCustomRootType() throws IOException {
+ FileFormat fileFormat = FileFormat.fromString(format);
+ OutputFile outputFile = fileIO.newOutputFile(tempDir.resolve("test." + format).toString());
+
+ List<Record> testData = createSimpleTestRecords();
+
+ try (FileAppender<Record> appender =
+ InternalData.write(fileFormat, outputFile).schema(simpleSchema()).build()) {
+ appender.addAll(testData);
+ }
+
+ InputFile inputFile = fileIO.newInputFile(outputFile.location());
+ List<PartitionData> readRecords = Lists.newArrayList();
+
+ try (CloseableIterable<PartitionData> reader =
+ InternalData.read(fileFormat, inputFile)
+ .project(simpleSchema())
+ .setRootType(PartitionData.class)
+ .build()) {
+ for (PartitionData record : reader) {
+ readRecords.add(record);
+ }
+ }
+
+ assertThat(readRecords).hasSize(testData.size());
+
+ for (int i = 0; i < testData.size(); i++) {
+ Record expected = testData.get(i);
+ PartitionData actual = readRecords.get(i);
+
+ assertThat(actual.get(0, Long.class)).isEqualTo(expected.get(0, Long.class));
+ assertThat(actual.get(1, String.class)).isEqualTo(expected.get(1, String.class));
+ }
+ }
+
+ @TestTemplate
+ public void testCustomTypeForNestedField() throws IOException {
+ FileFormat fileFormat = FileFormat.fromString(format);
+ OutputFile outputFile = fileIO.newOutputFile(tempDir.resolve("test." + format).toString());
+
+ List<Record> testData = createNestedTestRecords();
+
+ try (FileAppender<Record> appender =
+ InternalData.write(fileFormat, outputFile).schema(NESTED_SCHEMA).build()) {
+ appender.addAll(testData);
+ }
+
+ InputFile inputFile = fileIO.newInputFile(outputFile.location());
+ List<Record> readRecords = Lists.newArrayList();
+
+ try (CloseableIterable<Record> reader =
+ InternalData.read(fileFormat, inputFile)
+ .project(NESTED_SCHEMA)
+ .setCustomType(2, TestCustomRow.class)
+ .build()) {
+ for (Record record : reader) {
+ readRecords.add(record);
+ }
+ }
+
+ assertThat(readRecords).hasSize(testData.size());
+
+ for (int i = 0; i < testData.size(); i++) {
+ Record expected = testData.get(i);
+ Record actual = readRecords.get(i);
+
+ assertThat(actual.get(0, Long.class)).isEqualTo(expected.get(0, Long.class));
+
+ Object expectedNested = expected.get(1);
+ Object actualNested = actual.get(1);
+
+ if (expectedNested == null && actualNested == null) {
+ continue;
+ }
+
+ if (actualNested != null) {
Review Comment:
Currently the test does check that a null in the expected data is still handled correctly, so we are testing that case.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2356905298
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetValueReaders.java:
##########
@@ -1132,6 +1144,46 @@ private TripleIterator<?> firstNonNullColumn(List<TripleIterator<?>> columns) {
}
}
+ private static class StructLikeReader<T extends StructLike> extends StructReader<T, T> {
+ private final Types.StructType struct;
+ private final DynConstructors.Ctor<T> ctor;
+
+ StructLikeReader(
+ List<ParquetValueReader<?>> readers, Types.StructType struct, Class<T> structLikeClass) {
+ super(readers);
+ this.struct = struct;
+ this.ctor =
+ DynConstructors.builder(StructLike.class)
+ .hiddenImpl(structLikeClass, Types.StructType.class)
+ .hiddenImpl(structLikeClass)
Review Comment:
fair enough
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2359578242
##########
parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java:
##########
@@ -1161,6 +1160,61 @@ public static class ReadBuilder implements InternalData.ReadBuilder {
private NameMapping nameMapping = null;
private ByteBuffer fileEncryptionKey = null;
private ByteBuffer fileAADPrefix = null;
+ private Class<? extends StructLike> rootType = null;
+ private Map<Integer, Class<? extends StructLike>> customTypes = Maps.newHashMap();
+
+ public interface ReaderFunction {
Review Comment:
Cool... Looks nice.
Thanks @RussellSpitzer!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2356601959
##########
core/src/test/java/org/apache/iceberg/TestInternalData.java:
##########
@@ -0,0 +1,245 @@
+/*
+ * 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 java.io.IOException;
+import java.nio.file.Path;
+import java.util.Arrays;
+import java.util.List;
+import org.apache.iceberg.data.GenericRecord;
+import org.apache.iceberg.data.Record;
+import org.apache.iceberg.io.CloseableIterable;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.io.FileIO;
+import org.apache.iceberg.io.InputFile;
+import org.apache.iceberg.io.OutputFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.io.TempDir;
+
+@ExtendWith(ParameterizedTestExtension.class)
+public class TestInternalData {
+
+ @Parameter(index = 0)
+ private String format;
+
+ @Parameters(name = "format={0}")
+ public static Object[][] parameters() {
+ return new Object[][] {{"avro"}, {"parquet"}};
+ }
+
+ private static final Schema NESTED_SCHEMA =
+ new Schema(
+ Types.NestedField.required(1, "outer_id", Types.LongType.get()),
+ Types.NestedField.optional(
+ 2,
+ "nested_struct",
+ Types.StructType.of(
+ Types.NestedField.optional(3, "inner_id", Types.LongType.get()),
+ Types.NestedField.optional(4, "inner_name", Types.StringType.get()))));
+
+ @TempDir private Path tempDir;
+
+ private final FileIO fileIO = new TestTables.LocalFileIO();
+
+ @TestTemplate
+ public void testCustomRootType() throws IOException {
+ FileFormat fileFormat = FileFormat.fromString(format);
+ OutputFile outputFile = fileIO.newOutputFile(tempDir.resolve("test." + format).toString());
+
+ List<Record> testData = createSimpleTestRecords();
+
+ try (FileAppender<Record> appender =
+ InternalData.write(fileFormat, outputFile).schema(simpleSchema()).build()) {
+ appender.addAll(testData);
+ }
+
+ InputFile inputFile = fileIO.newInputFile(outputFile.location());
+ List<PartitionData> readRecords = Lists.newArrayList();
+
+ try (CloseableIterable<PartitionData> reader =
+ InternalData.read(fileFormat, inputFile)
+ .project(simpleSchema())
+ .setRootType(PartitionData.class)
+ .build()) {
+ for (PartitionData record : reader) {
+ readRecords.add(record);
+ }
+ }
+
+ assertThat(readRecords).hasSize(testData.size());
+
+ for (int i = 0; i < testData.size(); i++) {
+ Record expected = testData.get(i);
+ PartitionData actual = readRecords.get(i);
+
+ assertThat(actual.get(0, Long.class)).isEqualTo(expected.get(0, Long.class));
+ assertThat(actual.get(1, String.class)).isEqualTo(expected.get(1, String.class));
+ }
+ }
+
+ @TestTemplate
+ public void testCustomTypeForNestedField() throws IOException {
+ FileFormat fileFormat = FileFormat.fromString(format);
+ OutputFile outputFile = fileIO.newOutputFile(tempDir.resolve("test." + format).toString());
+
+ List<Record> testData = createNestedTestRecords();
+
+ try (FileAppender<Record> appender =
+ InternalData.write(fileFormat, outputFile).schema(NESTED_SCHEMA).build()) {
+ appender.addAll(testData);
+ }
+
+ InputFile inputFile = fileIO.newInputFile(outputFile.location());
+ List<Record> readRecords = Lists.newArrayList();
+
+ try (CloseableIterable<Record> reader =
+ InternalData.read(fileFormat, inputFile)
+ .project(NESTED_SCHEMA)
+ .setCustomType(2, TestCustomRow.class)
+ .build()) {
+ for (Record record : reader) {
+ readRecords.add(record);
+ }
+ }
+
+ assertThat(readRecords).hasSize(testData.size());
+
+ for (int i = 0; i < testData.size(); i++) {
+ Record expected = testData.get(i);
+ Record actual = readRecords.get(i);
+
+ assertThat(actual.get(0, Long.class)).isEqualTo(expected.get(0, Long.class));
+
+ Object expectedNested = expected.get(1);
+ Object actualNested = actual.get(1);
+
+ if (expectedNested == null && actualNested == null) {
+ continue;
+ }
+
+ if (actualNested != null) {
+ assertThat(actualNested)
+ .as("Custom type should be TestCustomRow, but was: " + actualNested.getClass())
+ .isInstanceOf(TestCustomRow.class);
+ TestCustomRow customRow = (TestCustomRow) actualNested;
+ Record expectedRecord = (Record) expectedNested;
+
+ assertThat(customRow.get(0, Long.class))
+ .isEqualTo(expectedRecord.get(0, Long.class)); // inner_id
+ assertThat(customRow.get(1, String.class))
+ .isEqualTo(expectedRecord.get(1, String.class)); // inner_name
+ }
+ }
+ }
+
+ private List<Record> createSimpleTestRecords() {
+ Schema schema = simpleSchema();
+ List<Record> records = Lists.newArrayList();
+
+ Record record1 = GenericRecord.create(schema);
+ record1.set(0, 1L);
+ record1.set(1, "Alice");
+
+ Record record2 = GenericRecord.create(schema);
+ record2.set(0, 2L);
+ record2.set(1, "Bob");
+
+ records.add(record1);
+ records.add(record2);
+
+ return records;
+ }
+
+ private List<Record> createNestedTestRecords() {
+ List<Record> records = Lists.newArrayList();
+
+ Record record1 = GenericRecord.create(NESTED_SCHEMA);
+ record1.set(0, 1L);
+
+ Record nestedStruct1 =
+ GenericRecord.create(NESTED_SCHEMA.findType("nested_struct").asStructType());
+ nestedStruct1.set(0, 100L);
+ nestedStruct1.set(1, "inner_alice");
+ record1.set(1, nestedStruct1);
+
+ Record record2 = GenericRecord.create(NESTED_SCHEMA);
+ record2.set(0, 2L);
+ record2.set(1, null); // null nested struct
+
+ records.add(record1);
+ records.add(record2);
+
+ return records;
+ }
+
+ private Schema simpleSchema() {
+ return new Schema(
+ Types.NestedField.required(1, "id", Types.LongType.get()),
+ Types.NestedField.optional(2, "name", Types.StringType.get()));
+ }
+
+ public static class TestCustomRow implements StructLike {
Review Comment:
+1, This is possible.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2360067935
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/InternalReader.java:
##########
@@ -20,37 +20,87 @@
import java.util.List;
import java.util.Map;
+import java.util.function.Function;
import org.apache.iceberg.Schema;
import org.apache.iceberg.StructLike;
+import org.apache.iceberg.data.Record;
+import org.apache.iceberg.parquet.Parquet;
import org.apache.iceberg.parquet.ParquetValueReader;
import org.apache.iceberg.parquet.ParquetValueReaders;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
import org.apache.iceberg.types.Types.StructType;
import org.apache.parquet.column.ColumnDescriptor;
import org.apache.parquet.schema.MessageType;
public class InternalReader<T extends StructLike> extends BaseParquetReaders<T> {
+ private final Map<Integer, Class<? extends StructLike>> typesById = Maps.newHashMap();
+
private static final InternalReader<?> INSTANCE = new InternalReader<>();
private InternalReader() {}
@SuppressWarnings("unchecked")
public static <T extends StructLike> ParquetValueReader<T> create(
- Schema expectedSchema, MessageType fileSchema) {
- return (ParquetValueReader<T>) INSTANCE.createReader(expectedSchema, fileSchema);
+ Schema expectedSchema, MessageType fileSchema, Map<Integer, ?> idToConstant) {
+ return (ParquetValueReader<T>) INSTANCE.createReader(expectedSchema, fileSchema, idToConstant);
}
@SuppressWarnings("unchecked")
public static <T extends StructLike> ParquetValueReader<T> create(
- Schema expectedSchema, MessageType fileSchema, Map<Integer, ?> idToConstant) {
- return (ParquetValueReader<T>) INSTANCE.createReader(expectedSchema, fileSchema, idToConstant);
+ Schema expectedSchema, MessageType fileSchema) {
+ return (ParquetValueReader<T>) INSTANCE.createReader(expectedSchema, fileSchema);
+ }
+
+ public static Parquet.ReadBuilder.ReaderFunction readerFunction() {
+ InternalReader<?> reader = new InternalReader<>();
+
+ return new Parquet.ReadBuilder.ReaderFunction() {
+ private Schema schema;
+
+ @Override
+ public Function<MessageType, ParquetValueReader<?>> apply() {
+ return messageType -> reader.createReader(schema, messageType);
+ }
+
+ @Override
+ public Parquet.ReadBuilder.ReaderFunction withSchema(Schema schema) {
+ this.schema = schema;
+ return this;
+ }
+
+ @Override
+ public Parquet.ReadBuilder.ReaderFunction withCustomTypes(
+ Map<Integer, Class<? extends StructLike>> customTypes) {
+ reader.typesById.putAll(customTypes);
+ return this;
+ }
+
+ @Override
+ public Parquet.ReadBuilder.ReaderFunction withRootType(Class<? extends StructLike> rootType) {
+ if (rootType != null) {
+ reader.typesById.put(ROOT_ID, rootType);
+ }
+ return this;
+ }
+ };
}
@Override
- @SuppressWarnings("unchecked")
protected ParquetValueReader<T> createStructReader(
List<ParquetValueReader<?>> fieldReaders, StructType structType) {
- return (ParquetValueReader<T>) ParquetValueReaders.recordReader(fieldReaders, structType);
+ throw new UnsupportedOperationException(
Review Comment:
Yeah I think deprecate is the right thing to do here, I didn't want to break anyone's subclasses (although I think that's pretty unlikely that there actually are any subclasses out there)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2359932407
##########
core/src/test/java/org/apache/iceberg/TestInternalData.java:
##########
@@ -0,0 +1,157 @@
+/*
+ * 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 java.io.IOException;
+import java.nio.file.Path;
+import java.util.Arrays;
+import java.util.List;
+import org.apache.iceberg.data.Record;
+import org.apache.iceberg.io.CloseableIterable;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.io.FileIO;
+import org.apache.iceberg.io.InputFile;
+import org.apache.iceberg.io.OutputFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.io.TempDir;
+
+@ExtendWith(ParameterizedTestExtension.class)
+public class TestInternalData {
+
+ @Parameter(index = 0)
+ private FileFormat format;
+
+ @Parameters(name = " format = {0}")
+ protected static List<Object> parameters() {
+ return Arrays.asList(new Object[] {FileFormat.AVRO}, new Object[] {FileFormat.PARQUET});
+ }
+
+ private static final Schema SIMPLE_SCHEMA =
+ new Schema(
+ Types.NestedField.required(1, "id", Types.LongType.get()),
+ Types.NestedField.optional(2, "name", Types.StringType.get()));
+
+ private static final Schema NESTED_SCHEMA =
+ new Schema(
+ Types.NestedField.required(1, "outer_id", Types.LongType.get()),
+ Types.NestedField.optional(
+ 2,
+ "nested_struct",
+ Types.StructType.of(
+ Types.NestedField.optional(3, "inner_id", Types.LongType.get()),
+ Types.NestedField.optional(4, "inner_name", Types.StringType.get()))));
+
+ @TempDir private Path tempDir;
+
+ private final FileIO fileIO = new TestTables.LocalFileIO();
+
Review Comment:
The "assertEquals" method only works on Records
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2337211131
##########
parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java:
##########
@@ -1281,12 +1304,24 @@ public ReadBuilder withNameMapping(NameMapping newNameMapping) {
@Override
public ReadBuilder setRootType(Class<? extends StructLike> rootClass) {
- throw new UnsupportedOperationException("Custom types are not yet supported");
+ Preconditions.checkArgument(
+ this.internalReader != null, "Cannot set root type without using an Internal Reader");
+ Preconditions.checkArgument(
+ this.readerFunc == null && this.readerFuncWithSchema == null,
+ "Setting root type is not compatible with setting a reader function");
+ internalReader.setRootType(rootClass);
Review Comment:
The issues here are mostly centered around the "readerFunc" apis. We want to use "internalReader" to generate our reader func since it needs to be built based on InteralReader and BaseParquetReader. So we need some marker to let Parquet.java know it should be using that class to build the reader func.
The previous code just set the reader func immediately upon using the InternalData pathway but that meant we would basically have to assume that if you call Root or CustomClass that we are able to replace that readerFunc with one built from internal data. I felt like this was a bit more straightforward.
Ie Previously
User calls InternalData read parquet
InternalData calls Parquet.read(...).withReaderFunct(() -> InternalReader.create())
Which gives us back
```
ReadBuilder {
readerFunc = Lambda$FromInternalData
}
```
Then when you call set custom data we would have to do this
```
setRootType() {
// Assume readerFunc Lambda was created by InternalReader so reassign it
readerFunc = () ->> InternalReader.create(..., customClassMap)
}
```
Or we could mess up the actual assignment logic where we check if these functions are set, and reset there
or reset with an internal reader there. I'm not sure any of these are really great solutions though.
```
if (customClasses & readerFunc not set or "use Internal reader flag set")) {
readerFunc = InternalReader.create(..., customClassMap)
}
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2343694682
##########
parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java:
##########
@@ -1281,12 +1304,24 @@ public ReadBuilder withNameMapping(NameMapping newNameMapping) {
@Override
public ReadBuilder setRootType(Class<? extends StructLike> rootClass) {
- throw new UnsupportedOperationException("Custom types are not yet supported");
+ Preconditions.checkArgument(
+ this.internalReader != null, "Cannot set root type without using an Internal Reader");
+ Preconditions.checkArgument(
+ this.readerFunc == null && this.readerFuncWithSchema == null,
+ "Setting root type is not compatible with setting a reader function");
+ internalReader.setRootType(rootClass);
Review Comment:
https://github.com/apache/iceberg/commit/01afda90b99f6fa31bc9a3bb6d9b42e6722a3bfd only needs a new interface, because Java is dumb, and there is no `TriFunction` available 😄
I have the following concerns with the current proposal:
1. If you take a look at the proposal with the `InternalReader`, the order of the method calls is important which is a bad practice:
```
// This is working
builder.useInternalReader(internal).setRootType(PartitionData.class);
// This is failing
builder.setRootType(PartitionData.class).useInternalReader(internal);
```
2. Also it is a bit confusing that we have 2 modes to create the `InternalReader`:
- We can create them with static methods,
- We also expose the constructor.
I think it is better to stick to the current patterns:
- Collect the info in the `Parquet.ReadBuilder`
- Use a read function to share the important part of the configuration to the reader.
This is how it is done in all of the Readers/Writers at this point, and I have followed this pattern in my File Format API implementation as well.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2348289165
##########
parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java:
##########
@@ -1281,12 +1304,24 @@ public ReadBuilder withNameMapping(NameMapping newNameMapping) {
@Override
public ReadBuilder setRootType(Class<? extends StructLike> rootClass) {
- throw new UnsupportedOperationException("Custom types are not yet supported");
+ Preconditions.checkArgument(
+ this.internalReader != null, "Cannot set root type without using an Internal Reader");
+ Preconditions.checkArgument(
+ this.readerFunc == null && this.readerFuncWithSchema == null,
+ "Setting root type is not compatible with setting a reader function");
+ internalReader.setRootType(rootClass);
Review Comment:
Oh.. I thought, that I have answered this one. Sorry for the late reply.
> I was wondering if there is a practical difference between making a TriFunction and just using the InternalReader.
In my File Format API PR, I need to push some of the builder parameters to the engine specific code, so the FormatModel can create the correct readerFuncion/writerFunction. For example: Iceberg schema, File schema, Engine schema, Constant fields (former idToConstant map), delete filters (which are needed for creating vectorized Parquet readers).
In all of these cases, I stick to the following pattern:
- Collect the input in the Builder
- In the build method, based on the parameter decide which function will be used
- Call the function with the collected parameters
This pattern works well when we have multiple possible read/write functions using some overlapping parameters.
If we follow the `.useInternalReader(...)` pattern everywhere, we need to split the parameters to 2 groups:
- Common parameters - these will be the parameters provided to the reader/writer functions
- Specific parameters - these will be collected by the reader/writer objects, and the builder method will be only a wrapper around the InternalReader type object.
I rather stick to a single model.
WDYT?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#issuecomment-3391730830
> I don't think there's value in adding custom type support to ORC if we're not going to use it anyway?
The suggestion is not to add custom type support. The suggestion is to merge together the different readerFunctions, writerFunctions. Currently we have 2-3 versions of them for every file format. We also have a different builder method setting every one of them. Russell's change created a single, common reader function which handles all of the cases, and enables us to add new inputs to the functions easily. We could follow the same pattern for every FileFormat. The File Format API will need to add new parameters to these methods. This is way easier if we use similar ReadBuilder interfaces like the one introduced by Russell in this PR.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2373110160
##########
.palantir/revapi.yml:
##########
@@ -1333,6 +1333,44 @@ acceptedBreaks:
\ java.util.List<org.apache.iceberg.PositionDeletesScanTask>)"
justification: "Removing deprecations for 1.10.0"
org.apache.iceberg:iceberg-parquet:
+ - code: "java.method.parameterTypeChanged"
Review Comment:
Rebasing fixed this!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#issuecomment-3349864678
Merged! Thanks everyone for your thoughtful reviews!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "shangxinli (via GitHub)" <gi...@apache.org>.
shangxinli commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2384369169
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetValueReaders.java:
##########
@@ -1132,6 +1144,46 @@ private TripleIterator<?> firstNonNullColumn(List<TripleIterator<?>> columns) {
}
}
+ private static class StructLikeReader<T extends StructLike> extends StructReader<T, T> {
+ private final Types.StructType struct;
+ private final DynConstructors.Ctor<T> ctor;
+
+ StructLikeReader(
+ List<ParquetValueReader<?>> readers, Types.StructType struct, Class<T> structLikeClass) {
+ super(readers);
+ this.struct = struct;
+ this.ctor =
Review Comment:
i see
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2344488608
##########
parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java:
##########
@@ -1281,12 +1304,24 @@ public ReadBuilder withNameMapping(NameMapping newNameMapping) {
@Override
public ReadBuilder setRootType(Class<? extends StructLike> rootClass) {
- throw new UnsupportedOperationException("Custom types are not yet supported");
+ Preconditions.checkArgument(
+ this.internalReader != null, "Cannot set root type without using an Internal Reader");
+ Preconditions.checkArgument(
+ this.readerFunc == null && this.readerFuncWithSchema == null,
+ "Setting root type is not compatible with setting a reader function");
+ internalReader.setRootType(rootClass);
Review Comment:
We can fix the ordering issue by making InternalReader passed the class map as well is what I was suggesting. I was wondering if there is a practical difference between making a TriFunction and just using the InternalReader.
Ie
.setRootType(...).useInternalReader(...)
Where when we get the reader function we still call
InternalReader.create(Func, TypeMap)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2353766565
##########
parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java:
##########
@@ -1281,12 +1304,24 @@ public ReadBuilder withNameMapping(NameMapping newNameMapping) {
@Override
public ReadBuilder setRootType(Class<? extends StructLike> rootClass) {
- throw new UnsupportedOperationException("Custom types are not yet supported");
+ Preconditions.checkArgument(
+ this.internalReader != null, "Cannot set root type without using an Internal Reader");
+ Preconditions.checkArgument(
+ this.readerFunc == null && this.readerFuncWithSchema == null,
+ "Setting root type is not compatible with setting a reader function");
+ internalReader.setRootType(rootClass);
Review Comment:
Sorry, I followed what you were saying I was just wondering if we shouldn't be making an interface but making a ReaderFunctionFactory class itself
```
class ParquetReaderFunction {
Function<Message, ParquetValueReaderBuilder> build()
ParquetReaderFunction withSchema(Schema schema) {
// Overload if we are a readerFuncWithSchema
return this
}
ParquetReaderFunction withCustomTypes<Map<Integer, Class<T extends StructLike> {
// Overload if we are using CustomTypes
return this
}
}
```
This is a bigger overall refactor, but I'm wondering if we can replace the currently 3 (will be 4 after this) setters we currently have with just 2
ParquetReaderFunction
And
VectorizedReaderFunction
And we rely on implementations to decide whether they do something with withSchema or withCustomTypes\
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2335138340
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/BaseParquetReaders.java:
##########
@@ -53,6 +53,9 @@
import org.apache.parquet.schema.Type;
abstract class BaseParquetReaders<T> {
+ // Root ID is used for the top-level struct in the Parquet Schema
+ protected static final int ROOT_ID = 1;
Review Comment:
1 is a valid field id. should use use invalid value like `0` or `-1`?
##########
core/src/test/java/org/apache/iceberg/TestInternalData.java:
##########
@@ -0,0 +1,245 @@
+/*
+ * 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 java.io.IOException;
+import java.nio.file.Path;
+import java.util.Arrays;
+import java.util.List;
+import org.apache.iceberg.data.GenericRecord;
+import org.apache.iceberg.data.Record;
+import org.apache.iceberg.io.CloseableIterable;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.io.FileIO;
+import org.apache.iceberg.io.InputFile;
+import org.apache.iceberg.io.OutputFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.io.TempDir;
+
+@ExtendWith(ParameterizedTestExtension.class)
+public class TestInternalData {
+
+ @Parameter(index = 0)
+ private String format;
+
+ @Parameters(name = "format={0}")
+ public static Object[][] parameters() {
+ return new Object[][] {{"avro"}, {"parquet"}};
+ }
+
+ private static final Schema NESTED_SCHEMA =
+ new Schema(
+ Types.NestedField.required(1, "outer_id", Types.LongType.get()),
+ Types.NestedField.optional(
+ 2,
+ "nested_struct",
+ Types.StructType.of(
+ Types.NestedField.optional(3, "inner_id", Types.LongType.get()),
+ Types.NestedField.optional(4, "inner_name", Types.StringType.get()))));
+
+ @TempDir private Path tempDir;
+
+ private final FileIO fileIO = new TestTables.LocalFileIO();
+
+ @TestTemplate
+ public void testCustomRootType() throws IOException {
+ FileFormat fileFormat = FileFormat.fromString(format);
+ OutputFile outputFile = fileIO.newOutputFile(tempDir.resolve("test." + format).toString());
+
+ List<Record> testData = createSimpleTestRecords();
+
+ try (FileAppender<Record> appender =
+ InternalData.write(fileFormat, outputFile).schema(simpleSchema()).build()) {
+ appender.addAll(testData);
+ }
+
+ InputFile inputFile = fileIO.newInputFile(outputFile.location());
+ List<PartitionData> readRecords = Lists.newArrayList();
+
+ try (CloseableIterable<PartitionData> reader =
+ InternalData.read(fileFormat, inputFile)
+ .project(simpleSchema())
+ .setRootType(PartitionData.class)
+ .build()) {
+ for (PartitionData record : reader) {
+ readRecords.add(record);
+ }
+ }
+
+ assertThat(readRecords).hasSize(testData.size());
+
+ for (int i = 0; i < testData.size(); i++) {
+ Record expected = testData.get(i);
+ PartitionData actual = readRecords.get(i);
+
+ assertThat(actual.get(0, Long.class)).isEqualTo(expected.get(0, Long.class));
+ assertThat(actual.get(1, String.class)).isEqualTo(expected.get(1, String.class));
+ }
+ }
+
+ @TestTemplate
+ public void testCustomTypeForNestedField() throws IOException {
+ FileFormat fileFormat = FileFormat.fromString(format);
+ OutputFile outputFile = fileIO.newOutputFile(tempDir.resolve("test." + format).toString());
+
+ List<Record> testData = createNestedTestRecords();
+
+ try (FileAppender<Record> appender =
+ InternalData.write(fileFormat, outputFile).schema(NESTED_SCHEMA).build()) {
+ appender.addAll(testData);
+ }
+
+ InputFile inputFile = fileIO.newInputFile(outputFile.location());
+ List<Record> readRecords = Lists.newArrayList();
+
+ try (CloseableIterable<Record> reader =
+ InternalData.read(fileFormat, inputFile)
+ .project(NESTED_SCHEMA)
+ .setCustomType(2, TestCustomRow.class)
+ .build()) {
+ for (Record record : reader) {
+ readRecords.add(record);
+ }
+ }
+
+ assertThat(readRecords).hasSize(testData.size());
+
+ for (int i = 0; i < testData.size(); i++) {
+ Record expected = testData.get(i);
+ Record actual = readRecords.get(i);
+
+ assertThat(actual.get(0, Long.class)).isEqualTo(expected.get(0, Long.class));
+
+ Object expectedNested = expected.get(1);
+ Object actualNested = actual.get(1);
+
+ if (expectedNested == null && actualNested == null) {
+ continue;
+ }
+
+ if (actualNested != null) {
+ assertThat(actualNested)
+ .as("Custom type should be TestCustomRow, but was: " + actualNested.getClass())
+ .isInstanceOf(TestCustomRow.class);
+ TestCustomRow customRow = (TestCustomRow) actualNested;
+ Record expectedRecord = (Record) expectedNested;
+
+ assertThat(customRow.get(0, Long.class))
+ .isEqualTo(expectedRecord.get(0, Long.class)); // inner_id
+ assertThat(customRow.get(1, String.class))
+ .isEqualTo(expectedRecord.get(1, String.class)); // inner_name
+ }
+ }
+ }
+
+ private List<Record> createSimpleTestRecords() {
+ Schema schema = simpleSchema();
+ List<Record> records = Lists.newArrayList();
+
+ Record record1 = GenericRecord.create(schema);
+ record1.set(0, 1L);
+ record1.set(1, "Alice");
+
+ Record record2 = GenericRecord.create(schema);
+ record2.set(0, 2L);
+ record2.set(1, "Bob");
+
+ records.add(record1);
+ records.add(record2);
+
+ return records;
+ }
+
+ private List<Record> createNestedTestRecords() {
+ List<Record> records = Lists.newArrayList();
+
+ Record record1 = GenericRecord.create(NESTED_SCHEMA);
+ record1.set(0, 1L);
+
+ Record nestedStruct1 =
+ GenericRecord.create(NESTED_SCHEMA.findType("nested_struct").asStructType());
+ nestedStruct1.set(0, 100L);
+ nestedStruct1.set(1, "inner_alice");
+ record1.set(1, nestedStruct1);
+
+ Record record2 = GenericRecord.create(NESTED_SCHEMA);
+ record2.set(0, 2L);
+ record2.set(1, null); // null nested struct
+
+ records.add(record1);
+ records.add(record2);
+
+ return records;
+ }
+
+ private Schema simpleSchema() {
+ return new Schema(
+ Types.NestedField.required(1, "id", Types.LongType.get()),
+ Types.NestedField.optional(2, "name", Types.StringType.get()));
+ }
+
+ public static class TestCustomRow implements StructLike {
Review Comment:
can we enhance the `CustomRow` from `TestHelpers` class by adding a new public constructor that takes a struct type?
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/BaseParquetReaders.java:
##########
@@ -75,6 +78,16 @@ protected ParquetValueReader<T> createReader(
}
}
+ /**
+ * This method can be overridden to provide a custom implementation which also uses the fieldId of
+ * the Schema when creating the struct reader
+ */
+ protected ParquetValueReader<T> createStructReader(
Review Comment:
A little weird that revapi complains about API change for non-public class.
Should we just modify the original method in line 91 directly? there are only two child classes `InternalReader` and `GenericParquetReaders`. Then we can avoid the fallback behavior, which is a bit odd to me.
##########
core/src/test/java/org/apache/iceberg/TestInternalData.java:
##########
@@ -0,0 +1,245 @@
+/*
+ * 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 java.io.IOException;
+import java.nio.file.Path;
+import java.util.Arrays;
+import java.util.List;
+import org.apache.iceberg.data.GenericRecord;
+import org.apache.iceberg.data.Record;
+import org.apache.iceberg.io.CloseableIterable;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.io.FileIO;
+import org.apache.iceberg.io.InputFile;
+import org.apache.iceberg.io.OutputFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.io.TempDir;
+
+@ExtendWith(ParameterizedTestExtension.class)
+public class TestInternalData {
+
+ @Parameter(index = 0)
+ private String format;
+
+ @Parameters(name = "format={0}")
+ public static Object[][] parameters() {
+ return new Object[][] {{"avro"}, {"parquet"}};
+ }
+
+ private static final Schema NESTED_SCHEMA =
+ new Schema(
+ Types.NestedField.required(1, "outer_id", Types.LongType.get()),
+ Types.NestedField.optional(
+ 2,
+ "nested_struct",
+ Types.StructType.of(
+ Types.NestedField.optional(3, "inner_id", Types.LongType.get()),
+ Types.NestedField.optional(4, "inner_name", Types.StringType.get()))));
+
+ @TempDir private Path tempDir;
+
+ private final FileIO fileIO = new TestTables.LocalFileIO();
+
+ @TestTemplate
+ public void testCustomRootType() throws IOException {
+ FileFormat fileFormat = FileFormat.fromString(format);
+ OutputFile outputFile = fileIO.newOutputFile(tempDir.resolve("test." + format).toString());
+
+ List<Record> testData = createSimpleTestRecords();
+
+ try (FileAppender<Record> appender =
+ InternalData.write(fileFormat, outputFile).schema(simpleSchema()).build()) {
+ appender.addAll(testData);
+ }
+
+ InputFile inputFile = fileIO.newInputFile(outputFile.location());
+ List<PartitionData> readRecords = Lists.newArrayList();
+
+ try (CloseableIterable<PartitionData> reader =
+ InternalData.read(fileFormat, inputFile)
+ .project(simpleSchema())
+ .setRootType(PartitionData.class)
+ .build()) {
+ for (PartitionData record : reader) {
+ readRecords.add(record);
+ }
+ }
+
+ assertThat(readRecords).hasSize(testData.size());
+
+ for (int i = 0; i < testData.size(); i++) {
+ Record expected = testData.get(i);
+ PartitionData actual = readRecords.get(i);
+
+ assertThat(actual.get(0, Long.class)).isEqualTo(expected.get(0, Long.class));
+ assertThat(actual.get(1, String.class)).isEqualTo(expected.get(1, String.class));
+ }
+ }
+
+ @TestTemplate
+ public void testCustomTypeForNestedField() throws IOException {
+ FileFormat fileFormat = FileFormat.fromString(format);
+ OutputFile outputFile = fileIO.newOutputFile(tempDir.resolve("test." + format).toString());
+
+ List<Record> testData = createNestedTestRecords();
+
+ try (FileAppender<Record> appender =
+ InternalData.write(fileFormat, outputFile).schema(NESTED_SCHEMA).build()) {
+ appender.addAll(testData);
+ }
+
+ InputFile inputFile = fileIO.newInputFile(outputFile.location());
+ List<Record> readRecords = Lists.newArrayList();
+
+ try (CloseableIterable<Record> reader =
+ InternalData.read(fileFormat, inputFile)
+ .project(NESTED_SCHEMA)
+ .setCustomType(2, TestCustomRow.class)
+ .build()) {
+ for (Record record : reader) {
+ readRecords.add(record);
+ }
+ }
+
+ assertThat(readRecords).hasSize(testData.size());
+
+ for (int i = 0; i < testData.size(); i++) {
+ Record expected = testData.get(i);
+ Record actual = readRecords.get(i);
+
+ assertThat(actual.get(0, Long.class)).isEqualTo(expected.get(0, Long.class));
+
+ Object expectedNested = expected.get(1);
+ Object actualNested = actual.get(1);
+
+ if (expectedNested == null && actualNested == null) {
+ continue;
+ }
+
+ if (actualNested != null) {
+ assertThat(actualNested)
+ .as("Custom type should be TestCustomRow, but was: " + actualNested.getClass())
+ .isInstanceOf(TestCustomRow.class);
+ TestCustomRow customRow = (TestCustomRow) actualNested;
+ Record expectedRecord = (Record) expectedNested;
+
+ assertThat(customRow.get(0, Long.class))
+ .isEqualTo(expectedRecord.get(0, Long.class)); // inner_id
+ assertThat(customRow.get(1, String.class))
+ .isEqualTo(expectedRecord.get(1, String.class)); // inner_name
+ }
+ }
+ }
+
+ private List<Record> createSimpleTestRecords() {
Review Comment:
instead of the two create methods, maybe use `RandomInternalData.generate(Schema schema, int numRecords, long seed)` ?
##########
core/src/test/java/org/apache/iceberg/TestInternalData.java:
##########
@@ -0,0 +1,245 @@
+/*
+ * 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 java.io.IOException;
+import java.nio.file.Path;
+import java.util.Arrays;
+import java.util.List;
+import org.apache.iceberg.data.GenericRecord;
+import org.apache.iceberg.data.Record;
+import org.apache.iceberg.io.CloseableIterable;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.io.FileIO;
+import org.apache.iceberg.io.InputFile;
+import org.apache.iceberg.io.OutputFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.io.TempDir;
+
+@ExtendWith(ParameterizedTestExtension.class)
+public class TestInternalData {
+
+ @Parameter(index = 0)
+ private String format;
+
+ @Parameters(name = "format={0}")
+ public static Object[][] parameters() {
+ return new Object[][] {{"avro"}, {"parquet"}};
Review Comment:
nit: we can use enum directly here.
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetValueReaders.java:
##########
@@ -189,6 +191,16 @@ public static ParquetValueReader<Long> int96Timestamps(ColumnDescriptor desc) {
return new TimestampInt96Reader(desc);
}
+ @SuppressWarnings("unchecked")
+ public static <T extends StructLike> ParquetValueReader<T> structLikeReader(
+ List<ParquetValueReader<?>> readers, Types.StructType struct, Class<T> structClass) {
+ if (structClass.equals(Record.class)) {
Review Comment:
`Record` extends from `StructLike` type. can we remove the `RecordReader`? Maybe the only reason for potentially small performance difference with `GenericRecord.copy()` in the `newStructData` method.
```
// GenericRecord.copy() is more performant than GenericRecord.create(StructType) since
// NAME_MAP_CACHE access is eliminated. Using copy here to gain performance.
```
##########
core/src/test/java/org/apache/iceberg/TestInternalData.java:
##########
@@ -0,0 +1,245 @@
+/*
+ * 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 java.io.IOException;
+import java.nio.file.Path;
+import java.util.Arrays;
+import java.util.List;
+import org.apache.iceberg.data.GenericRecord;
+import org.apache.iceberg.data.Record;
+import org.apache.iceberg.io.CloseableIterable;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.io.FileIO;
+import org.apache.iceberg.io.InputFile;
+import org.apache.iceberg.io.OutputFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.io.TempDir;
+
+@ExtendWith(ParameterizedTestExtension.class)
+public class TestInternalData {
+
+ @Parameter(index = 0)
+ private String format;
+
+ @Parameters(name = "format={0}")
+ public static Object[][] parameters() {
+ return new Object[][] {{"avro"}, {"parquet"}};
+ }
+
+ private static final Schema NESTED_SCHEMA =
+ new Schema(
+ Types.NestedField.required(1, "outer_id", Types.LongType.get()),
+ Types.NestedField.optional(
+ 2,
+ "nested_struct",
+ Types.StructType.of(
+ Types.NestedField.optional(3, "inner_id", Types.LongType.get()),
+ Types.NestedField.optional(4, "inner_name", Types.StringType.get()))));
+
+ @TempDir private Path tempDir;
+
+ private final FileIO fileIO = new TestTables.LocalFileIO();
+
+ @TestTemplate
+ public void testCustomRootType() throws IOException {
+ FileFormat fileFormat = FileFormat.fromString(format);
+ OutputFile outputFile = fileIO.newOutputFile(tempDir.resolve("test." + format).toString());
+
+ List<Record> testData = createSimpleTestRecords();
+
+ try (FileAppender<Record> appender =
+ InternalData.write(fileFormat, outputFile).schema(simpleSchema()).build()) {
+ appender.addAll(testData);
+ }
+
+ InputFile inputFile = fileIO.newInputFile(outputFile.location());
+ List<PartitionData> readRecords = Lists.newArrayList();
+
+ try (CloseableIterable<PartitionData> reader =
+ InternalData.read(fileFormat, inputFile)
+ .project(simpleSchema())
+ .setRootType(PartitionData.class)
+ .build()) {
+ for (PartitionData record : reader) {
+ readRecords.add(record);
+ }
+ }
+
+ assertThat(readRecords).hasSize(testData.size());
+
+ for (int i = 0; i < testData.size(); i++) {
+ Record expected = testData.get(i);
+ PartitionData actual = readRecords.get(i);
+
+ assertThat(actual.get(0, Long.class)).isEqualTo(expected.get(0, Long.class));
+ assertThat(actual.get(1, String.class)).isEqualTo(expected.get(1, String.class));
+ }
+ }
+
+ @TestTemplate
+ public void testCustomTypeForNestedField() throws IOException {
+ FileFormat fileFormat = FileFormat.fromString(format);
+ OutputFile outputFile = fileIO.newOutputFile(tempDir.resolve("test." + format).toString());
+
+ List<Record> testData = createNestedTestRecords();
+
+ try (FileAppender<Record> appender =
+ InternalData.write(fileFormat, outputFile).schema(NESTED_SCHEMA).build()) {
+ appender.addAll(testData);
+ }
+
+ InputFile inputFile = fileIO.newInputFile(outputFile.location());
+ List<Record> readRecords = Lists.newArrayList();
+
+ try (CloseableIterable<Record> reader =
+ InternalData.read(fileFormat, inputFile)
+ .project(NESTED_SCHEMA)
+ .setCustomType(2, TestCustomRow.class)
+ .build()) {
+ for (Record record : reader) {
+ readRecords.add(record);
+ }
+ }
+
+ assertThat(readRecords).hasSize(testData.size());
+
+ for (int i = 0; i < testData.size(); i++) {
+ Record expected = testData.get(i);
+ Record actual = readRecords.get(i);
+
+ assertThat(actual.get(0, Long.class)).isEqualTo(expected.get(0, Long.class));
+
+ Object expectedNested = expected.get(1);
+ Object actualNested = actual.get(1);
+
+ if (expectedNested == null && actualNested == null) {
+ continue;
+ }
+
+ if (actualNested != null) {
+ assertThat(actualNested)
+ .as("Custom type should be TestCustomRow, but was: " + actualNested.getClass())
+ .isInstanceOf(TestCustomRow.class);
+ TestCustomRow customRow = (TestCustomRow) actualNested;
+ Record expectedRecord = (Record) expectedNested;
+
+ assertThat(customRow.get(0, Long.class))
+ .isEqualTo(expectedRecord.get(0, Long.class)); // inner_id
+ assertThat(customRow.get(1, String.class))
+ .isEqualTo(expectedRecord.get(1, String.class)); // inner_name
+ }
+ }
+ }
+
+ private List<Record> createSimpleTestRecords() {
+ Schema schema = simpleSchema();
+ List<Record> records = Lists.newArrayList();
+
+ Record record1 = GenericRecord.create(schema);
+ record1.set(0, 1L);
+ record1.set(1, "Alice");
+
+ Record record2 = GenericRecord.create(schema);
+ record2.set(0, 2L);
+ record2.set(1, "Bob");
+
+ records.add(record1);
+ records.add(record2);
+
+ return records;
+ }
+
+ private List<Record> createNestedTestRecords() {
+ List<Record> records = Lists.newArrayList();
+
+ Record record1 = GenericRecord.create(NESTED_SCHEMA);
+ record1.set(0, 1L);
+
+ Record nestedStruct1 =
+ GenericRecord.create(NESTED_SCHEMA.findType("nested_struct").asStructType());
+ nestedStruct1.set(0, 100L);
+ nestedStruct1.set(1, "inner_alice");
+ record1.set(1, nestedStruct1);
+
+ Record record2 = GenericRecord.create(NESTED_SCHEMA);
+ record2.set(0, 2L);
+ record2.set(1, null); // null nested struct
+
+ records.add(record1);
+ records.add(record2);
+
+ return records;
+ }
+
+ private Schema simpleSchema() {
Review Comment:
nit: define it at the beginning of this class like the nested schema?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2336660088
##########
core/src/test/java/org/apache/iceberg/TestInternalData.java:
##########
@@ -0,0 +1,245 @@
+/*
+ * 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 java.io.IOException;
+import java.nio.file.Path;
+import java.util.Arrays;
+import java.util.List;
+import org.apache.iceberg.data.GenericRecord;
+import org.apache.iceberg.data.Record;
+import org.apache.iceberg.io.CloseableIterable;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.io.FileIO;
+import org.apache.iceberg.io.InputFile;
+import org.apache.iceberg.io.OutputFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.io.TempDir;
+
+@ExtendWith(ParameterizedTestExtension.class)
+public class TestInternalData {
+
+ @Parameter(index = 0)
+ private String format;
+
+ @Parameters(name = "format={0}")
+ public static Object[][] parameters() {
+ return new Object[][] {{"avro"}, {"parquet"}};
+ }
+
+ private static final Schema NESTED_SCHEMA =
+ new Schema(
+ Types.NestedField.required(1, "outer_id", Types.LongType.get()),
+ Types.NestedField.optional(
+ 2,
+ "nested_struct",
+ Types.StructType.of(
+ Types.NestedField.optional(3, "inner_id", Types.LongType.get()),
+ Types.NestedField.optional(4, "inner_name", Types.StringType.get()))));
+
+ @TempDir private Path tempDir;
+
+ private final FileIO fileIO = new TestTables.LocalFileIO();
+
+ @TestTemplate
+ public void testCustomRootType() throws IOException {
+ FileFormat fileFormat = FileFormat.fromString(format);
+ OutputFile outputFile = fileIO.newOutputFile(tempDir.resolve("test." + format).toString());
+
+ List<Record> testData = createSimpleTestRecords();
+
+ try (FileAppender<Record> appender =
+ InternalData.write(fileFormat, outputFile).schema(simpleSchema()).build()) {
+ appender.addAll(testData);
+ }
+
+ InputFile inputFile = fileIO.newInputFile(outputFile.location());
+ List<PartitionData> readRecords = Lists.newArrayList();
+
+ try (CloseableIterable<PartitionData> reader =
+ InternalData.read(fileFormat, inputFile)
+ .project(simpleSchema())
+ .setRootType(PartitionData.class)
+ .build()) {
+ for (PartitionData record : reader) {
+ readRecords.add(record);
+ }
+ }
+
+ assertThat(readRecords).hasSize(testData.size());
+
+ for (int i = 0; i < testData.size(); i++) {
+ Record expected = testData.get(i);
+ PartitionData actual = readRecords.get(i);
+
+ assertThat(actual.get(0, Long.class)).isEqualTo(expected.get(0, Long.class));
+ assertThat(actual.get(1, String.class)).isEqualTo(expected.get(1, String.class));
+ }
+ }
+
+ @TestTemplate
+ public void testCustomTypeForNestedField() throws IOException {
+ FileFormat fileFormat = FileFormat.fromString(format);
+ OutputFile outputFile = fileIO.newOutputFile(tempDir.resolve("test." + format).toString());
+
+ List<Record> testData = createNestedTestRecords();
+
+ try (FileAppender<Record> appender =
+ InternalData.write(fileFormat, outputFile).schema(NESTED_SCHEMA).build()) {
+ appender.addAll(testData);
+ }
+
+ InputFile inputFile = fileIO.newInputFile(outputFile.location());
+ List<Record> readRecords = Lists.newArrayList();
+
+ try (CloseableIterable<Record> reader =
+ InternalData.read(fileFormat, inputFile)
+ .project(NESTED_SCHEMA)
+ .setCustomType(2, TestCustomRow.class)
+ .build()) {
+ for (Record record : reader) {
+ readRecords.add(record);
+ }
+ }
+
+ assertThat(readRecords).hasSize(testData.size());
+
+ for (int i = 0; i < testData.size(); i++) {
+ Record expected = testData.get(i);
+ Record actual = readRecords.get(i);
+
+ assertThat(actual.get(0, Long.class)).isEqualTo(expected.get(0, Long.class));
+
+ Object expectedNested = expected.get(1);
+ Object actualNested = actual.get(1);
+
+ if (expectedNested == null && actualNested == null) {
+ continue;
+ }
+
+ if (actualNested != null) {
Review Comment:
If something went wrong, the `expectedNested` could still be `null` at this point.
Maybe:
```
assertThat(actualNested).isNotNull();
assertThat(expectedNested).isNotNull();
```
and then we can skip the if?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2373096694
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/BaseParquetReaders.java:
##########
@@ -75,8 +78,26 @@ protected ParquetValueReader<T> createReader(
}
}
- protected abstract ParquetValueReader<T> createStructReader(
- List<ParquetValueReader<?>> fieldReaders, Types.StructType structType);
+ /**
+ * This method can be overridden to provide a custom implementation which also uses the fieldId of
+ * the Schema when creating the struct reader
+ */
+ protected ParquetValueReader<T> createStructReader(
+ List<ParquetValueReader<?>> fieldReaders, Types.StructType structType, Integer fieldId) {
+ // Fallback to the signature without fieldId if not overridden
+ return createStructReader(fieldReaders, structType);
Review Comment:
@nastra ^ See here (this is what keeps subclasses written by outside users from breaking)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2373098707
##########
.palantir/revapi.yml:
##########
@@ -1333,6 +1333,44 @@ acceptedBreaks:
\ java.util.List<org.apache.iceberg.PositionDeletesScanTask>)"
justification: "Removing deprecations for 1.10.0"
org.apache.iceberg:iceberg-parquet:
+ - code: "java.method.parameterTypeChanged"
Review Comment:
They still fail on my machine, let me try rebasing
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2373144937
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/BaseParquetReaders.java:
##########
@@ -75,8 +78,26 @@ protected ParquetValueReader<T> createReader(
}
}
- protected abstract ParquetValueReader<T> createStructReader(
- List<ParquetValueReader<?>> fieldReaders, Types.StructType structType);
+ /**
+ * This method can be overridden to provide a custom implementation which also uses the fieldId of
+ * the Schema when creating the struct reader
+ */
+ protected ParquetValueReader<T> createStructReader(
+ List<ParquetValueReader<?>> fieldReaders, Types.StructType structType, Integer fieldId) {
+ // Fallback to the signature without fieldId if not overridden
+ return createStructReader(fieldReaders, structType);
+ }
+
+ /**
+ * @deprecated will be removed in 1.11.0. Subclasses should override {@link
+ * #createStructReader(List, Types.StructType, Integer)} instead
+ */
+ @Deprecated
+ protected ParquetValueReader<T> createStructReader(
Review Comment:
Now it should be much clearer that this used to be abstract!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2371762289
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/InternalReader.java:
##########
@@ -46,11 +52,48 @@ public static <T extends StructLike> ParquetValueReader<T> create(
return (ParquetValueReader<T>) INSTANCE.createReader(expectedSchema, fileSchema, idToConstant);
}
+ public static Parquet.ReadBuilder.ReaderFunction readerFunction() {
+ InternalReader<?> reader = new InternalReader<>();
+
+ return new Parquet.ReadBuilder.ReaderFunction() {
+ private Schema schema;
+
+ @Override
+ public Function<MessageType, ParquetValueReader<?>> apply() {
+ return messageType -> reader.createReader(schema, messageType);
+ }
+
+ @Override
+ public Parquet.ReadBuilder.ReaderFunction withSchema(Schema schema) {
+ this.schema = schema;
+ return this;
+ }
+
+ @Override
+ public Parquet.ReadBuilder.ReaderFunction withCustomTypes(
Review Comment:
I actually like this naming you have here better, but just wanted to point out that in the Avro reader we use `setCustomTypes` / `setRootType` / `setSchema`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#issuecomment-3390938789
I don't think there's value in adding custom type support to ORC if we're not going to use it anyway?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2363308336
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/BaseParquetReaders.java:
##########
@@ -75,8 +78,26 @@ protected ParquetValueReader<T> createReader(
}
}
- protected abstract ParquetValueReader<T> createStructReader(
- List<ParquetValueReader<?>> fieldReaders, Types.StructType structType);
+ /**
+ * This method can be overridden to provide a custom implementation which also uses the fieldId of
+ * the Schema when creating the struct reader
+ */
+ protected ParquetValueReader<T> createStructReader(
+ List<ParquetValueReader<?>> fieldReaders, Types.StructType structType, Integer fieldId) {
+ // Fallback to the signature without fieldId if not overridden
+ return createStructReader(fieldReaders, structType);
+ }
+
+ /**
+ * @deprecated will be removed in 1.11.0. Subclasses should override {@link
+ * #createStructReader(List, Types.StructType, Integer)} instead
+ */
+ @Deprecated
+ protected ParquetValueReader<T> createStructReader(
Review Comment:
🤦 You are so right
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2360129160
##########
core/src/test/java/org/apache/iceberg/TestInternalData.java:
##########
@@ -0,0 +1,245 @@
+/*
+ * 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 java.io.IOException;
+import java.nio.file.Path;
+import java.util.Arrays;
+import java.util.List;
+import org.apache.iceberg.data.GenericRecord;
+import org.apache.iceberg.data.Record;
+import org.apache.iceberg.io.CloseableIterable;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.io.FileIO;
+import org.apache.iceberg.io.InputFile;
+import org.apache.iceberg.io.OutputFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.io.TempDir;
+
+@ExtendWith(ParameterizedTestExtension.class)
+public class TestInternalData {
+
+ @Parameter(index = 0)
+ private String format;
+
+ @Parameters(name = "format={0}")
+ public static Object[][] parameters() {
+ return new Object[][] {{"avro"}, {"parquet"}};
+ }
+
+ private static final Schema NESTED_SCHEMA =
+ new Schema(
+ Types.NestedField.required(1, "outer_id", Types.LongType.get()),
+ Types.NestedField.optional(
+ 2,
+ "nested_struct",
+ Types.StructType.of(
+ Types.NestedField.optional(3, "inner_id", Types.LongType.get()),
+ Types.NestedField.optional(4, "inner_name", Types.StringType.get()))));
+
+ @TempDir private Path tempDir;
+
+ private final FileIO fileIO = new TestTables.LocalFileIO();
+
+ @TestTemplate
+ public void testCustomRootType() throws IOException {
+ FileFormat fileFormat = FileFormat.fromString(format);
+ OutputFile outputFile = fileIO.newOutputFile(tempDir.resolve("test." + format).toString());
+
+ List<Record> testData = createSimpleTestRecords();
+
+ try (FileAppender<Record> appender =
+ InternalData.write(fileFormat, outputFile).schema(simpleSchema()).build()) {
+ appender.addAll(testData);
+ }
+
+ InputFile inputFile = fileIO.newInputFile(outputFile.location());
+ List<PartitionData> readRecords = Lists.newArrayList();
+
+ try (CloseableIterable<PartitionData> reader =
+ InternalData.read(fileFormat, inputFile)
+ .project(simpleSchema())
+ .setRootType(PartitionData.class)
+ .build()) {
+ for (PartitionData record : reader) {
+ readRecords.add(record);
+ }
+ }
+
+ assertThat(readRecords).hasSize(testData.size());
+
+ for (int i = 0; i < testData.size(); i++) {
+ Record expected = testData.get(i);
+ PartitionData actual = readRecords.get(i);
+
+ assertThat(actual.get(0, Long.class)).isEqualTo(expected.get(0, Long.class));
+ assertThat(actual.get(1, String.class)).isEqualTo(expected.get(1, String.class));
+ }
+ }
+
+ @TestTemplate
+ public void testCustomTypeForNestedField() throws IOException {
+ FileFormat fileFormat = FileFormat.fromString(format);
+ OutputFile outputFile = fileIO.newOutputFile(tempDir.resolve("test." + format).toString());
+
+ List<Record> testData = createNestedTestRecords();
+
+ try (FileAppender<Record> appender =
+ InternalData.write(fileFormat, outputFile).schema(NESTED_SCHEMA).build()) {
+ appender.addAll(testData);
+ }
+
+ InputFile inputFile = fileIO.newInputFile(outputFile.location());
+ List<Record> readRecords = Lists.newArrayList();
+
+ try (CloseableIterable<Record> reader =
+ InternalData.read(fileFormat, inputFile)
+ .project(NESTED_SCHEMA)
+ .setCustomType(2, TestCustomRow.class)
+ .build()) {
+ for (Record record : reader) {
+ readRecords.add(record);
+ }
+ }
+
+ assertThat(readRecords).hasSize(testData.size());
+
+ for (int i = 0; i < testData.size(); i++) {
+ Record expected = testData.get(i);
+ Record actual = readRecords.get(i);
+
+ assertThat(actual.get(0, Long.class)).isEqualTo(expected.get(0, Long.class));
+
+ Object expectedNested = expected.get(1);
+ Object actualNested = actual.get(1);
+
+ if (expectedNested == null && actualNested == null) {
+ continue;
+ }
+
+ if (actualNested != null) {
Review Comment:
Changed this to check that actualNested is null if expected is null, and not-null otherwise.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2359578242
##########
parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java:
##########
@@ -1161,6 +1160,61 @@ public static class ReadBuilder implements InternalData.ReadBuilder {
private NameMapping nameMapping = null;
private ByteBuffer fileEncryptionKey = null;
private ByteBuffer fileAADPrefix = null;
+ private Class<? extends StructLike> rootType = null;
+ private Map<Integer, Class<? extends StructLike>> customTypes = Maps.newHashMap();
+
+ public interface ReaderFunction {
Review Comment:
Cool! Looks nice.
Thanks @RussellSpitzer!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "gaborkaszab (via GitHub)" <gi...@apache.org>.
gaborkaszab commented on PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#issuecomment-3366297727
Hey @RussellSpitzer ,
We discussed with @pvary the other day that the same enhancements for the Parquet writers, and also for Avro and Orc readers/writers would be useful for the file format API change that he has been working on. I just wanted to throw the idea in to see if there are any obvious reasons not to make those changes or in case you have any objections against. If you think it makes sense, I can take a deeper look. Thanks!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "gaborkaszab (via GitHub)" <gi...@apache.org>.
gaborkaszab commented on PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#issuecomment-3390819360
Thanks for your answer, @stevenzwu ! The motivation here is slightly different: @pvary found that having the same enhancement for all the readers/writers could help him simplify his file format API PR.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#issuecomment-3390667055
@gaborkaszab Assuming you are talking about porting this PR for custom types to Avro and Orc. This PR is a prep work to support writing manifest file in Parquet format. Avro already supports custom types for writing manifest file. There is no plan to support writing manifest in Orc.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2359953906
##########
core/src/test/java/org/apache/iceberg/TestInternalData.java:
##########
@@ -0,0 +1,245 @@
+/*
+ * 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 java.io.IOException;
+import java.nio.file.Path;
+import java.util.Arrays;
+import java.util.List;
+import org.apache.iceberg.data.GenericRecord;
+import org.apache.iceberg.data.Record;
+import org.apache.iceberg.io.CloseableIterable;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.io.FileIO;
+import org.apache.iceberg.io.InputFile;
+import org.apache.iceberg.io.OutputFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.io.TempDir;
+
+@ExtendWith(ParameterizedTestExtension.class)
+public class TestInternalData {
+
+ @Parameter(index = 0)
+ private String format;
+
+ @Parameters(name = "format={0}")
+ public static Object[][] parameters() {
+ return new Object[][] {{"avro"}, {"parquet"}};
+ }
+
+ private static final Schema NESTED_SCHEMA =
+ new Schema(
+ Types.NestedField.required(1, "outer_id", Types.LongType.get()),
+ Types.NestedField.optional(
+ 2,
+ "nested_struct",
+ Types.StructType.of(
+ Types.NestedField.optional(3, "inner_id", Types.LongType.get()),
+ Types.NestedField.optional(4, "inner_name", Types.StringType.get()))));
+
+ @TempDir private Path tempDir;
+
+ private final FileIO fileIO = new TestTables.LocalFileIO();
+
+ @TestTemplate
+ public void testCustomRootType() throws IOException {
+ FileFormat fileFormat = FileFormat.fromString(format);
+ OutputFile outputFile = fileIO.newOutputFile(tempDir.resolve("test." + format).toString());
+
+ List<Record> testData = createSimpleTestRecords();
+
+ try (FileAppender<Record> appender =
+ InternalData.write(fileFormat, outputFile).schema(simpleSchema()).build()) {
+ appender.addAll(testData);
+ }
+
+ InputFile inputFile = fileIO.newInputFile(outputFile.location());
+ List<PartitionData> readRecords = Lists.newArrayList();
+
+ try (CloseableIterable<PartitionData> reader =
+ InternalData.read(fileFormat, inputFile)
+ .project(simpleSchema())
+ .setRootType(PartitionData.class)
+ .build()) {
+ for (PartitionData record : reader) {
+ readRecords.add(record);
+ }
+ }
+
+ assertThat(readRecords).hasSize(testData.size());
+
+ for (int i = 0; i < testData.size(); i++) {
+ Record expected = testData.get(i);
+ PartitionData actual = readRecords.get(i);
+
+ assertThat(actual.get(0, Long.class)).isEqualTo(expected.get(0, Long.class));
+ assertThat(actual.get(1, String.class)).isEqualTo(expected.get(1, String.class));
+ }
+ }
+
+ @TestTemplate
+ public void testCustomTypeForNestedField() throws IOException {
+ FileFormat fileFormat = FileFormat.fromString(format);
+ OutputFile outputFile = fileIO.newOutputFile(tempDir.resolve("test." + format).toString());
+
+ List<Record> testData = createNestedTestRecords();
+
+ try (FileAppender<Record> appender =
+ InternalData.write(fileFormat, outputFile).schema(NESTED_SCHEMA).build()) {
+ appender.addAll(testData);
+ }
+
+ InputFile inputFile = fileIO.newInputFile(outputFile.location());
+ List<Record> readRecords = Lists.newArrayList();
+
+ try (CloseableIterable<Record> reader =
+ InternalData.read(fileFormat, inputFile)
+ .project(NESTED_SCHEMA)
+ .setCustomType(2, TestCustomRow.class)
+ .build()) {
+ for (Record record : reader) {
+ readRecords.add(record);
+ }
+ }
+
+ assertThat(readRecords).hasSize(testData.size());
+
+ for (int i = 0; i < testData.size(); i++) {
+ Record expected = testData.get(i);
+ Record actual = readRecords.get(i);
+
+ assertThat(actual.get(0, Long.class)).isEqualTo(expected.get(0, Long.class));
+
+ Object expectedNested = expected.get(1);
+ Object actualNested = actual.get(1);
+
+ if (expectedNested == null && actualNested == null) {
+ continue;
+ }
+
+ if (actualNested != null) {
Review Comment:
Good point!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2356652587
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetValueReaders.java:
##########
@@ -189,6 +191,16 @@ public static ParquetValueReader<Long> int96Timestamps(ColumnDescriptor desc) {
return new TimestampInt96Reader(desc);
}
+ @SuppressWarnings("unchecked")
+ public static <T extends StructLike> ParquetValueReader<T> structLikeReader(
+ List<ParquetValueReader<?>> readers, Types.StructType struct, Class<T> structClass) {
+ if (structClass.equals(Record.class)) {
Review Comment:
Yeah I thought about this as well, I didn't want to break that since someone had spent the time to add in the comment.
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetValueReaders.java:
##########
@@ -189,6 +191,16 @@ public static ParquetValueReader<Long> int96Timestamps(ColumnDescriptor desc) {
return new TimestampInt96Reader(desc);
}
+ @SuppressWarnings("unchecked")
+ public static <T extends StructLike> ParquetValueReader<T> structLikeReader(
+ List<ParquetValueReader<?>> readers, Types.StructType struct, Class<T> structClass) {
+ if (structClass.equals(Record.class)) {
Review Comment:
Yeah I thought about this as well, I didn't want to remove the interface since someone had spent the time to add in the comment.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2356628434
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/InternalReader.java:
##########
@@ -72,4 +91,12 @@ protected ParquetValueReader<?> timeReader(ColumnDescriptor desc) {
protected ParquetValueReader<?> timestampReader(ColumnDescriptor desc, boolean isAdjustedToUTC) {
return ParquetValueReaders.timestamps(desc);
}
+
+ public void setCustomType(int fieldId, Class<? extends StructLike> type) {
+ this.typesById.put(fieldId, type);
+ }
+
+ public void setRootType(Class<? extends StructLike> rootClass) {
+ setCustomType(ROOT_ID, rootClass);
Review Comment:
Removed do to other refactors
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/InternalReader.java:
##########
@@ -72,4 +91,12 @@ protected ParquetValueReader<?> timeReader(ColumnDescriptor desc) {
protected ParquetValueReader<?> timestampReader(ColumnDescriptor desc, boolean isAdjustedToUTC) {
return ParquetValueReaders.timestamps(desc);
}
+
+ public void setCustomType(int fieldId, Class<? extends StructLike> type) {
+ this.typesById.put(fieldId, type);
+ }
+
+ public void setRootType(Class<? extends StructLike> rootClass) {
+ setCustomType(ROOT_ID, rootClass);
Review Comment:
Removed due to other refactors
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2359333485
##########
parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java:
##########
@@ -1281,12 +1304,24 @@ public ReadBuilder withNameMapping(NameMapping newNameMapping) {
@Override
public ReadBuilder setRootType(Class<? extends StructLike> rootClass) {
- throw new UnsupportedOperationException("Custom types are not yet supported");
+ Preconditions.checkArgument(
+ this.internalReader != null, "Cannot set root type without using an Internal Reader");
+ Preconditions.checkArgument(
+ this.readerFunc == null && this.readerFuncWithSchema == null,
+ "Setting root type is not compatible with setting a reader function");
+ internalReader.setRootType(rootClass);
Review Comment:
We directly use the Iceberg `schema` in the readers for column pruning and filter pushdown, so we should store that in both the ReadBuilder, and in the ParquetReaderFunction. Other than that, it could work.
I think we should deprecate and remove the old setters for readerFunctions/writerFunctions which could help removing the clutter around this.
We have to decide if we really want to formalize the ReaderFunction/WriterFunction API.
Currently I have this in the FileFormat API proposal:
```
new ParquetFormatModel<InternalRow, StructType, DeleteFilter<InternalRow>>(
InternalRow.class,
(schema, messageType, constantFieldAccessors) ->
SparkParquetReaders.buildReader(...),
(icebergSchema, messageType, inputType) ->
SparkParquetWriters.buildWriter(
SparkSchemaUtil.convert(icebergSchema), messageType))
new ParquetFormatModel<ColumnarBatch, StructType, DeleteFilter<InternalRow>>(
ColumnarBatch.class,
(schema, messageType, constantFieldAccessors, deleteFilter, config) ->
VectorizedSparkParquetReaders.buildReader(...))
```
We can formalize the ReaderFunction/WriterFunction, but I have the following concerns:
- This will be inherently FileFormat specific (See: messageType, deleteFilter, config)
- Always prone to adding more fields - I don't see why we would not share any of the builder parameters
I still prefer to have the readerFunctions/writerFunctions, as this makes clear which of the parameters are really used there, but can be convinced otherwise.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2337211131
##########
parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java:
##########
@@ -1281,12 +1304,24 @@ public ReadBuilder withNameMapping(NameMapping newNameMapping) {
@Override
public ReadBuilder setRootType(Class<? extends StructLike> rootClass) {
- throw new UnsupportedOperationException("Custom types are not yet supported");
+ Preconditions.checkArgument(
+ this.internalReader != null, "Cannot set root type without using an Internal Reader");
+ Preconditions.checkArgument(
+ this.readerFunc == null && this.readerFuncWithSchema == null,
+ "Setting root type is not compatible with setting a reader function");
+ internalReader.setRootType(rootClass);
Review Comment:
The issues here are mostly centered around the "readerFunc" apis. We want to use "internalReader" to generate our reader func since it needs to be built based on InteralReader and BaseParquetReader. So we need some marker to let Parquet.java know it should be using that class to build the reader func.
The previous code just set the reader func immediately upon using the InternalData pathway but that meant we would basically have to assume that if you call Root or CustomClass that we are able to replace that readerFunc with one built from internal data. I felt like this was a bit more straightforward.
Ie Previously
User calls InternalData read parquet
InternalData calls Parquet.read(...).withReaderFunct(() -> InternalReader.create())
Which gives us back
ReadBuilder {
readerFunc = Lambda
}
Then when you call set custom data we would have to do this
setRootType() {
// Assume readerFunc Lambda was created by InternalReader so reassign it
readerFunc = () ->> InternalReader.create(..., customClassMap)
}
Or we could mess up the actual assignment logic where we check if these functions are set, and reset there
or reset with an internal reader there. I'm not sure any of these are really great solutions though.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2337236843
##########
parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java:
##########
@@ -1281,12 +1304,24 @@ public ReadBuilder withNameMapping(NameMapping newNameMapping) {
@Override
public ReadBuilder setRootType(Class<? extends StructLike> rootClass) {
- throw new UnsupportedOperationException("Custom types are not yet supported");
+ Preconditions.checkArgument(
+ this.internalReader != null, "Cannot set root type without using an Internal Reader");
+ Preconditions.checkArgument(
+ this.readerFunc == null && this.readerFuncWithSchema == null,
+ "Setting root type is not compatible with setting a reader function");
+ internalReader.setRootType(rootClass);
Review Comment:
Internal Reader flag is just an example in the above example to signify that the Parquet.java read builder should use the internal reader to create the reader func instead of ... something else
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2337453015
##########
core/src/test/java/org/apache/iceberg/TestInternalData.java:
##########
@@ -0,0 +1,245 @@
+/*
+ * 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 java.io.IOException;
+import java.nio.file.Path;
+import java.util.Arrays;
+import java.util.List;
+import org.apache.iceberg.data.GenericRecord;
+import org.apache.iceberg.data.Record;
+import org.apache.iceberg.io.CloseableIterable;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.io.FileIO;
+import org.apache.iceberg.io.InputFile;
+import org.apache.iceberg.io.OutputFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.io.TempDir;
+
+@ExtendWith(ParameterizedTestExtension.class)
+public class TestInternalData {
+
+ @Parameter(index = 0)
+ private String format;
+
+ @Parameters(name = "format={0}")
+ public static Object[][] parameters() {
+ return new Object[][] {{"avro"}, {"parquet"}};
+ }
+
+ private static final Schema NESTED_SCHEMA =
+ new Schema(
+ Types.NestedField.required(1, "outer_id", Types.LongType.get()),
+ Types.NestedField.optional(
+ 2,
+ "nested_struct",
+ Types.StructType.of(
+ Types.NestedField.optional(3, "inner_id", Types.LongType.get()),
+ Types.NestedField.optional(4, "inner_name", Types.StringType.get()))));
+
+ @TempDir private Path tempDir;
+
+ private final FileIO fileIO = new TestTables.LocalFileIO();
+
+ @TestTemplate
+ public void testCustomRootType() throws IOException {
+ FileFormat fileFormat = FileFormat.fromString(format);
+ OutputFile outputFile = fileIO.newOutputFile(tempDir.resolve("test." + format).toString());
+
+ List<Record> testData = createSimpleTestRecords();
+
+ try (FileAppender<Record> appender =
+ InternalData.write(fileFormat, outputFile).schema(simpleSchema()).build()) {
+ appender.addAll(testData);
+ }
+
+ InputFile inputFile = fileIO.newInputFile(outputFile.location());
+ List<PartitionData> readRecords = Lists.newArrayList();
+
+ try (CloseableIterable<PartitionData> reader =
+ InternalData.read(fileFormat, inputFile)
+ .project(simpleSchema())
+ .setRootType(PartitionData.class)
+ .build()) {
+ for (PartitionData record : reader) {
+ readRecords.add(record);
+ }
+ }
+
+ assertThat(readRecords).hasSize(testData.size());
+
+ for (int i = 0; i < testData.size(); i++) {
+ Record expected = testData.get(i);
+ PartitionData actual = readRecords.get(i);
+
+ assertThat(actual.get(0, Long.class)).isEqualTo(expected.get(0, Long.class));
+ assertThat(actual.get(1, String.class)).isEqualTo(expected.get(1, String.class));
+ }
+ }
+
+ @TestTemplate
+ public void testCustomTypeForNestedField() throws IOException {
+ FileFormat fileFormat = FileFormat.fromString(format);
+ OutputFile outputFile = fileIO.newOutputFile(tempDir.resolve("test." + format).toString());
+
+ List<Record> testData = createNestedTestRecords();
+
+ try (FileAppender<Record> appender =
+ InternalData.write(fileFormat, outputFile).schema(NESTED_SCHEMA).build()) {
+ appender.addAll(testData);
+ }
+
+ InputFile inputFile = fileIO.newInputFile(outputFile.location());
+ List<Record> readRecords = Lists.newArrayList();
+
+ try (CloseableIterable<Record> reader =
+ InternalData.read(fileFormat, inputFile)
+ .project(NESTED_SCHEMA)
+ .setCustomType(2, TestCustomRow.class)
+ .build()) {
+ for (Record record : reader) {
+ readRecords.add(record);
+ }
+ }
+
+ assertThat(readRecords).hasSize(testData.size());
+
+ for (int i = 0; i < testData.size(); i++) {
+ Record expected = testData.get(i);
+ Record actual = readRecords.get(i);
+
+ assertThat(actual.get(0, Long.class)).isEqualTo(expected.get(0, Long.class));
+
+ Object expectedNested = expected.get(1);
+ Object actualNested = actual.get(1);
+
+ if (expectedNested == null && actualNested == null) {
+ continue;
+ }
+
+ if (actualNested != null) {
+ assertThat(actualNested)
+ .as("Custom type should be TestCustomRow, but was: " + actualNested.getClass())
+ .isInstanceOf(TestCustomRow.class);
+ TestCustomRow customRow = (TestCustomRow) actualNested;
+ Record expectedRecord = (Record) expectedNested;
+
+ assertThat(customRow.get(0, Long.class))
+ .isEqualTo(expectedRecord.get(0, Long.class)); // inner_id
+ assertThat(customRow.get(1, String.class))
+ .isEqualTo(expectedRecord.get(1, String.class)); // inner_name
+ }
+ }
+ }
+
+ private List<Record> createSimpleTestRecords() {
Review Comment:
That's fair, the only important thing is having a few nulls
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Core: Allows Internal Parquet Readers to use Custom Types [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #14040:
URL: https://github.com/apache/iceberg/pull/14040#discussion_r2384315183
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/BaseParquetReaders.java:
##########
@@ -53,6 +53,9 @@
import org.apache.parquet.schema.Type;
abstract class BaseParquetReaders<T> {
+ // Root ID is used for the top-level struct in the Parquet Schema
+ protected static final int ROOT_ID = -1;
Review Comment:
Is the line above this not sufficent?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org