[GitHub] [avro] liu-du commented on a change in pull request #1355: AVRO-3224: Fix ReflectDatumReader.readField when using ReflectData.AllowNull with two conversions that convert to the same avro logical type

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

liu-du commented on a change in pull request #1355:
URL: https://github.com/apache/avro/pull/1355#discussion_r779289567



##########
File path: lang/java/avro/src/main/java/org/apache/avro/reflect/ReflectDatumReader.java
##########
@@ -287,6 +288,26 @@ protected void readField(Object record, Field field, Object oldDatum, ResolvingD
             return;
           }
         }
+
+        if (field.schema().isUnion()) {

Review comment:
       Hi opwvhk, thanks for reviewing. 
   
   In order to get correct conversion, I need both class type and logical type (since two different classes are mapped to the same logical type). At line 298, I can get the class type using `accessor.getField().getType()` and pass it to `getData().getConversionByClass`. I guess for the same reason, the logic of line 277-290 also ended up where it is now (see line 283 where it passes `accessor.getField().getType()` to `getData().getConversionByClass`).
   
   If the logic is moved to `GenericDatumReader#read(Object, Schema, ResolvingDecoder)` then we _cannot_ get class type without passing an extra argument into the `GenericDatumReader#read` method. This may requires changing a lot of the code. 
   
   I think `ReflectData.AllowNull` doesn't generate the array of unions since it only makes class fields nullable (see: https://github.com/apache/avro/blob/master/lang/java/avro/src/main/java/org/apache/avro/reflect/ReflectData.java#L100), and therefore the logic is put into `ReflectDatumReader#readField` method. 
   
   




-- 
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: issues-unsubscribe@avro.apache.org

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