iemejia commented on a change in pull request #11217:
URL: https://github.com/apache/beam/pull/11217#discussion_r484493417
##########
File path:
sdks/java/core/src/test/java/org/apache/beam/sdk/schemas/utils/AvroUtilsTest.java
##########
@@ -260,8 +260,25 @@ 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("timestampMillis", FieldType.DATETIME))
+ .addField(
Review comment:
If I understood correctly there are options at the Schema level, can we
also set some options at the Schema level to test that conversion too.
##########
File path: build.gradle
##########
@@ -63,8 +63,7 @@ rat {
"**/.github/**/*",
"**/package-list",
- "**/test.avsc",
- "**/user.avsc",
+ "**/*.avsc",
Review comment:
:+1:
##########
File path:
sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/utils/AvroUtils.java
##########
@@ -316,6 +444,15 @@ public static Schema toBeamSchema(org.apache.avro.Schema
schema) {
builder.addField(beamField);
}
+ return builder.setOptions(toBeamSchemaOptions(schema)).build();
+ }
+
+ private static Schema.Options toBeamSchemaOptions(org.apache.avro.Schema
schema) {
+ Schema.Options.Builder builder = Schema.Options.builder();
+ schema
+ .getJsonProps()
Review comment:
Worth to replace with `.getObjectProps()` too
##########
File path:
sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/utils/AvroUtils.java
##########
@@ -287,7 +297,125 @@ protected StackManipulation
convertDefault(TypeDescriptor<?> type) {
public static Schema.Field toBeamField(org.apache.avro.Schema.Field field) {
TypeWithNullability nullableType = new TypeWithNullability(field.schema());
FieldType beamFieldType = toFieldType(nullableType);
- return Field.of(field.name(), beamFieldType);
+ return Field.of(field.name(),
beamFieldType).withOptions(toBeamFieldOptions(field));
+ }
+
+ private static Schema.Options
toBeamFieldOptions(org.apache.avro.Schema.Field field) {
+ Schema.Options.Builder builder = Schema.Options.builder();
+ field
+ .getJsonProps()
Review comment:
`getJsonProps()` was removed on Avro >= 1.9 so we should probably do all
this logic with `getObjectProps` instead. I know this can change a good chunk
of the main code, but probably better to be forwards compatible.
##########
File path:
sdks/java/core/src/test/java/org/apache/beam/sdk/schemas/AvroSchemaTest.java
##########
@@ -375,6 +464,12 @@ public void testSpecificRecordSchema() {
assertEquals(SCHEMA, new
AvroRecordSchema().schemaFor(TypeDescriptor.of(TestAvro.class)));
}
+ @Test
+ public void testOptionsRecordSchema() {
+ assertEquals(
+ OPTIONS_SCHEMA, new
AvroRecordSchema().schemaFor(TypeDescriptor.of(TestOptionsAvro.class)));
Review comment:
:+1: good approach this test I like using a generated class for
completeness.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
