Parquet, Core: Allows Internal Parquet Readers to use Custom Types #14040

RussellSpitzer · 2025-09-09T21:54:31Z

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.

parquet/src/main/java/org/apache/iceberg/data/parquet/BaseParquetReaders.java

RussellSpitzer · 2025-09-09T21:58:50Z

core/src/test/java/org/apache/iceberg/TestInternalData.java

+import org.junit.jupiter.api.io.TempDir;
+
+@ExtendWith(ParameterizedTestExtension.class)
+public class TestInternalData {


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.

RussellSpitzer · 2025-09-09T22:09:37Z

parquet/src/main/java/org/apache/iceberg/data/parquet/InternalReader.java

 import org.apache.parquet.column.ColumnDescriptor;
 import org.apache.parquet.schema.MessageType;

 public class InternalReader<T extends StructLike> extends BaseParquetReaders<T> {


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.

RussellSpitzer · 2025-09-09T22:11:36Z

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.

RussellSpitzer · 2025-09-09T22:14:09Z

parquet/src/main/java/org/apache/iceberg/InternalParquet.java


  private static Parquet.ReadBuilder readInternal(InputFile inputFile) {
-    return Parquet.read(inputFile).createReaderFunc(InternalReader::create);
+    return Parquet.read(inputFile).useInternalReader(new InternalReader<>());


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.

stevenzwu · 2025-09-09T23:50:02Z

@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.

stevenzwu

some early comments from a quick glance. will look deeper

core/src/test/java/org/apache/iceberg/TestInternalData.java

parquet/src/main/java/org/apache/iceberg/data/parquet/BaseParquetReaders.java

parquet/src/main/java/org/apache/iceberg/parquet/ParquetValueReaders.java

core/src/test/java/org/apache/iceberg/TestInternalData.java

parquet/src/main/java/org/apache/iceberg/parquet/ParquetValueReaders.java

parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java

parquet/src/main/java/org/apache/iceberg/data/parquet/InternalReader.java

core/src/test/java/org/apache/iceberg/TestInternalData.java

parquet/src/main/java/org/apache/iceberg/data/parquet/InternalReader.java

parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java

parquet/src/main/java/org/apache/iceberg/data/parquet/BaseParquetReaders.java

RussellSpitzer · 2025-09-10T16:06:05Z

@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.

The key thing is implementing two interfaces which previously returned unsupported operation exception

parquet/src/main/java/org/apache/iceberg/data/parquet/InternalReader.java

parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java

RussellSpitzer · 2025-09-17T21:28:50Z

api/src/test/java/org/apache/iceberg/TestHelpers.java

    public static CustomRow of(Object... values) {
      return new CustomRow(values);
    }



Constructors so we can use Custom Row as a CustomType in internal Data

parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java

RussellSpitzer · 2025-09-17T21:33:20Z

parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java

+      }
+    }
+
+    private static class UnaryReaderFunction implements ReaderFunction {


This is an instance of the original readerFunc which takes only messageType as an arg.

stevenzwu

just some nit comments. This looks pretty good to me

stevenzwu · 2025-09-17T22:35:14Z

parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java

-          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");


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.

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

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.

parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java

parquet/src/main/java/org/apache/iceberg/data/parquet/InternalReader.java

core/src/test/java/org/apache/iceberg/TestInternalData.java

stevenzwu

LGTM. great work on solving the reader function setter problem

pvary

Thanks @RussellSpitzer!
Nice solution!

nastra · 2025-09-23T09:41:41Z

.palantir/revapi.yml

        \ java.util.List<org.apache.iceberg.PositionDeletesScanTask>)"
      justification: "Removing deprecations for 1.10.0"
    org.apache.iceberg:iceberg-parquet:
+    - code: "java.method.parameterTypeChanged"


I don't think any of these RevAPI changes are actually needed anymore. I've removed them locally and things passed

They still fail on my machine, let me try rebasing

Rebasing fixed this!

parquet/src/main/java/org/apache/iceberg/data/parquet/BaseParquetReaders.java

nastra · 2025-09-23T09:48:58Z

parquet/src/main/java/org/apache/iceberg/data/parquet/BaseParquetReaders.java

+  @Deprecated
+  protected ParquetValueReader<T> createStructReader(
+      List<ParquetValueReader<?>> fieldReaders, Types.StructType structType) {
+    throw new UnsupportedOperationException(


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)?

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.

nastra · 2025-09-23T09:51:18Z

parquet/src/main/java/org/apache/iceberg/data/parquet/InternalReader.java

+      }
+
+      @Override
+      public Parquet.ReadBuilder.ReaderFunction withCustomTypes(


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

🤷 Yeah I'm not sure it's worth standardizing since this is only called by Parquet.java anyway

core/src/test/java/org/apache/iceberg/TestInternalData.java

nastra

code changes and tests LGTM but I believe we should be able to remove all of the newly added RevAPI exceptions. Also I added a question around backward comp.

RussellSpitzer · 2025-09-23T18:08:41Z

parquet/src/main/java/org/apache/iceberg/data/parquet/BaseParquetReaders.java

+  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);


@nastra ^ See here (this is what keeps subclasses written by outside users from breaking)

# This is the 1st commit message: Parquet, Core: Allows Internal Parquet Readers to use Custom Types # This is the commit message #2: Remove some test code that is no longer in use # This is the commit message #3: Add Preconditions for double setting Parquet # This is the commit message #4: Reviewer Comments # This is the commit message #5: Reviewer Comments

shangxinli · 2025-09-24T02:05:16Z

parquet/src/main/java/org/apache/iceberg/data/parquet/BaseParquetReaders.java


 abstract class BaseParquetReaders<T> {
+  // Root ID is used for the top-level struct in the Parquet Schema
+  protected static final int ROOT_ID = -1;


Magic number could use documentation

Is the line above this not sufficent?

shangxinli · 2025-09-24T02:09:22Z

parquet/src/main/java/org/apache/iceberg/parquet/ParquetValueReaders.java

+        List<ParquetValueReader<?>> readers, Types.StructType struct, Class<T> structLikeClass) {
+      super(readers);
+      this.struct = struct;
+      this.ctor =


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);
}

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.

