[GitHub] [beam] iemejia commented on a change in pull request #11217: [BEAM-9416] BIP-1: Convert Avro metadata to Schema options

[GitBox <gi...@apache.org>]

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:
users@infra.apache.org