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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
