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> 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> 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> 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<String> 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 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]
