[BEAM-12769] Adds support for expanding a Java cross-language transform using the class name and builder methods #15343

chamikaramj · 2021-08-17T18:00:27Z

This adds proto and Java updates.
Python updates to follow.

Please see here for the design: https://docs.google.com/document/d/1ECXSWicE31K-vSxdb4qL6UcmovOAWvE-ZHFT3NTM654/edit?usp=sharing

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Choose reviewer(s) and mention them in a comment (R: @username).
Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

`ValidatesRunner` compliance status (on master branch)

Lang	ULR	Twister2
Go	---	---
Java
Python	---	---
XLang		---

Examples testing status on various runners

Lang	ULR	Dataflow	Flink	Samza	Spark	Twister2
Go	---	---	---	---	---	---	---
Java	---		---	---	---	---	---
Python	---	---	---	---	---	---	---
XLang	---	---	---	---	---	---	---

Post-Commit SDK/Transform Integration Tests Status (on master branch)

Go	Java	Python

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website	Whitespace	Typescript
Non-portable
Portable	---			---	---	---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI.

chamikaramj · 2021-08-17T18:00:45Z

R: @robertwb

codecov · 2021-08-17T18:52:52Z

Codecov Report

Merging #15343 (8c75b93) into master (bd3649e) will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #15343      +/-   ##
==========================================
- Coverage   83.77%   83.75%   -0.03%     
==========================================
  Files         442      443       +1     
  Lines       60050    60077      +27     
==========================================
+ Hits        50308    50318      +10     
- Misses       9742     9759      +17

