rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1927919640
##########
api/src/main/java/org/apache/iceberg/types/TypeUtil.java:
##########
@@ -709,6 +709,10 @@ public T map(Types.MapType map, Supplier<T> keyResult,
Supplier<T> valueResult)
return null;
}
+ public T variant() {
+ return null;
Review Comment:
The `map` and `list` methods have been part of the visitors from the start,
so when new visitors are added, the author decides what to do for `map` and
`list`. Now that `variant` is added, returning `null` may not be correct.
Consider a simple example:
```java
public static boolean isNestedTypes(Type type) {
return visit(type, new SchemaVisitor<>() {
@Override
public Boolean struct(Types.StructType struct, List<Boolean>
fieldResults) {
return true;
}
@Override
public Boolean field(Types.NestedField field, Boolean fieldResult) {
return true;
}
@Override
public Boolean list(Types.ListType list, Boolean elementResult) {
return true;
}
@Override
public Boolean map(Types.MapType map, Boolean keyResult, Boolean
valueResult) {
return true;
}
@Override
public Boolean primitive(Type.PrimitiveType primitive) {
return false;
}
});
}
```
When `TypeVisitor` has the new method added, nothing catches that it is not
implemented and violates the assumptions of the existing implementation, which
is that the methods return a `Boolean`. The result would be a
`NullPointerException` when this code encounters a variant. It would be better
if the result were a helpful error message that states that the visitor hasn't
been updated for variant.
In addition, some visitors use `null` to pass information. For example,
`PruneColumns` returns `null` to indicate that a field was not projected.
Returning null in those cases could result in incorrect behavior for the
visitor. That's why I think it would be safer to throw an
`UnsupportedOperationException` when the visitor does not implement `variant`.
--
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]