RussellSpitzer · 2025-09-30T04:04:38Z

Merged! Thanks everyone for your thoughtful reviews!

…pache#14040) Parquet, Core: Allows Internal Parquet Readers to use Custom Types

gaborkaszab · 2025-10-03T16:05:30Z

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!

gaborkaszab · 2025-10-10T14:59:10Z

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!

stevenzwu · 2025-10-10T15:06:47Z

@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.

gaborkaszab · 2025-10-10T15:37:44Z

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.

nastra · 2025-10-10T16:01:12Z

I don't think there's value in adding custom type support to ORC if we're not going to use it anyway?

pvary · 2025-10-10T18:31:34Z

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.

…pache#14040) Parquet, Core: Allows Internal Parquet Readers to use Custom Types

github-actions bot added parquet core build labels Sep 9, 2025

RussellSpitzer commented Sep 9, 2025

View reviewed changes

parquet/src/main/java/org/apache/iceberg/data/parquet/BaseParquetReaders.java Show resolved Hide resolved

RussellSpitzer commented Sep 9, 2025

View reviewed changes

RussellSpitzer requested review from amogh-jahagirdar, manuzhang and rdblue September 9, 2025 22:10

RussellSpitzer requested review from huaxingao and stevenzwu September 9, 2025 22:11

RussellSpitzer commented Sep 9, 2025

View reviewed changes

stevenzwu requested a review from pvary September 9, 2025 22:39

stevenzwu reviewed Sep 10, 2025

View reviewed changes

parquet/src/main/java/org/apache/iceberg/parquet/ParquetValueReaders.java Show resolved Hide resolved

stevenzwu reviewed Sep 10, 2025

View reviewed changes

pvary reviewed Sep 10, 2025

View reviewed changes

core/src/test/java/org/apache/iceberg/TestInternalData.java Outdated Show resolved Hide resolved

pvary reviewed Sep 10, 2025

