RussellSpitzer commented on code in PR #3711:
URL: https://github.com/apache/parquet-java/pull/3711#discussion_r3961838079
##########
parquet-column/src/main/java/org/apache/parquet/schema/Types.java:
##########
@@ -590,9 +632,15 @@ public Optional<Boolean> visit(
return checkBinaryPrimitiveType(geographyLogicalType);
}
+ private void checkAnnotation(boolean valid, String message,
Object... args) {
Review Comment:
I'd generally want to avoid passing a "boolean" into the "checkAnnotation"
method. Since it doesn't actually check an annotation here, it just throws an
exception if you pass through is false. I think this is a bit over-indexing on
the previous "checkState" work and possibly hiding some other holes.
I would probably try to re-think the whole visitor here rather than try to
wrap a previous failure differently.
Instead of what we have here maybe something like
AllowedPhysicalTypes visitor
```java
/**
* The physical types a logical type annotation may annotate. An empty set
means the annotation
* cannot be applied to a primitive type at all.
*/
private static final class AllowedPhysicalTypes {
private static final AllowedPhysicalTypes NONE =
new AllowedPhysicalTypes(EnumSet.noneOf(PrimitiveTypeName.class),
NOT_SET);
private static final AllowedPhysicalTypes ANY =
new AllowedPhysicalTypes(EnumSet.allOf(PrimitiveTypeName.class),
NOT_SET);
private final Set<PrimitiveTypeName> types;
private final int requiredLength;
static Optional<AllowedPhysicalTypes> of(PrimitiveTypeName... types) {
return Optional.of(new
AllowedPhysicalTypes(EnumSet.copyOf(asList(types)), NOT_SET));
}
static Optional<AllowedPhysicalTypes> fixed(int requiredLength) {
return Optional.of(new AllowedPhysicalTypes(
EnumSet.of(PrimitiveTypeName.FIXED_LEN_BYTE_ARRAY), requiredLength));
}
boolean accepts(PrimitiveTypeName type, int length) {
return types.contains(type) && (requiredLength == NOT_SET || length ==
requiredLength);
}
@Override
public String toString() {
if (requiredLength != NOT_SET) {
return PrimitiveTypeName.FIXED_LEN_BYTE_ARRAY + "(" + requiredLength +
")";
}
return types.stream().map(Enum::name).collect(Collectors.joining(", "));
}
}
```
```java
private static final LogicalTypeAnnotationVisitor<AllowedPhysicalTypes>
ALLOWED_PHYSICAL_TYPES =
new LogicalTypeAnnotationVisitor<AllowedPhysicalTypes>() {
@Override public Optional<AllowedPhysicalTypes>
visit(StringLogicalTypeAnnotation t) { return of(BINARY); }
@Override public Optional<AllowedPhysicalTypes>
visit(JsonLogicalTypeAnnotation t) { return of(BINARY); }
@Override public Optional<AllowedPhysicalTypes>
visit(BsonLogicalTypeAnnotation t) { return of(BINARY); }
@Override public Optional<AllowedPhysicalTypes>
visit(EnumLogicalTypeAnnotation t) { return of(BINARY); }
@Override public Optional<AllowedPhysicalTypes>
visit(GeometryLogicalTypeAnnotation t) { return of(BINARY); }
@Override public Optional<AllowedPhysicalTypes>
visit(GeographyLogicalTypeAnnotation t) { return of(BINARY); }
@Override public Optional<AllowedPhysicalTypes>
visit(DateLogicalTypeAnnotation t) { return of(INT32); }
@Override public Optional<AllowedPhysicalTypes>
visit(TimestampLogicalTypeAnnotation t) { return of(INT64); }
@Override public Optional<AllowedPhysicalTypes>
visit(UUIDLogicalTypeAnnotation t) { return
fixed(UUIDLogicalTypeAnnotation.BYTES); }
@Override public Optional<AllowedPhysicalTypes>
visit(Float16LogicalTypeAnnotation t) { return
fixed(Float16LogicalTypeAnnotation.BYTES); }
@Override public Optional<AllowedPhysicalTypes>
visit(IntervalLogicalTypeAnnotation t) { return fixed(12); }
@Override public Optional<AllowedPhysicalTypes>
visit(TimeLogicalTypeAnnotation t) {
return t.getUnit() == TimeUnit.MILLIS ? of(INT32) : of(INT64);
}
@Override public Optional<AllowedPhysicalTypes>
visit(IntLogicalTypeAnnotation t) {
return t.getBitWidth() == 64 ? of(INT64) : of(INT32);
}
@Override public Optional<AllowedPhysicalTypes>
visit(DecimalLogicalTypeAnnotation t) {
return of(INT32, INT64, BINARY, FIXED_LEN_BYTE_ARRAY);
}
@Override public Optional<AllowedPhysicalTypes>
visit(UnknownLogicalTypeAnnotation t) {
return Optional.of(ANY);
}
};
```
Then instead of trying to throw our way to control flow we can do something
like
```java
if (logicalTypeAnnotation != null) {
AllowedPhysicalTypes allowed =
logicalTypeAnnotation.accept(ALLOWED_PHYSICAL_TYPES).orElse(AllowedPhysicalTypes.NONE);
if (!allowed.accepts(primitiveType, length)) {
if (!dropUnsupportedLogicalAnnotations) {
throw new IllegalStateException(allowed.isEmpty()
? logicalTypeAnnotation + " can not be applied to a primitive type"
: String.format("%s can only annotate %s", logicalTypeAnnotation,
allowed));
}
LOGGER.warn(
"Dropping unsupported logical type annotation {} on physical type
{}",
logicalTypeAnnotation, primitiveType);
return new PrimitiveType(
repetition, primitiveType, this.length, name, null, null, id,
ColumnOrder.undefined());
}
validateDecimalPrecision(meta);
}
```
--
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]
