[GitHub] [beam] mosche commented on a diff in pull request #17372: [WIP][BEAM-8715] Bump Avro version to 1.9.2

[GitBox <gi...@apache.org>]

mosche commented on code in PR #17372:
URL: https://github.com/apache/beam/pull/17372#discussion_r856132501


##########
sdks/java/core/src/main/java/org/apache/beam/sdk/values/RowWithGetters.java:
##########
@@ -128,6 +129,14 @@ private <T> T getValue(FieldType type, Object fieldValue, @Nullable Integer cach
               cachedMaps.computeIfAbsent(
                   cacheKey, i -> getMapValue(type.getMapKeyType(), type.getMapValueType(), map))
           : (T) getMapValue(type.getMapKeyType(), type.getMapValueType(), map);
+    } else if (type.getTypeName().equals(TypeName.DATETIME)) {
+      if (fieldValue instanceof java.time.LocalDate) {
+        Instant instant = java.time.Instant.parse(fieldValue + "T00:00:00.00Z");
+        return (T) org.joda.time.Instant.ofEpochMilli(instant.toEpochMilli());
+      } else if (fieldValue instanceof java.time.Instant) {
+        return (T) org.joda.time.Instant.ofEpochMilli(((Instant) fieldValue).toEpochMilli());
+      }
+      return (T) fieldValue;

Review Comment:
   @aromanenko-dev I think @TheNeuralBit is right. Based on [benchmarks](https://github.com/apache/beam/pull/17172#issuecomment-1085981280) I've done just recently, branching in `RowWithGetters` doesn't perform well. In https://github.com/apache/beam/pull/17172 I'm suggesting to push all of the current code down into the getters.
   
   The `GetterBasedSchemaProvider`s (except the Avro one) only support Joda [ReadableInstant](https://github.com/apache/beam/blob/master/sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/utils/StaticSchemaInference.java#L144-L145) (type is the method return type or field type) as `DATETIME`. Attempting to use java time would most likely fail during schema generation ([or generate a row schema with nested internal fields](https://github.com/apache/beam/blob/master/sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/utils/StaticSchemaInference.java#L166))
   
   For Avro there's already a [conversion layer](https://github.com/apache/beam/blob/master/sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/utils/AvroUtils.java#L278-L294) in place you could leverage for that. For `DATETIME` it's using these converters:
   - [ConvertValueForGetter.convertDateTime](https://github.com/apache/beam/blob/master/sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/utils/ByteBuddyUtils.java#L792)
   - [ConvertValueForSetter.convertDateTime](https://github.com/apache/beam/blob/master/sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/utils/ByteBuddyUtils.java#L1160)



-- 
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: github-unsubscribe@beam.apache.org

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