apilloud commented on a change in pull request #11074:
URL: https://github.com/apache/beam/pull/11074#discussion_r833730138
##########
File path:
sdks/java/core/src/main/java/org/apache/beam/sdk/values/RowWithGetters.java
##########
@@ -130,10 +130,11 @@ private Iterable getIterableValue(FieldType elementType,
Object fieldValue) {
OneOfType oneOfType = type.getLogicalType(OneOfType.class);
OneOfType.Value oneOfValue = (OneOfType.Value) fieldValue;
Object convertedOneOfField =
- getValue(oneOfValue.getFieldType(), oneOfValue.getValue(), null);
- return (T)
- oneOfType.toBaseType(
- oneOfType.createValue(oneOfValue.getCaseType(),
convertedOneOfField));
+ getValue(oneOfType.getFieldType(oneOfValue),
oneOfValue.getValue(), null);
+ return (T) oneOfType.createValue(oneOfValue.getCaseType(),
convertedOneOfField);
+ } else if (type.getTypeName().isLogicalType()) {
+ // Getters are assumed to return the base type.
+ return (T) type.getLogicalType().toInputType(fieldValue);
Review comment:
There was a past world where the getters supported returning either
logical type or base type. This was suppose to be whatever the previous
transform provided (or the base type if coming from a coder). The goal was to
save a conversion from base type to logical type for values that were just
passed along. I believe the only use case was SQL and I removed that in #13930
because it is broken and like you notice here more expensive.
It should be possible to do more simplification such that we always work
with logical types instead of base types. Eventually we will want a
pass-through optimization again, but it will need to be at a lower level than
logical types. (The only expensive type today is String, which isn't a logical
type.)
--
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]
