[GitHub] [iceberg] rdblue commented on a change in pull request #2336: Reduce boxing in ParquetValueReaders.StructReader

[GitBox <gi...@apache.org>]

rdblue commented on a change in pull request #2336:
URL: https://github.com/apache/iceberg/pull/2336#discussion_r595605786



##########
File path: parquet/src/main/java/org/apache/iceberg/parquet/ParquetValueReaders.java
##########
@@ -696,8 +738,20 @@ public final T read(T reuse) {
       I intermediate = newStructData(reuse);
 
       for (int i = 0; i < readers.length; i += 1) {
-        set(intermediate, i, readers[i].read(get(intermediate, i)));
-        // setters[i].set(intermediate, i, get(intermediate, i));
+        final ParquetValueReader<?> reader = readers[i];
+        if (reader instanceof ParquetValueReader.OfBoolean) {
+          setBoolean(intermediate, i, ((OfBoolean) reader).readBoolean());
+        } else if (reader instanceof ParquetValueReader.OfInt) {
+          setInteger(intermediate, i, ((OfInt) reader).readInteger());
+        } else if (reader instanceof ParquetValueReader.OfLong) {
+          setLong(intermediate, i, ((OfLong) reader).readLong());
+        } else if (reader instanceof ParquetValueReader.OfFloat) {
+          setFloat(intermediate, i, ((OfFloat) reader).readFloat());
+        } else if (reader instanceof ParquetValueReader.OfDouble) {
+          setDouble(intermediate, i, ((OfDouble) reader).readDouble());
+        } else {
+          set(intermediate, i, reader.read(get(intermediate, i)));
+        }

Review comment:
       I think it would be better to continue using the `setters` produced by `newSetter` here because this logic will test each reader several times. I think that avoiding `if` statements by using a `Setter` implementation will be faster in the long run because it will incur invocation cost but not have near as many branches.
   
   Feel free to run the benchmarks and find which one is better, though. I'd be interested in testing the `setter` approach (uncomment the previous line 700) against 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.

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