You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by GitBox <gi...@apache.org> on 2021/10/07 07:53:11 UTC
[GitHub] [avro] liu-du opened a new pull request #1355: AVRO-3224: Fix ReflectDatumReader.readField when using ReflectData.AllowNull with two conversions that convert to the same avro logical type
liu-du opened a new pull request #1355:
URL: https://github.com/apache/avro/pull/1355
Make sure you have checked _all_ steps below.
### Jira
- [x] My PR addresses the following [Avro Jira](https://issues.apache.org/jira/browse/AVRO/) issues and references them in the PR title. For example, "AVRO-1234: My Avro PR"
- https://issues.apache.org/jira/browse/AVRO-3224
### Tests
- [x] My PR adds the following unit tests:
- TestReflectLogicalTypes.testReflectedSchemaWithMultipleDateConversions
- TestReflectLogicalTypes.testReflectedSchemaWithMultipleDateConversions
### Commits
- [] My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](https://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Documentation
- [] In case of new functionality, my PR adds documentation that describes how to use it.
- All the public functions and the classes in the PR contain Javadoc that explain what it does
--
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: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[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
Posted by 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 280 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
[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
Posted by GitBox <gi...@apache.org>.
liu-du commented on a change in pull request #1355:
URL: https://github.com/apache/avro/pull/1355#discussion_r821215780
##########
File path: lang/java/avro/src/main/java/org/apache/avro/reflect/ReflectDatumReader.java
##########
@@ -152,68 +160,39 @@ protected Object readArray(Object old, Schema expected, ResolvingDecoder in) thr
throw new AvroRuntimeException(msg);
}
} else {
- return readJavaArray(array, expectedType, l, in);
- }
- }
-
- private Object readJavaArray(Object array, Schema expectedType, long l, ResolvingDecoder in) throws IOException {
- Class<?> elementType = array.getClass().getComponentType();
- if (elementType.isPrimitive()) {
- return readPrimitiveArray(array, elementType, l, in);
- } else {
- return readObjectArray((Object[]) array, expectedType, l, in);
+ return readJavaArray(array, elementClass, elementSchema, l, in);
}
}
- private Object readPrimitiveArray(Object array, Class<?> c, long l, ResolvingDecoder in) throws IOException {
- return ArrayAccessor.readArray(array, c, l, in);
- }
-
- private Object readObjectArray(Object[] array, Schema expectedType, long l, ResolvingDecoder in) throws IOException {
- LogicalType logicalType = expectedType.getLogicalType();
- Conversion<?> conversion = getData().getConversionFor(logicalType);
- int index = 0;
- if (logicalType != null && conversion != null) {
- do {
- int limit = index + (int) l;
- while (index < limit) {
- Object element = readWithConversion(null, expectedType, logicalType, conversion, in);
- array[index] = element;
- index++;
- }
- } while ((l = in.arrayNext()) > 0);
+ private Object readJavaArray(Object array, Class<?> elementClass, Schema expected, long l, ResolvingDecoder in)
+ throws IOException {
+ Class<?> arrayElementClass = array.getClass().getComponentType();
+ if (arrayElementClass.isPrimitive()) {
+ return ArrayAccessor.readArray(array, arrayElementClass, l, in);
Review comment:
the call to "newArray" at line [138](https://github.com/apache/avro/pull/1355/files/d6014337563c779e57ceb101913d30094f896e8f#diff-cac357b9bc2b77f6eaaccdf0e2ef41eb8b6c17a925d5f091c4fd090313eb44ffR138) accepts an elementClass, it will only return a primitive array if elementClass==null ,that is, if there is no conversion. I guess this might be a performance optimisation for the common case when there is no conversion?
--
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
[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
Posted by GitBox <gi...@apache.org>.
liu-du commented on a change in pull request #1355:
URL: https://github.com/apache/avro/pull/1355#discussion_r779481230
##########
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:
I like the idea of adding target type property to the schema and overriding the `GenericDatumReader#read` method in `ReflectDatumReader`. I tried it by adding a property `SpecificData.CLASS_PROP` (java-class) indicating java class for logical types (it seems CLASS_PROP is the conventional property name used to indicate java class in the code base). It broke a few unit tests but they're easy to fix.
It almost worked but when I tried fixing the last broken unit test `TestReflectLogicalTypes.testReflectedSchemaLocalDateTime`, I got the following exception:
Although I was able to add java-class property to schema with a logical type, it seems some code assumes logical types should not be used with java-class. PS: The code changes are here, though I'm no longer sure we should go this way: https://github.com/liu-du/avro/commit/6320aae25b6f3c8f10d48b6b4e0fd31d03ebd1bd .
--
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
[GitHub] [avro] opwvhk 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
Posted by GitBox <gi...@apache.org>.
opwvhk commented on a change in pull request #1355:
URL: https://github.com/apache/avro/pull/1355#discussion_r779367196
##########
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:
The only problem will be how this extra parameter will spread. We need to come up with a way to prevent that.
Maybe we should add a schema property to generated unions to store the target 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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[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
Posted by GitBox <gi...@apache.org>.
liu-du commented on a change in pull request #1355:
URL: https://github.com/apache/avro/pull/1355#discussion_r779481230
##########
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:
I like the idea of adding target type property to the schema and overriding the `GenericDatumReader#read` method in `ReflectDatumReader`. I tried it by adding a property `SpecificData.CLASS_PROP` (java-class) indicating java class for logical types (it seems CLASS_PROP is the conventional property name used to indicate java class in the code base). It broke a few unit tests but they're easy to fix.
It almost worked but when I tried fixing the last broken unit test `TestReflectLogicalTypes.testReflectedSchemaLocalDateTime`, I got the following exception:
Although I was able to add java-class property to schema with a logical type, it seems some code assumes logical types property should not be used with java-class. PS: The code changes are here, though I'm no longer sure we should go this way: https://github.com/liu-du/avro/commit/6320aae25b6f3c8f10d48b6b4e0fd31d03ebd1bd .
--
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
[GitHub] [avro] opwvhk commented on pull request #1355: AVRO-3224: Fix ReflectDatumReader.readField when using ReflectData.AllowNull with two conversions that convert to the same avro logical type
Posted by GitBox <gi...@apache.org>.
opwvhk commented on pull request #1355:
URL: https://github.com/apache/avro/pull/1355#issuecomment-1060311283
Looks a lot better now! Thank you.
I've left a few minor remarks which I think would make it even better, but other than that it looks good to me.
--
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
[GitHub] [avro] opwvhk 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
Posted by GitBox <gi...@apache.org>.
opwvhk commented on a change in pull request #1355:
URL: https://github.com/apache/avro/pull/1355#discussion_r820460191
##########
File path: lang/java/avro/src/main/java/org/apache/avro/reflect/ReflectDatumReader.java
##########
@@ -70,18 +72,13 @@ public ReflectDatumReader(ReflectData data) {
super(data);
}
- @Override
- protected Object newArray(Object old, int size, Schema schema) {
+ protected Object newArray(Object old, int size, Schema schema, Class<?> cls) {
Class<?> collectionClass = ReflectData.getClassProp(schema, SpecificData.CLASS_PROP);
- Class<?> elementClass = ReflectData.getClassProp(schema, SpecificData.ELEMENT_PROP);
+ // java-element-class takes priority
Review comment:
> java-element-class takes priority
Should be: java-element-class takes priority over cls
--
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
[GitHub] [avro] liu-du commented on pull request #1355: AVRO-3224: Fix ReflectDatumReader.readField when using ReflectData.AllowNull with two conversions that convert to the same avro logical type
Posted by GitBox <gi...@apache.org>.
liu-du commented on pull request #1355:
URL: https://github.com/apache/avro/pull/1355#issuecomment-1061372757
I've changed the code according to the remarks. Thank you for the review!
--
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
[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
Posted by GitBox <gi...@apache.org>.
liu-du commented on a change in pull request #1355:
URL: https://github.com/apache/avro/pull/1355#discussion_r821215780
##########
File path: lang/java/avro/src/main/java/org/apache/avro/reflect/ReflectDatumReader.java
##########
@@ -152,68 +160,39 @@ protected Object readArray(Object old, Schema expected, ResolvingDecoder in) thr
throw new AvroRuntimeException(msg);
}
} else {
- return readJavaArray(array, expectedType, l, in);
- }
- }
-
- private Object readJavaArray(Object array, Schema expectedType, long l, ResolvingDecoder in) throws IOException {
- Class<?> elementType = array.getClass().getComponentType();
- if (elementType.isPrimitive()) {
- return readPrimitiveArray(array, elementType, l, in);
- } else {
- return readObjectArray((Object[]) array, expectedType, l, in);
+ return readJavaArray(array, elementClass, elementSchema, l, in);
}
}
- private Object readPrimitiveArray(Object array, Class<?> c, long l, ResolvingDecoder in) throws IOException {
- return ArrayAccessor.readArray(array, c, l, in);
- }
-
- private Object readObjectArray(Object[] array, Schema expectedType, long l, ResolvingDecoder in) throws IOException {
- LogicalType logicalType = expectedType.getLogicalType();
- Conversion<?> conversion = getData().getConversionFor(logicalType);
- int index = 0;
- if (logicalType != null && conversion != null) {
- do {
- int limit = index + (int) l;
- while (index < limit) {
- Object element = readWithConversion(null, expectedType, logicalType, conversion, in);
- array[index] = element;
- index++;
- }
- } while ((l = in.arrayNext()) > 0);
+ private Object readJavaArray(Object array, Class<?> elementClass, Schema expected, long l, ResolvingDecoder in)
+ throws IOException {
+ Class<?> arrayElementClass = array.getClass().getComponentType();
+ if (arrayElementClass.isPrimitive()) {
+ return ArrayAccessor.readArray(array, arrayElementClass, l, in);
Review comment:
the call to "newArray" at line 138 accepts an elementClass, it will only return a primitive array if elementClass==null ,that is, if there is no conversion. I guess this might be a performance optimisation for the common case when there is no conversion?
--
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
[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
Posted by GitBox <gi...@apache.org>.
liu-du commented on a change in pull request #1355:
URL: https://github.com/apache/avro/pull/1355#discussion_r821240718
##########
File path: lang/java/avro/src/main/java/org/apache/avro/reflect/ReflectDatumReader.java
##########
@@ -123,22 +116,37 @@ protected void addToArray(Object array, long pos, Object e) {
* representations.
*/
protected Object readArray(Object old, Schema expected, ResolvingDecoder in) throws IOException {
- Schema expectedType = expected.getElementType();
+ return readArray(old, null, expected, in);
+ }
+
+ private Object readArray(Object old, Type type, Schema expected, ResolvingDecoder in) throws IOException {
+ Class<?> elementClass = null;
+ if (type instanceof Class && ((Class<?>) type).isArray()) {
+ // get array element class
+ elementClass = ((Class<?>) type).getComponentType();
+ } else if (type instanceof ParameterizedType
+ && Collection.class.isAssignableFrom((Class<?>) ((ParameterizedType) type).getRawType())) {
+ // get collection element class
+ elementClass = (Class<?>) ((ParameterizedType) type).getActualTypeArguments()[0];
+ }
Review comment:
the logic is little different, I've extracted them into two private methods `getConversion` and `getElementClass`. The logic here is to get element class of arrays and collections, the logic in readAndConvert is to get the raw type of parametrized classes and unparametrized classes (including arrays).
--
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
[GitHub] [avro] liu-du commented on pull request #1355: AVRO-3224: Fix ReflectDatumReader.readField when using ReflectData.AllowNull with two conversions that convert to the same avro logical type
Posted by GitBox <gi...@apache.org>.
liu-du commented on pull request #1355:
URL: https://github.com/apache/avro/pull/1355#issuecomment-1050633001
@opwvhk
I changed the code a bit so now it's able to handle array of unions as well. Also the logic of apply logical conversions are now removed into one place in ReflectDatumReader.readAndConvert. Could you help review again? Thank you
--
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
[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
Posted by 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
[GitHub] [avro] opwvhk 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
Posted by GitBox <gi...@apache.org>.
opwvhk commented on a change in pull request #1355:
URL: https://github.com/apache/avro/pull/1355#discussion_r820465995
##########
File path: lang/java/avro/src/main/java/org/apache/avro/reflect/ReflectDatumReader.java
##########
@@ -123,22 +116,37 @@ protected void addToArray(Object array, long pos, Object e) {
* representations.
*/
protected Object readArray(Object old, Schema expected, ResolvingDecoder in) throws IOException {
- Schema expectedType = expected.getElementType();
+ return readArray(old, null, expected, in);
+ }
+
+ private Object readArray(Object old, Type type, Schema expected, ResolvingDecoder in) throws IOException {
+ Class<?> elementClass = null;
+ if (type instanceof Class && ((Class<?>) type).isArray()) {
+ // get array element class
+ elementClass = ((Class<?>) type).getComponentType();
+ } else if (type instanceof ParameterizedType
+ && Collection.class.isAssignableFrom((Class<?>) ((ParameterizedType) type).getRawType())) {
+ // get collection element class
+ elementClass = (Class<?>) ((ParameterizedType) type).getActualTypeArguments()[0];
+ }
Review comment:
Nicely done!
This logic also occurs in `readAndConvert` as well, and I think it would be easier to understand if extracted info a 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
[GitHub] [avro] opwvhk 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
Posted by GitBox <gi...@apache.org>.
opwvhk commented on a change in pull request #1355:
URL: https://github.com/apache/avro/pull/1355#discussion_r778746120
##########
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:
You've pulled handling the union up from `readWithoutConversion`.
This means that handling fields like this works, but if the dates are in an array of unions, it will fail (see the method `readArray` starting at line 125).
It's probably better to put this logic in an overridden version of `GenericDatumReader#read(Object, Schema, ResolvingDecoder)`, which is called from `readWithoutConversion`. Granted, this applies conversion inside a method named "without conversion", but _the current implementation already does (buggy) conversions for unions_, so we won't be off worse.
--
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
[GitHub] [avro] opwvhk 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
Posted by GitBox <gi...@apache.org>.
opwvhk commented on a change in pull request #1355:
URL: https://github.com/apache/avro/pull/1355#discussion_r820463337
##########
File path: lang/java/avro/src/main/java/org/apache/avro/reflect/ReflectDatumReader.java
##########
@@ -152,68 +160,39 @@ protected Object readArray(Object old, Schema expected, ResolvingDecoder in) thr
throw new AvroRuntimeException(msg);
}
} else {
- return readJavaArray(array, expectedType, l, in);
- }
- }
-
- private Object readJavaArray(Object array, Schema expectedType, long l, ResolvingDecoder in) throws IOException {
- Class<?> elementType = array.getClass().getComponentType();
- if (elementType.isPrimitive()) {
- return readPrimitiveArray(array, elementType, l, in);
- } else {
- return readObjectArray((Object[]) array, expectedType, l, in);
+ return readJavaArray(array, elementClass, elementSchema, l, in);
}
}
- private Object readPrimitiveArray(Object array, Class<?> c, long l, ResolvingDecoder in) throws IOException {
- return ArrayAccessor.readArray(array, c, l, in);
- }
-
- private Object readObjectArray(Object[] array, Schema expectedType, long l, ResolvingDecoder in) throws IOException {
- LogicalType logicalType = expectedType.getLogicalType();
- Conversion<?> conversion = getData().getConversionFor(logicalType);
- int index = 0;
- if (logicalType != null && conversion != null) {
- do {
- int limit = index + (int) l;
- while (index < limit) {
- Object element = readWithConversion(null, expectedType, logicalType, conversion, in);
- array[index] = element;
- index++;
- }
- } while ((l = in.arrayNext()) > 0);
+ private Object readJavaArray(Object array, Class<?> elementClass, Schema expected, long l, ResolvingDecoder in)
+ throws IOException {
+ Class<?> arrayElementClass = array.getClass().getComponentType();
+ if (arrayElementClass.isPrimitive()) {
+ return ArrayAccessor.readArray(array, arrayElementClass, l, in);
Review comment:
I don't understand this; are conversions of primitive types really impossible?
--
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
[GitHub] [avro] opwvhk 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
Posted by GitBox <gi...@apache.org>.
opwvhk commented on a change in pull request #1355:
URL: https://github.com/apache/avro/pull/1355#discussion_r779371758
##########
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:
This just occurred to me: maybe we should bypass this union issue by adding the target type property to the schema with the logical type. After all, the underlying cause is that some conversions (most commonly date/time conversions) have multiple target types.
Anyway, have a look at the callsites of `makeNullable` and `getAnnotatedUnion` and choose the best/easiest option.
--
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
[GitHub] [avro] opwvhk 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
Posted by GitBox <gi...@apache.org>.
opwvhk commented on a change in pull request #1355:
URL: https://github.com/apache/avro/pull/1355#discussion_r779365882
##########
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:
Actually, the method `getAnnotatedUnion` has 4 usages. This method allows creating unions for common supertypes, and is the reason unions also occur for other use cases than nullable fields.
As a result, I think we do need to override that `GenericDatumReader#read` method in `ReflectDatumReader`. Maybe by directly calling an overloaded version with an extra type parameter (`null` in the overridden `read` method as it's unknown).
--
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
[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
Posted by GitBox <gi...@apache.org>.
liu-du commented on a change in pull request #1355:
URL: https://github.com/apache/avro/pull/1355#discussion_r779481230
##########
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:
I like the idea of adding target type property to the schema and overriding the `GenericDatumReader#read` method in `ReflectDatumReader`. I tried it by adding a property `SpecificData.CLASS_PROP` (java-class) indicating java class for logical types (it seems CLASS_PROP is the conventional property name used to indicate java class in the code base). It broke a few unit tests but they're easy to fix.
It almost worked but when I tried fixing the last broken unit test `TestReflectLogicalTypes.testReflectedSchemaLocalDateTime`, I got the following exception:
Although I was able to add java-class property to schema with a logical type, it seems some code assumes logical types should not be used with java-class. PS: The code changes are here: https://github.com/liu-du/avro/commit/6320aae25b6f3c8f10d48b6b4e0fd31d03ebd1bd , though I'm no longer sure we should go this way.
--
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