Impacted Files	Coverage Δ
...hon/apache_beam/runners/direct/test_stream_impl.py	`94.02% <0.00%> (-2.24%)`	⬇️
.../python/apache_beam/transforms/periodicsequence.py	`96.72% <0.00%> (-1.64%)`	⬇️
sdks/python/apache_beam/io/source_test_utils.py	`88.47% <0.00%> (-1.39%)`	⬇️
sdks/python/apache_beam/io/localfilesystem.py	`91.47% <0.00%> (-0.78%)`	⬇️
...apache_beam/runners/dataflow/internal/apiclient.py	`76.46% <0.00%> (-0.34%)`	⬇️
sdks/python/apache_beam/transforms/util.py	`95.81% <0.00%> (-0.17%)`	⬇️
setup.py	`0.00% <0.00%> (ø)`
...he_beam/portability/api/external_transforms_pb2.py	`100.00% <0.00%> (ø)`
...am/portability/api/external_transforms_pb2_urns.py	`0.00% <0.00%> (ø)`
...hon/apache_beam/runners/worker/bundle_processor.py	`93.64% <0.00%> (+0.12%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd3649e...8c75b93. Read the comment docs.

chamikaramj · 2021-08-22T17:11:36Z

cc: @lukecwik

chamikaramj · 2021-08-22T17:12:59Z

Added following based on the discussions in the doc and the mailing list.

Added a yaml based allowlist for classes/methods.
Added two annotations so that transform authors can customize constructor/builder method names offered.
Automatically simplifies the common builder pattern "withXyz" by also allowing the format "xyz".

PTAL

chamikaramj · 2021-08-24T18:49:08Z

Friendly ping :)

All tests pass now.

chamikaramj · 2021-08-25T19:05:01Z

R: @ihji as well

lukecwik

Does this work with complex types for method parameters like List and/or MyCustomUserType that can be loaded via Schema?

.../expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionService.java

...ion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionServiceOptions.java

...ce/src/main/java/org/apache/beam/sdk/expansion/service/JavaClassLookupTransformProvider.java

...rc/test/java/org/apache/beam/sdk/expansion/service/JavaCLassLookupTransformProviderTest.java

chamikaramj

Thanks. PTAL.

...ce/src/main/java/org/apache/beam/sdk/expansion/service/JavaClassLookupTransformProvider.java

.../expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionService.java

...ion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionServiceOptions.java

...ce/src/main/java/org/apache/beam/sdk/expansion/service/JavaClassLookupTransformProvider.java

...rc/test/java/org/apache/beam/sdk/expansion/service/JavaCLassLookupTransformProviderTest.java

chamikaramj · 2021-08-29T22:21:31Z

Regarding the "complex types" question. Yeah, this is expected to work for all types that can be represented by a Beam Row+schema.

model/pipeline/src/main/proto/external_transforms.proto

...ion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionServiceOptions.java

lukecwik · 2021-08-30T18:08:32Z

...ion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionServiceOptions.java

+import org.apache.beam.sdk.options.Description;
+import org.apache.beam.sdk.options.PipelineOptions;
+
+public interface ExpansionServiceOptions extends PipelineOptions {


interface comment

lukecwik · 2021-08-30T18:09:55Z

...ion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionServiceOptions.java

+
+  void setJavaClassLookupAllowlistFile(String file);
+
+  class JavaClassLookupAllowListFactory implements DefaultValueFactory<AllowList> {


Class comment, e.g.:

Loads the allow list from {@link #getJavaClassLookupAllowlistFile}, defaulting to an empty AllowList.

lukecwik · 2021-08-30T18:49:48Z

...rc/test/java/org/apache/beam/sdk/expansion/service/JavaCLassLookupTransformProviderTest.java

+            "intField1"));
+    payloadBuilder.addBuilderMethods(builderMethodBuilder);
+
+    testClassLookupExpansionRequestConstruction(payloadBuilder.build());


Check that strField1, strField2 and intField1 was set.

lukecwik · 2021-08-30T18:49:59Z

...rc/test/java/org/apache/beam/sdk/expansion/service/JavaCLassLookupTransformProviderTest.java

+            "intField1"));
+    payloadBuilder.addBuilderMethods(builderMethodBuilder);
+
+    testClassLookupExpansionRequestConstruction(payloadBuilder.build());


Check that strField1, strField2 and intField1 was set.

lukecwik · 2021-08-30T18:50:05Z

...rc/test/java/org/apache/beam/sdk/expansion/service/JavaCLassLookupTransformProviderTest.java

+            "intField1"));
+    payloadBuilder.addBuilderMethods(builderMethodBuilder);
+
+    testClassLookupExpansionRequestConstruction(payloadBuilder.build());


Check that strField1, strField2 and intField1 was set.

lukecwik · 2021-08-30T18:50:26Z

...rc/test/java/org/apache/beam/sdk/expansion/service/JavaCLassLookupTransformProviderTest.java

+            "intField1"));
+    payloadBuilder.addBuilderMethods(builderMethodBuilder);
+
+    testClassLookupExpansionRequestConstruction(payloadBuilder.build());


Check that strField1, abc and xyz was set.

lukecwik · 2021-08-30T18:53:07Z

...rc/test/java/org/apache/beam/sdk/expansion/service/JavaCLassLookupTransformProviderTest.java

+
+  public static class DummyTransform extends PTransform<PBegin, PCollection<String>> {
+
+    String strField1;


Add tests for a wrapper type (e.g. Double field), and a complex type (non string, primitive, wrapper), and a list of simple type and list of complex type

Thanks. I realized that we do not support some types yet. I added support for arrays.

Now we support:

Java primitive types and Strings.

Java types that can be represented by a Beam schema (if a schema has not
been registered service will try to generate a schema using
'JavaFieldSchema').

Arrays of supported types.

We do not support collections as top level parameters yet. This can be added in the future.

I clarified what's supported in the spec and added tests for complex types and arrays.

Added support for lists as well.

…class name and builder methods

Co-authored-by: Lukasz Cwik <lcwik@google.com>

chamikaramj

Thanks. PTAL.

chamikaramj · 2021-08-31T01:20:39Z

...ion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionServiceOptions.java

+import org.apache.beam.sdk.options.Description;
+import org.apache.beam.sdk.options.PipelineOptions;
+
+public interface ExpansionServiceOptions extends PipelineOptions {


chamikaramj · 2021-08-31T01:23:35Z

...ion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionServiceOptions.java

+
+  void setJavaClassLookupAllowlistFile(String file);
+
+  class JavaClassLookupAllowListFactory implements DefaultValueFactory<AllowList> {


chamikaramj · 2021-08-31T01:24:12Z

...ce/src/main/java/org/apache/beam/sdk/expansion/service/JavaClassLookupTransformProvider.java

+  public static final String ALLOW_LIST_VERSION = "v1";
+
+  public JavaClassLookupTransformProvider(AllowList allowList) {
+    if (!allowList.getVersion().equals(ALLOW_LIST_VERSION)) {


This will allow us to modify the format of the allow-list in the future while supporting old versions.

chamikaramj · 2021-08-31T18:31:29Z

...ce/src/main/java/org/apache/beam/sdk/expansion/service/JavaClassLookupTransformProvider.java

+
+    if (matchingMethods.size() != 1) {
+      throw new RuntimeException(
+          "Expected to find exact one matching method in transform "


chamikaramj · 2021-08-31T18:32:27Z

...rc/test/java/org/apache/beam/sdk/expansion/service/JavaCLassLookupTransformProviderTest.java

+                .build(),
+            "strField1"));
+
+    testClassLookupExpansionRequestConstruction(payloadBuilder.build());


chamikaramj · 2021-08-31T18:32:50Z

...rc/test/java/org/apache/beam/sdk/expansion/service/JavaCLassLookupTransformProviderTest.java

+            "intField1"));
+    payloadBuilder.addBuilderMethods(builderMethodBuilder);
+
+    testClassLookupExpansionRequestConstruction(payloadBuilder.build());


chamikaramj · 2021-08-31T18:33:11Z

...rc/test/java/org/apache/beam/sdk/expansion/service/JavaCLassLookupTransformProviderTest.java

+            "intField1"));
+    payloadBuilder.addBuilderMethods(builderMethodBuilder);
+
+    testClassLookupExpansionRequestConstruction(payloadBuilder.build());


chamikaramj · 2021-08-31T18:33:30Z

...rc/test/java/org/apache/beam/sdk/expansion/service/JavaCLassLookupTransformProviderTest.java

+            "intField1"));
+    payloadBuilder.addBuilderMethods(builderMethodBuilder);
+
+    testClassLookupExpansionRequestConstruction(payloadBuilder.build());


chamikaramj · 2021-08-31T18:33:47Z

...rc/test/java/org/apache/beam/sdk/expansion/service/JavaCLassLookupTransformProviderTest.java

+            "intField1"));
+    payloadBuilder.addBuilderMethods(builderMethodBuilder);
+
+    testClassLookupExpansionRequestConstruction(payloadBuilder.build());


chamikaramj · 2021-09-01T18:50:55Z

...rc/test/java/org/apache/beam/sdk/expansion/service/JavaCLassLookupTransformProviderTest.java

+
+  public static class DummyTransform extends PTransform<PBegin, PCollection<String>> {
+
+    String strField1;


Thanks. I realized that we do not support some types yet. I added support for arrays.

Now we support:

Java primitive types and Strings.

Java types that can be represented by a Beam schema (if a schema has not
been registered service will try to generate a schema using
'JavaFieldSchema').

Arrays of supported types.

We do not support collections as top level parameters yet. This can be added in the future.

I clarified what's supported in the spec and added tests for complex types and arrays.

…and each builder method. Updated the implementation accordingly and added additional tests.

chamikaramj

As discussed offline, I simplified the proto and the implementation by removing the "Parameter" message and defining a single schema/payload for the constructor and each builder method.

This allows us to support more types in a generic way.

Added tests for complex types, arrays and lists.

PTAL. Thanks.

chamikaramj · 2021-09-03T02:53:39Z

...rc/test/java/org/apache/beam/sdk/expansion/service/JavaCLassLookupTransformProviderTest.java

+
+  public static class DummyTransform extends PTransform<PBegin, PCollection<String>> {
+
+    String strField1;


Added support for lists as well.

chamikaramj · 2021-09-03T16:01:49Z

Run Java PreCommit

chamikaramj · 2021-09-03T20:25:21Z

Run Java PreCommit

lukecwik

LGTM,

mostly comment changes and nits

great work on expanding the set of types and simplifying down to one schema + row per method

lukecwik · 2021-09-03T21:55:57Z

model/pipeline/src/main/proto/external_transforms.proto

+  // Name of the builder method
+  string name = 1;
+
+  // A schema that describes the parameter of the constructor or the constructor


Suggested change

// A schema that describes the parameter of the constructor or the constructor

// A schema that describes the parameters of the constructor or the constructor's

Outdated now.

lukecwik · 2021-09-03T21:56:50Z

model/pipeline/src/main/proto/external_transforms.proto

+  // The top level fields of the schema represents the parameters in order.
+  // Top level field names map to parameter names to use.


Can you disambiguate when we choose names over field ordering?

We check the number of fields as well as the names of the fields in order.

lukecwik · 2021-09-03T21:58:42Z

model/pipeline/src/main/proto/external_transforms.proto

+  // Top level field names map to parameter names to use.
+  Schema schema = 2;
+
+  // A payload that maps to the provided builder method schema.


We should state that we expect beam:coder:row:v1 encoding of the schema.

lukecwik · 2021-09-03T21:58:48Z

model/pipeline/src/main/proto/external_transforms.proto

+  // Top level field names map to parameter names to use.
+  Schema constructor_schema = 4;
+
+  // A payload that maps to the provided constructor schema.


We should state that we expect beam:coder:row:v1 encoding of the schema.

lukecwik · 2021-09-03T21:59:44Z

model/pipeline/src/main/proto/external_transforms.proto

+  // transform object is constructed.
+  // Given builder methods will be invoked in order when constructing the
+  // transform objects.
+  repeated BuilderMethod builder_methods = 3;


nit: re-order to place builder methods at the bottom so that constructor fields are grouped together.

lukecwik · 2021-09-03T22:02:43Z

model/pipeline/src/main/proto/external_transforms.proto

+  // Top level field names map to parameter names to use.
+  Schema schema = 2;
+
+  // A payload that maps to the provided builder method schema.


We should state that we expect beam:coder:row:v1 encoding of the schema.

lukecwik · 2021-09-03T22:17:57Z

model/pipeline/src/main/proto/external_transforms.proto

+  // The top level fields of the schema represent the parameters in order.
+  // Top level field names map to parameter names to use.


Suggested change

// The top level fields of the schema represent the parameters in order.

// Top level field names map to parameter names to use.

// The top level fields of the schema represent the method parameters in order.

// If able, top level field names are also verified against the method parameters for a match.

Mapping names sounds like we will use the names to map parameters and not the field order. We seem to only use the names for validation that the correct parameter is being used.

lukecwik · 2021-09-03T22:18:13Z

model/pipeline/src/main/proto/external_transforms.proto

+  // The top level fields of the schema represents the parameters in order.
+  // Top level field names map to parameter names to use.


Suggested change

// The top level fields of the schema represents the parameters in order.

// Top level field names map to parameter names to use.

// The top level fields of the schema represent the method parameters in order.

// If able, top level field names are also verified against the method parameters for a match.

Ditto here as well.

chamikaramj

Thanks!

chamikaramj · 2021-09-04T03:04:58Z

model/pipeline/src/main/proto/external_transforms.proto

+  // transform object is constructed.
+  // Given builder methods will be invoked in order when constructing the
+  // transform objects.
+  repeated BuilderMethod builder_methods = 3;


chamikaramj · 2021-09-04T03:05:42Z

model/pipeline/src/main/proto/external_transforms.proto

+  // The top level fields of the schema represent the parameters in order.
+  // Top level field names map to parameter names to use.


chamikaramj · 2021-09-04T03:06:35Z

model/pipeline/src/main/proto/external_transforms.proto

+  // Top level field names map to parameter names to use.
+  Schema constructor_schema = 4;
+
+  // A payload that maps to the provided constructor schema.


chamikaramj · 2021-09-04T03:07:37Z

model/pipeline/src/main/proto/external_transforms.proto

+  // The top level fields of the schema represents the parameters in order.
+  // Top level field names map to parameter names to use.


chamikaramj · 2021-09-04T03:07:46Z

model/pipeline/src/main/proto/external_transforms.proto

+  // Top level field names map to parameter names to use.
+  Schema schema = 2;
+
+  // A payload that maps to the provided builder method schema.


chamikaramj · 2021-09-04T03:15:24Z

model/pipeline/src/main/proto/external_transforms.proto

+  // Name of the builder method
+  string name = 1;
+
+  // A schema that describes the parameter of the constructor or the constructor


Outdated now.

chamikaramj · 2021-09-04T03:15:34Z

model/pipeline/src/main/proto/external_transforms.proto

+  // Top level field names map to parameter names to use.
+  Schema schema = 2;
+
+  // A payload that maps to the provided builder method schema.


chamikaramj · 2021-09-04T03:18:08Z

model/pipeline/src/main/proto/external_transforms.proto

+  // The top level fields of the schema represents the parameters in order.
+  // Top level field names map to parameter names to use.


We check the number of fields as well as the names of the fields in order.

chamikaramj · 2021-09-04T03:28:31Z

Run XVR_Dataflow PostCommit

chamikaramj · 2021-09-04T03:29:18Z

Run XVR_Flink PostCommit

chamikaramj · 2021-09-04T06:16:14Z

Run Java PreCommit

chamikaramj · 2021-09-04T06:16:23Z

Run Java_Examples_Dataflow PreCommit

chamikaramj · 2021-09-04T16:43:07Z

Run Java PreCommit

chamikaramj · 2021-09-04T19:01:42Z

PreCommit failures are unrelated but trying again.

chamikaramj · 2021-09-04T19:01:46Z

Run Java PreCommit

…rm using the class name and builder methods (apache#15343) * Adds support for expanding a Java cross-language transform using the class name and builder methods * Adds an allowlist and adds support for annotations * Fix tests * Address CheckerFramework errors * Adds license * Addresses reviewer comments. * Apply suggestions from code review Co-authored-by: Lukasz Cwik <lcwik@google.com> * Addresses reviewer comments. * Updated the proto to include a single schema/payload for constructor and each builder method. Updated the implementation accordingly and added additional tests. * Some doc updates and few other minor updates. * Addressing reviewer comments Co-authored-by: Lukasz Cwik <lcwik@google.com>

chamikaramj requested a review from robertwb August 17, 2021 18:00

lukecwik requested changes Aug 27, 2021

View reviewed changes

chamikaramj commented Aug 29, 2021

View reviewed changes

lukecwik reviewed Aug 30, 2021

View reviewed changes

chamikaramj and others added 8 commits August 30, 2021 17:34

Adds support for expanding a Java cross-language transform using the …

cacc25f

…class name and builder methods

Adds an allowlist and adds support for annotations

f7e0967

Fix tests

81d1390

Address CheckerFramework errors

8018c8f

Adds license

9db9a10

Addresses reviewer comments.

ae94c4d

Apply suggestions from code review

131e0c3

Co-authored-by: Lukasz Cwik <lcwik@google.com>

Addresses reviewer comments.

8ae5690

chamikaramj force-pushed the xlang_api_simplify branch from 58a192b to 8ae5690 Compare September 1, 2021 19:31

chamikaramj commented Sep 1, 2021

View reviewed changes

Updated the proto to include a single schema/payload for constructor …

0040802

…and each builder method. Updated the implementation accordingly and added additional tests.

chamikaramj commented Sep 3, 2021

View reviewed changes

Some doc updates and few other minor updates.

10cf7dd

lukecwik approved these changes Sep 3, 2021

View reviewed changes

Addressing reviewer comments

8c75b93

chamikaramj commented Sep 4, 2021

View reviewed changes

chamikaramj merged commit 1455c54 into apache:master Sep 4, 2021

lukecwik mentioned this pull request Sep 8, 2021

[BEAM-12769] Fix typo in test class name, CLass -> Class #15481

Merged

4 tasks

chamikaramj mentioned this pull request Sep 8, 2021

[BEAM-12769] Python support for directly using Java transforms using constructor and builder methods #15484

Merged

4 tasks


		void setJavaClassLookupAllowlistFile(String file);

		class JavaClassLookupAllowListFactory implements DefaultValueFactory<AllowList> {


		public static class DummyTransform extends PTransform<PBegin, PCollection<String>> {

		String strField1;

	// A schema that describes the parameter of the constructor or the constructor
	// A schema that describes the parameters of the constructor or the constructor's

		// The top level fields of the schema represents the parameters in order.
		// Top level field names map to parameter names to use.

[BEAM-12769] Adds support for expanding a Java cross-language transform using the class name and builder methods #15343

[BEAM-12769] Adds support for expanding a Java cross-language transform using the class name and builder methods #15343

Conversation

chamikaramj commented Aug 17, 2021

ValidatesRunner compliance status (on master branch)

Examples testing status on various runners

Post-Commit SDK/Transform Integration Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

GitHub Actions Tests Status (on master branch)

chamikaramj commented Aug 17, 2021

codecov bot commented Aug 17, 2021 • edited

Codecov Report

chamikaramj commented Aug 22, 2021

chamikaramj commented Aug 22, 2021

chamikaramj commented Aug 24, 2021

chamikaramj commented Aug 25, 2021

lukecwik left a comment

Choose a reason for hiding this comment

chamikaramj left a comment

Choose a reason for hiding this comment

chamikaramj commented Aug 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chamikaramj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chamikaramj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chamikaramj commented Sep 3, 2021

chamikaramj commented Sep 3, 2021

lukecwik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chamikaramj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

`ValidatesRunner` compliance status (on master branch)

codecov bot commented Aug 17, 2021 •

edited