[BEAM-4453] Add schema support for Java POJOs and Java Beans by reuvenlax · Pull Request #5873 · apache/beam

reuvenlax · 2018-07-03T00:42:41Z

Adds automatic schema support for these types of classes. ByteBuddy is use to generate automatic connectors, so that Row objects can transparently delegate to the underlying storage in user objects.

While this PR is large, splitting it up didn't seem to help much. Also, much of the line count is in unit tests.

R: @apilloud

* Recursive POJOs and collection/map fields still not support. * getter/setter generation still needs to be moved into helper class. * Still missing optimized Row -> POJO conversion when the Row is a wrapper around a POJO.

…e[] parameter (as that's what Row.getBytes vends), so make sure we wrap it before assiging.

…s. This also allows a fast path for Row -> T conversion for the case of RowWithGetters.

Make setters and getters work for array types. Row.getArray always returns a Java List type, but we support POJOs that contain Java array; what's more, the POJO array might contain either the primitive or the boxed type. Detect these cases, and insert bytecode to do the appropriate conversions. Make setters/getters work for other types as well.

…ns up the code, and makes these utilities reusable for other types of getters (e.g. JavaBean getters).

…d for other class types.

…ictable field order.

apilloud

Found a bug, a few cleanup items, and a bunch of nits. Didn't review the tests too carefully.

apilloud · 2018-07-03T16:07:31Z

sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/DefaultSchema.java

-                    + type
-                    + " has a @DefaultSchemaProvider annotation "
-                    + " with a null argument.");
+                "Type " + type + " has a @DefaultSchema annotation " + " with a null argument.");


Looks like spotless messed up your string. (Unnecessary " + " in the middle.)

apilloud · 2018-07-03T23:20:23Z

sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/GetterBasedSchemaProvider.java

+
+    // Iterate over the row, and set (possibly recursively) each field in the underlying object
+    // using the setter.
+    Schema schema = row.getSchema();


You are also calling row.getSchema() a few lines up. If getSchema is expensive it makes sense to move this before createSetters() and use it there.

apilloud · 2018-07-03T23:27:36Z

sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/Schema.java

  private List<Field> fields;
+  // Cache the hashCode, so it doesn't have to be recomputed. Schema objects are immutable, so this
+  // is correct.
+  private int hashCode;


All of these fields are immutable. Add final?

apilloud · 2018-07-06T17:36:55Z

sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/utils/ByteBuddyUtils.java

+   */
+  static class ConvertValueForGetter extends TypeConversion<StackManipulation> {
+    // The code that reads the value.
+    private StackManipulation readValue;


apilloud · 2018-07-06T17:53:00Z

sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/utils/ByteBuddyUtils.java

+          readValue,
+          TypeCasting.to(READABLE_INSTANT_TYPE),
+          // Call ReadableInstant.getMillis to extract the millis since the epoch.
+          MethodInvocation.invoke(


Thanks for the comments calling out what each of these do!

nit: I still think this particular block is still somewhat difficult to follow. It might be slightly easier to read if you pulled the MethodInvocaitons each into it's own Compound like convertArray above? (I'm not sure that would make it easier to read, so no strong opinion here.)

not sure it would improve the readability

apilloud · 2018-07-06T20:39:08Z

sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/utils/StaticSchemaInference.java

+          break;
+        case SETTER:
+          if (method.getName().startsWith("set")) {
+            this.name = ReflectUtils.stripPrefix(method.getName(), "get");


s/get/set/

You must be missing some tests for setters?

apilloud · 2018-07-06T21:07:13Z

sdks/java/core/src/main/java/org/apache/beam/sdk/values/Row.java

-        },
-        values -> Row.withSchema(schema).addValues(values).build());
-  }
+  private Schema schema;


apilloud · 2018-07-06T21:09:21Z

sdks/java/core/src/main/java/org/apache/beam/sdk/values/Row.java

+  }
+
+  /**
+   * Get a {@link TypeName#INT16 16} value by field index, {@link ClassCastException} is thrown if


Looks like there is an extra 16? (This line should be unchanged.)

apilloud · 2018-07-06T21:14:01Z

sdks/java/core/src/main/java/org/apache/beam/sdk/values/RowWithGetters.java

+ * the appropriate fields from the POJO.
+ */
+public class RowWithGetters extends Row {
+  private FieldValueGetterFactory fieldValueGetterFactory;


All these things appear to be final?

apilloud · 2018-07-06T21:15:50Z

sdks/java/core/src/main/java/org/apache/beam/sdk/values/RowWithStorage.java

+
+/** Concrete subclass of {@link Row} that explicitly stores all fields of the row. */
+public class RowWithStorage extends Row {
+  private List<Object> values;


apilloud

Thanks for the cleanup. LGTM

Reuven Lax added 15 commits July 2, 2018 17:13

Add schema inference from POJO.

0b353a4

Add BYTES type to schema, and POJO inference.

b2eaf22

Add automatic getter/setter inference from POJOS.

5a244e3

* Recursive POJOs and collection/map fields still not support. * getter/setter generation still needs to be moved into helper class. * Still missing optimized Row -> POJO conversion when the Row is a wrapper around a POJO.

Handle setter for ByteBuffer fields. Setter will be called with a byt…

1df588d

…e[] parameter (as that's what Row.getBytes vends), so make sure we wrap it before assiging.

Split Row into separate subclass for RowWithStorage and RowWithGetter…

be3f53d

…s. This also allows a fast path for Row -> T conversion for the case of RowWithGetters.

Introduce FieldValueGetterFactory and FieldValueSetterFactory.

01e3c41

Add support for nested, array, and map types.

6cde85f

Refactor getter/setter conversion utilities into a library. This clea…

c7f97a8

…ns up the code, and makes these utilities reusable for other types of getters (e.g. JavaBean getters).

Refactor Schema inference from POJOs so much of the code can be reuse…

536fe4b

…d for other class types.

Add encoding byte BYTES type.

cdea677

Make FieldValueGetter and FieldValueSetter generic types again.

f5c0b71

Start adding JavaBean getters and setters.

633bac3

Make sure that field order is stable, and make tests robust to unpred…

dfd72db

…ictable field order.

Simplify convertArray.

b1aade9

apilloud reviewed Jul 6, 2018

View reviewed changes

Reuven Lax added 3 commits July 9, 2018 09:42

Address code-review comments.

5d0ca72

Address code-review comments.

5194e00

Add better validation to isSetter isGetter

77f66df

apilloud approved these changes Jul 9, 2018

View reviewed changes

reuvenlax merged commit 784f17e into apache:master Jul 9, 2018

reuvenlax deleted the schemas_javabean_inference branch December 9, 2018 23:02

Conversation

reuvenlax commented Jul 3, 2018

Uh oh!

apilloud left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apilloud Jul 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apilloud left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

apilloud Jul 6, 2018 •

edited

Loading