Re: [PR] Parquet: Clean up Parquet generic and internal readers [iceberg]

[via GitHub]

rdblue commented on code in PR #12102:
URL: https://github.com/apache/iceberg/pull/12102#discussion_r1932642614


##########
parquet/src/main/java/org/apache/iceberg/data/parquet/BaseParquetReaders.java:
##########
@@ -148,96 +167,81 @@ public ParquetValueReader<?> struct(
     }
   }
 
-  private class LogicalTypeAnnotationParquetValueReaderVisitor
-      implements 
LogicalTypeAnnotation.LogicalTypeAnnotationVisitor<ParquetValueReader<?>> {
+  private class LogicalTypeReadBuilder
+      implements LogicalTypeAnnotationVisitor<ParquetValueReader<?>> {
 
     private final ColumnDescriptor desc;
     private final org.apache.iceberg.types.Type.PrimitiveType expected;
-    private final PrimitiveType primitive;
 
-    LogicalTypeAnnotationParquetValueReaderVisitor(
-        ColumnDescriptor desc,
-        org.apache.iceberg.types.Type.PrimitiveType expected,
-        PrimitiveType primitive) {
+    LogicalTypeReadBuilder(
+        ColumnDescriptor desc, org.apache.iceberg.types.Type.PrimitiveType 
expected) {
       this.desc = desc;
       this.expected = expected;
-      this.primitive = primitive;
     }
 
     @Override
-    public Optional<ParquetValueReader<?>> visit(
-        LogicalTypeAnnotation.StringLogicalTypeAnnotation stringLogicalType) {
-      return Optional.of(new ParquetValueReaders.StringReader(desc));
+    public Optional<ParquetValueReader<?>> visit(StringLogicalTypeAnnotation 
stringLogicalType) {
+      return Optional.of(ParquetValueReaders.strings(desc));
     }
 
     @Override
-    public Optional<ParquetValueReader<?>> visit(
-        LogicalTypeAnnotation.EnumLogicalTypeAnnotation enumLogicalType) {
-      return Optional.of(new ParquetValueReaders.StringReader(desc));
+    public Optional<ParquetValueReader<?>> visit(EnumLogicalTypeAnnotation 
enumLogicalType) {
+      return Optional.of(ParquetValueReaders.strings(desc));
     }
 
     @Override
     public Optional<ParquetValueReader<?>> visit(DecimalLogicalTypeAnnotation 
decimalLogicalType) {
-      switch (primitive.getPrimitiveTypeName()) {
-        case BINARY:
-        case FIXED_LEN_BYTE_ARRAY:
-          return Optional.of(
-              new ParquetValueReaders.BinaryAsDecimalReader(desc, 
decimalLogicalType.getScale()));
-        case INT64:
-          return Optional.of(
-              new ParquetValueReaders.LongAsDecimalReader(desc, 
decimalLogicalType.getScale()));
-        case INT32:
-          return Optional.of(
-              new ParquetValueReaders.IntegerAsDecimalReader(desc, 
decimalLogicalType.getScale()));
-        default:
-          throw new UnsupportedOperationException(
-              "Unsupported base type for decimal: " + 
primitive.getPrimitiveTypeName());
-      }
+      return Optional.of(ParquetValueReaders.bigDecimals(desc));
     }
 
     @Override
-    public Optional<ParquetValueReader<?>> visit(
-        LogicalTypeAnnotation.DateLogicalTypeAnnotation dateLogicalType) {
+    public Optional<ParquetValueReader<?>> visit(DateLogicalTypeAnnotation 
dateLogicalType) {
       return Optional.of(dateReader(desc));
     }
 
     @Override
-    public Optional<ParquetValueReader<?>> visit(
-        LogicalTypeAnnotation.TimeLogicalTypeAnnotation timeLogicalType) {
-      return Optional.of(timeReader(desc, timeLogicalType.getUnit()));
+    public Optional<ParquetValueReader<?>> visit(TimeLogicalTypeAnnotation 
timeLogicalType) {
+      return Optional.of(timeReader(desc));
     }
 
     @Override
     public Optional<ParquetValueReader<?>> visit(
-        LogicalTypeAnnotation.TimestampLogicalTypeAnnotation 
timestampLogicalType) {
+        TimestampLogicalTypeAnnotation timestampLogicalType) {
       return Optional.of(
-          timestampReader(
-              desc,
-              timestampLogicalType.getUnit(),
-              ((Types.TimestampType) expected).shouldAdjustToUTC()));
+          timestampReader(desc, ((Types.TimestampType) 
expected).shouldAdjustToUTC()));
     }
 
     @Override
-    public Optional<ParquetValueReader<?>> visit(
-        LogicalTypeAnnotation.IntLogicalTypeAnnotation intLogicalType) {
+    public Optional<ParquetValueReader<?>> visit(IntLogicalTypeAnnotation 
intLogicalType) {
       if (intLogicalType.getBitWidth() == 64) {
+        if (intLogicalType.isSigned()) {
+          // this will throw an UnsupportedOperationException

Review Comment:
   Updated.



-- 
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-unsubscr...@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org