Re: [PR] GH-698: Improve and fix Avro read consumers [arrow-java]
lidavidm merged PR #718: URL: https://github.com/apache/arrow-java/pull/718 -- 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]
Re: [PR] GH-698: Improve and fix Avro read consumers [arrow-java]
martin-traverse commented on PR #718: URL: https://github.com/apache/arrow-java/pull/718#issuecomment-2822728753 > Looks like there are some lint errors to be fixed Apologies - I have reapplied spotless, should be ok now! -- 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]
Re: [PR] GH-698: Improve and fix Avro read consumers [arrow-java]
lidavidm commented on PR #718: URL: https://github.com/apache/arrow-java/pull/718#issuecomment-2817591251 Looks like there are some lint errors to be fixed -- 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]
Re: [PR] GH-698: Improve and fix Avro read consumers [arrow-java]
lidavidm commented on code in PR #718:
URL: https://github.com/apache/arrow-java/pull/718#discussion_r2051858753
##
adapter/avro/src/main/java/org/apache/arrow/adapter/avro/AvroToArrowUtils.java:
##
@@ -277,11 +333,17 @@ private static Consumer createConsumer(
break;
case BYTES:
if (logicalType instanceof LogicalTypes.Decimal) {
- arrowType = createDecimalArrowType((LogicalTypes.Decimal)
logicalType);
+ LogicalTypes.Decimal decimalType = (LogicalTypes.Decimal)
logicalType;
+ arrowType = createDecimalArrowType(decimalType, schema);
fieldType =
new FieldType(nullable, arrowType, /* dictionary= */ null,
getMetaData(schema));
vector = createVector(consumerVector, fieldType, name, allocator);
- consumer = new
AvroDecimalConsumer.BytesDecimalConsumer((DecimalVector) vector);
+ if (decimalType.getPrecision() <= 38) {
Review Comment:
I think FIXED is okay to allow round-trip by default even if it's not
technically as compact.
--
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]
Re: [PR] GH-698: Improve and fix Avro read consumers [arrow-java]
martin-traverse commented on code in PR #718:
URL: https://github.com/apache/arrow-java/pull/718#discussion_r2050405594
##
adapter/avro/src/main/java/org/apache/arrow/adapter/avro/AvroToArrowConfig.java:
##
@@ -81,4 +114,8 @@ public DictionaryProvider.MapDictionaryProvider
getProvider() {
public Set getSkipFieldNames() {
return skipFieldNames;
}
+
+ public boolean isHandleNullable() {
Review Comment:
This is now part of the legacyMode parameter
--
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]
Re: [PR] GH-698: Improve and fix Avro read consumers [arrow-java]
martin-traverse commented on PR #718: URL: https://github.com/apache/arrow-java/pull/718#issuecomment-2815088209 I have removed the breaking changes text from the headline text but am not able to remove the label. -- 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]
Re: [PR] GH-698: Improve and fix Avro read consumers [arrow-java]
martin-traverse commented on code in PR #718:
URL: https://github.com/apache/arrow-java/pull/718#discussion_r2050413561
##
adapter/avro/src/main/java/org/apache/arrow/adapter/avro/AvroToArrowUtils.java:
##
@@ -277,11 +333,17 @@ private static Consumer createConsumer(
break;
case BYTES:
if (logicalType instanceof LogicalTypes.Decimal) {
- arrowType = createDecimalArrowType((LogicalTypes.Decimal)
logicalType);
+ LogicalTypes.Decimal decimalType = (LogicalTypes.Decimal)
logicalType;
+ arrowType = createDecimalArrowType(decimalType, schema);
fieldType =
new FieldType(nullable, arrowType, /* dictionary= */ null,
getMetaData(schema));
vector = createVector(consumerVector, fieldType, name, allocator);
- consumer = new
AvroDecimalConsumer.BytesDecimalConsumer((DecimalVector) vector);
+ if (decimalType.getPrecision() <= 38) {
Review Comment:
For FIXED decimals I am using the fixedSize to choose the decimal type. For
BYTES decimals yes they would just come back as the smaller type if the
precision fits.
I used FIXED as the default output for decimals in the producers, because it
is closer to the Arrow representation, but on reflection Avro as a format is
very focused on keeping data compact, maybe BYTES makes more sense. It seems to
be the default choice in Avro. Do you think we should go with that?
--
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]
Re: [PR] GH-698: Improve and fix Avro read consumers [arrow-java]
martin-traverse commented on code in PR #718: URL: https://github.com/apache/arrow-java/pull/718#discussion_r2050407127 ## adapter/avro/src/test/java/org/apache/arrow/adapter/avro/RoundTripSchemaTest.java: ## Review Comment: I have factored out the common code as a helper. Don't think it can go all the way to being parameterised because the types need to be set up differently. -- 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]
Re: [PR] GH-698: Improve and fix Avro read consumers [arrow-java]
martin-traverse commented on code in PR #718:
URL: https://github.com/apache/arrow-java/pull/718#discussion_r2050405239
##
adapter/avro/src/main/java/org/apache/arrow/adapter/avro/producers/AvroNullableProducer.java:
##
@@ -21,8 +21,8 @@
import org.apache.avro.io.Encoder;
/**
- * Producer wrapper which producers nullable types to an avro encoder. Write
the data to the
- * underlying {@link FieldVector}.
+ * Producer wrapper which producers nullable types to an avro encoder. Reed
data from the underlying
Review Comment:
;-)
--
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]
Re: [PR] GH-698: Improve and fix Avro read consumers [arrow-java]
martin-traverse commented on code in PR #718:
URL: https://github.com/apache/arrow-java/pull/718#discussion_r2050404883
##
adapter/avro/src/main/java/org/apache/arrow/adapter/avro/ArrowToAvroUtils.java:
##
@@ -332,7 +332,17 @@ private static T buildBaseTypeSchema(
case List:
case FixedSizeList:
-return buildArraySchema(builder.array(), field, namespace);
+// Arrow uses "$data$" as the field name for list items, that is not a
valid Avro name
+Field itemField = field.getChildren().get(0);
+if (ListVector.DATA_VECTOR_NAME.equals(itemField.getName())) {
Review Comment:
Hm, I think for list / map types using the constant defined names for
children makes sense, with "item" instead of "$data$" for list items. More
generally, we could normalise illegal chars to "_" to match the [Avro name
rules](https://avro.apache.org/docs/1.12.0/specification/#names). Per my
understanding similar rules are already enforced in C++, but are not part of
the Arrow spec or Java implementation.
Very happy to put the normalisation in, it's probably a more useful
behaviour than throwing an error in the adapter. Would you like me to do it?
--
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]
Re: [PR] GH-698: Improve and fix Avro read consumers [arrow-java]
martin-traverse commented on PR #718: URL: https://github.com/apache/arrow-java/pull/718#issuecomment-2814996409 Ok, here is an update. I have put a flag "legacyMode" in the config object and used that to control all the places where the old logic is impacted. I have allowed decimal 256 to come through in legacy mode and also timestamp-nanos with the old semantics. The local-timestamp-xxx types do not come through in legacy mode, because timestamp-xxx is already treated as local. I have replaced the original code for the logical types test and those are all passing. -- 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]
Re: [PR] GH-698: Improve and fix Avro read consumers [arrow-java]
martin-traverse commented on PR #718: URL: https://github.com/apache/arrow-java/pull/718#issuecomment-2803951943 > I think in the interest of trying to keep semver, we should avoid breaking changes if possible. Any thoughts @jbonofre @laurentgo? Or we could just call the next release 19.0.0... > > If it helps we could just have a single flag for "old" behavior? A major version bump feels a bit extreme for something as small as this! Let me try the single flag approach. There might be complications with the change for zone-aware vs local timestamps, because the types are different, but I think so long as the check happens before the types are decided it should be ok. -- 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]
Re: [PR] GH-698: Improve and fix Avro read consumers [arrow-java]
github-actions[bot] commented on PR #718: URL: https://github.com/apache/arrow-java/pull/718#issuecomment-2802462275 Thank you for opening a pull request! Please label the PR with one or more of: - bug-fix - chore - dependencies - documentation - enhancement Also, add the 'breaking-change' label if appropriate. See [CONTRIBUTING.md](https://github.com/apache/arrow-java/blob/main/CONTRIBUTING.md) for details. -- 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]
Re: [PR] GH-698: Improve and fix Avro read consumers [arrow-java]
lidavidm commented on code in PR #718:
URL: https://github.com/apache/arrow-java/pull/718#discussion_r2043326609
##
adapter/avro/src/main/java/org/apache/arrow/adapter/avro/producers/AvroNullableProducer.java:
##
@@ -21,8 +21,8 @@
import org.apache.avro.io.Encoder;
/**
- * Producer wrapper which producers nullable types to an avro encoder. Write
the data to the
- * underlying {@link FieldVector}.
+ * Producer wrapper which producers nullable types to an avro encoder. Reed
data from the underlying
Review Comment:
```suggestion
* Producer wrapper which produces nullable types to an avro encoder. Read
data from the underlying
```
##
adapter/avro/src/main/java/org/apache/arrow/adapter/avro/ArrowToAvroUtils.java:
##
@@ -332,7 +332,17 @@ private static T buildBaseTypeSchema(
case List:
case FixedSizeList:
-return buildArraySchema(builder.array(), field, namespace);
+// Arrow uses "$data$" as the field name for list items, that is not a
valid Avro name
Review Comment:
The funny thing is, arrow-java _shouldn't be doing that_, it was just never
corrected...
##
adapter/avro/src/test/java/org/apache/arrow/adapter/avro/RoundTripSchemaTest.java:
##
Review Comment:
Is there an opportunity to structure this as a parameterized test?
##
adapter/avro/src/main/java/org/apache/arrow/adapter/avro/ArrowToAvroUtils.java:
##
@@ -332,7 +332,17 @@ private static T buildBaseTypeSchema(
case List:
case FixedSizeList:
-return buildArraySchema(builder.array(), field, namespace);
+// Arrow uses "$data$" as the field name for list items, that is not a
valid Avro name
+Field itemField = field.getChildren().get(0);
+if (ListVector.DATA_VECTOR_NAME.equals(itemField.getName())) {
Review Comment:
Or just normalize all field names to something consistent in Avro?
##
adapter/avro/src/main/java/org/apache/arrow/adapter/avro/AvroToArrowUtils.java:
##
@@ -277,11 +333,17 @@ private static Consumer createConsumer(
break;
case BYTES:
if (logicalType instanceof LogicalTypes.Decimal) {
- arrowType = createDecimalArrowType((LogicalTypes.Decimal)
logicalType);
+ LogicalTypes.Decimal decimalType = (LogicalTypes.Decimal)
logicalType;
+ arrowType = createDecimalArrowType(decimalType, schema);
fieldType =
new FieldType(nullable, arrowType, /* dictionary= */ null,
getMetaData(schema));
vector = createVector(consumerVector, fieldType, name, allocator);
- consumer = new
AvroDecimalConsumer.BytesDecimalConsumer((DecimalVector) vector);
+ if (decimalType.getPrecision() <= 38) {
Review Comment:
Hmm, it's technically possible to have a decimal256 with a smaller precision
though
##
adapter/avro/src/main/java/org/apache/arrow/adapter/avro/ArrowToAvroUtils.java:
##
@@ -332,7 +332,17 @@ private static T buildBaseTypeSchema(
case List:
case FixedSizeList:
-return buildArraySchema(builder.array(), field, namespace);
+// Arrow uses "$data$" as the field name for list items, that is not a
valid Avro name
+Field itemField = field.getChildren().get(0);
+if (ListVector.DATA_VECTOR_NAME.equals(itemField.getName())) {
Review Comment:
Do we perhaps want to check for invalid names more generally and
mangle/normalize them?
##
adapter/avro/src/main/java/org/apache/arrow/adapter/avro/AvroToArrowConfig.java:
##
@@ -81,4 +114,8 @@ public DictionaryProvider.MapDictionaryProvider
getProvider() {
public Set getSkipFieldNames() {
return skipFieldNames;
}
+
+ public boolean isHandleNullable() {
Review Comment:
nit: could we perhaps get a more descriptive name for this parameter
overall? "handleUnionOfNullAsNullable"? (As enterprise-java-y as that is...)
##
adapter/avro/src/main/java/org/apache/arrow/adapter/avro/AvroToArrowUtils.java:
##
@@ -277,11 +333,17 @@ private static Consumer createConsumer(
break;
case BYTES:
if (logicalType instanceof LogicalTypes.Decimal) {
- arrowType = createDecimalArrowType((LogicalTypes.Decimal)
logicalType);
+ LogicalTypes.Decimal decimalType = (LogicalTypes.Decimal)
logicalType;
+ arrowType = createDecimalArrowType(decimalType, schema);
fieldType =
new FieldType(nullable, arrowType, /* dictionary= */ null,
getMetaData(schema));
vector = createVector(consumerVector, fieldType, name, allocator);
- consumer = new
AvroDecimalConsumer.BytesDecimalConsumer((DecimalVector) vector);
+ if (decimalType.getPrecision() <= 38) {
Review Comment:
I guess in that case it would round-trip to the smaller type?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log o