View reviewed changes

parquet/src/main/java/org/apache/iceberg/data/parquet/InternalReader.java Outdated Show resolved Hide resolved

pvary reviewed Sep 10, 2025

View reviewed changes

parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java Outdated Show resolved Hide resolved

manuzhang reviewed Sep 10, 2025

View reviewed changes

parquet/src/main/java/org/apache/iceberg/data/parquet/BaseParquetReaders.java Outdated Show resolved Hide resolved

huaxingao reviewed Sep 10, 2025

View reviewed changes

parquet/src/main/java/org/apache/iceberg/data/parquet/InternalReader.java Outdated Show resolved Hide resolved

huaxingao reviewed Sep 10, 2025

View reviewed changes

parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java Outdated Show resolved Hide resolved

github-actions bot added the API label Sep 17, 2025

RussellSpitzer commented Sep 17, 2025

View reviewed changes

parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java Show resolved Hide resolved

RussellSpitzer commented Sep 17, 2025

View reviewed changes

stevenzwu reviewed Sep 17, 2025

View reviewed changes

stevenzwu approved these changes Sep 18, 2025

View reviewed changes

pvary approved these changes Sep 19, 2025

View reviewed changes

nastra reviewed Sep 23, 2025

View reviewed changes

parquet/src/main/java/org/apache/iceberg/data/parquet/BaseParquetReaders.java Show resolved Hide resolved

nastra reviewed Sep 23, 2025

View reviewed changes

core/src/test/java/org/apache/iceberg/TestInternalData.java Outdated Show resolved Hide resolved

nastra reviewed Sep 23, 2025

View reviewed changes

core/src/test/java/org/apache/iceberg/TestInternalData.java Outdated Show resolved Hide resolved

nastra reviewed Sep 23, 2025

View reviewed changes

core/src/test/java/org/apache/iceberg/TestInternalData.java Outdated Show resolved Hide resolved

nastra reviewed Sep 23, 2025

View reviewed changes

RussellSpitzer commented Sep 23, 2025

View reviewed changes

RussellSpitzer force-pushed the ParquetInternalData branch from a2a5231 to 2564724 Compare September 23, 2025 18:14

RussellSpitzer added 2 commits September 23, 2025 13:16

Remove revapi changes

5205d85

Reviewer Comments

49fd7d6

shangxinli reviewed Sep 24, 2025

View reviewed changes

nastra approved these changes Sep 24, 2025

View reviewed changes

Typo

3e51b71

RussellSpitzer merged commit f9a4168 into apache:main Sep 30, 2025
43 checks passed

gabeiglio pushed a commit to gabeiglio/iceberg that referenced this pull request Oct 1, 2025

Parquet, Core: Allows Internal Parquet Readers to use Custom Types (a…

bebdc87

…pache#14040) Parquet, Core: Allows Internal Parquet Readers to use Custom Types

adawrapub pushed a commit to adawrapub/iceberg that referenced this pull request Oct 16, 2025

Parquet, Core: Allows Internal Parquet Readers to use Custom Types (a…

b107181

…pache#14040) Parquet, Core: Allows Internal Parquet Readers to use Custom Types

stevenzwu mentioned this pull request Nov 12, 2025

Prototype: Parquet as manifest format for v4 #14577

Closed

Parquet, Core: Allows Internal Parquet Readers to use Custom Types #14040

Parquet, Core: Allows Internal Parquet Readers to use Custom Types #14040

Uh oh!

Conversation

RussellSpitzer commented Sep 9, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RussellSpitzer commented Sep 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevenzwu commented Sep 9, 2025

Uh oh!

stevenzwu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RussellSpitzer commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevenzwu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stevenzwu left a comment

Choose a reason for hiding this comment

Uh oh!

pvary left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RussellSpitzer Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nastra Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

RussellSpitzer commented Sep 10, 2025 •

edited

Loading

RussellSpitzer Sep 23, 2025 •

edited

Loading

nastra Sep 23, 2025 •

edited

Loading

RussellSpitzer Sep 27, 2025 •

edited

Loading