nandorKollar commented on code in PR #13562:
URL: https://github.com/apache/iceberg/pull/13562#discussion_r2210680131
##########
arrow/src/main/java/org/apache/iceberg/arrow/vectorized/VectorizedArrowReader.java:
##########
@@ -180,6 +184,7 @@ public VectorHolder read(VectorHolder reuse, int
numValsToRead) {
.nextBatch(vec, typeWidth, nullabilityHolder);
break;
case TIMESTAMP_MILLIS:
+ case TIMESTAMP_NANOS:
vectorizedColumnIterator
.timestampMillisBatchReader()
Review Comment:
Millis batch reader should work here, as the annotation can be added to the
same physical types. Nevertheless, 'millis' in the method naming is misleading
here.
##########
arrow/src/main/java/org/apache/iceberg/arrow/vectorized/VectorizedArrowReader.java:
##########
@@ -213,8 +218,10 @@ private void allocateFieldVector(boolean
dictionaryEncodedVector) {
allocateDictEncodedVector();
} else {
Field arrowField =
ArrowSchemaUtil.convert(getPhysicalType(columnDescriptor, icebergField));
- if (columnDescriptor.getPrimitiveType().getOriginalType() != null) {
- allocateVectorBasedOnOriginalType(columnDescriptor.getPrimitiveType(),
arrowField);
+ if (columnDescriptor.getPrimitiveType().getLogicalTypeAnnotation() !=
null
+ && !(columnDescriptor.getPrimitiveType().getLogicalTypeAnnotation()
+ instanceof LogicalTypeAnnotation.UUIDLogicalTypeAnnotation)) {
+
allocateVectorBasedOnLogicalTypeAnnotation(columnDescriptor.getPrimitiveType(),
arrowField);
Review Comment:
You mean why use logical type annotations instead of original types? In
parquet-format, logical types were represented as
[ConvertedTypes](https://github.com/apache/parquet-format/blob/master/src/main/thrift/parquet.thrift#L536),
which is called in parquet-mr OriginalType. As it was an enum, it didn't give
sufficient flexibility, there was no way to properly represent logical type
parameters, like for example decimal precision and scale, time unit for
timestamps etc, therefore it was replaced with a union of structs called
[LogicalType](https://github.com/apache/parquet-format/blob/master/src/main/thrift/parquet.thrift#L471).
New types, like variant, geometry don't have a proper OriginalType
representation, only logical types. Nanosecond precision timestamp type falls
into this category:
https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#deprecated-timestamp-convertedtype.
##########
arrow/src/main/java/org/apache/iceberg/arrow/vectorized/VectorizedArrowReader.java:
##########
@@ -213,8 +218,10 @@ private void allocateFieldVector(boolean
dictionaryEncodedVector) {
allocateDictEncodedVector();
} else {
Field arrowField =
ArrowSchemaUtil.convert(getPhysicalType(columnDescriptor, icebergField));
- if (columnDescriptor.getPrimitiveType().getOriginalType() != null) {
- allocateVectorBasedOnOriginalType(columnDescriptor.getPrimitiveType(),
arrowField);
+ if (columnDescriptor.getPrimitiveType().getLogicalTypeAnnotation() !=
null
+ && !(columnDescriptor.getPrimitiveType().getLogicalTypeAnnotation()
+ instanceof LogicalTypeAnnotation.UUIDLogicalTypeAnnotation)) {
Review Comment:
I added this extra `instanceof
LogicalTypeAnnotation.UUIDLogicalTypeAnnotation`, because the logic, which
relied on original types didn't handle UUID. It doesn't have an original type
representation, only logical type annotation exists.
--
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]
