ahmedabu98 commented on code in PR #27329:
URL: https://github.com/apache/beam/pull/27329#discussion_r1304923275
##########
sdks/java/extensions/avro/src/main/java/org/apache/beam/sdk/extensions/avro/schemas/utils/AvroUtils.java:
##########
@@ -1330,15 +1339,18 @@ private static org.apache.avro.Schema getFieldSchema(
if (avroSchema.getType() == Type.FIXED) {
GenericFixed genericFixed = (GenericFixed) value;
BigDecimal bigDecimal =
- new Conversions.DecimalConversion().fromFixed(genericFixed,
type.type, logicalType);
+ new Conversions.DecimalConversion().fromFixed(genericFixed,
type.type, logicalType);
return convertDecimal(bigDecimal, fieldType);
} else {
- // avroSchema.getType() == Type.BYTES
- ByteBuffer byteBuffer = (ByteBuffer) value;
- BigDecimal bigDecimal =
- new Conversions.DecimalConversion()
- .fromBytes(byteBuffer.duplicate(), type.type,
logicalType);
- return convertDecimal(bigDecimal, fieldType);
+ if (avroSchema.getType() == Type.BYTES) {
Review Comment:
nit: Same here, can just make this into one `else if` condition.
##########
sdks/java/extensions/avro/src/test/java/org/apache/beam/sdk/extensions/avro/schemas/utils/AvroUtilsTest.java:
##########
@@ -287,7 +302,31 @@ private Schema getBeamSchema() {
.addField(Field.of("double", FieldType.DOUBLE))
.addField(Field.of("string", FieldType.STRING))
.addField(Field.of("bytes", FieldType.BYTES))
- .addField(Field.of("decimal", FieldType.DECIMAL))
+ .addField(
+ Field.of(
+ "decimal",
+ FieldType.DECIMAL,
+ Schema.Options.setOption("precision", FieldType.INT32,
Integer.MAX_VALUE)
Review Comment:
What happens when we don't specify options? We want it so that we don't
break the previous use case of `Field.of("decimal", FieldType.DECIMAL)`.
##########
sdks/java/extensions/avro/src/main/java/org/apache/beam/sdk/extensions/avro/schemas/utils/AvroUtils.java:
##########
@@ -1194,12 +1199,16 @@ private static org.apache.avro.Schema getFieldSchema(
case DECIMAL:
BigDecimal decimal = (BigDecimal) value;
LogicalType logicalType = typeWithNullability.type.getLogicalType();
- if (typeWithNullability.type.getType() == Type.FIXED) {
+ Type type = typeWithNullability.type.getType();
+ if (type == Type.FIXED) {
return new Conversions.DecimalConversion()
- .toFixed(decimal, typeWithNullability.type, logicalType);
+ .toFixed(decimal, typeWithNullability.type, logicalType);
} else {
- // typeWithNullability.type.getType() == Type.BYTES
- return new Conversions.DecimalConversion().toBytes(decimal, null,
logicalType);
+ if (type == Type.BYTES) {
Review Comment:
nit: I think we can merge this into
```
else if (type == Type.BYTES) { .. }
```
##########
sdks/java/extensions/avro/src/main/java/org/apache/beam/sdk/extensions/avro/schemas/utils/AvroUtils.java:
##########
@@ -1050,17 +1050,22 @@ private static org.apache.avro.Schema getFieldSchema(
String type = options.hasOption("type") ? options.getValue("type") :
"bytes";
Type decimalType = Type.valueOf(type.toUpperCase());
Integer precision =
- options.hasOption("precision") ? options.getValue("precision")
: Integer.MAX_VALUE;
+ options.hasOption("precision") ? options.getValue("precision") :
Integer.MAX_VALUE;
Integer scale = options.hasOption("scale") ? options.getValue("scale")
: 0;
LogicalTypes.Decimal decimal = LogicalTypes.decimal(precision, scale);
if (decimalType == Type.FIXED) {
+ if (!options.hasOption("size")) {
+ throw new IllegalArgumentException("'size' option is required for
field:" + fieldName);
Review Comment:
Not familiar with FIXED types, but does it make sense to set a default value
instead of throwing an exception?
##########
sdks/java/extensions/avro/src/test/java/org/apache/beam/sdk/extensions/avro/schemas/utils/AvroUtilsTest.java:
##########
@@ -287,7 +302,31 @@ private Schema getBeamSchema() {
.addField(Field.of("double", FieldType.DOUBLE))
.addField(Field.of("string", FieldType.STRING))
.addField(Field.of("bytes", FieldType.BYTES))
- .addField(Field.of("decimal", FieldType.DECIMAL))
+ .addField(
+ Field.of(
+ "decimal",
+ FieldType.DECIMAL,
+ Schema.Options.setOption("precision", FieldType.INT32,
Integer.MAX_VALUE)
Review Comment:
I think we should be able to leave options unspecified right? we have
default values in place for these fields 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]
