[BEAM-1928] Add ParDos by tgroh · Pull Request #2597 · apache/beam

tgroh · 2017-04-19T21:22:39Z

Add ParDoPayloadTranslator to PTransformTranslator

Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

Make sure the PR title is formatted like:
[BEAM-<Jira issue #>] Description of pull request
Make sure tests pass via mvn clean verify. (Even better, enable
Travis-CI on your fork and ensure the whole test matrix passes).
Replace <Jira issue #> in the title with the actual Jira issue
number, if there is one.
If this contribution is large, please file an Apache
Individual Contributor License Agreement.

davorbonaci · 2017-04-27T16:41:18Z

More details please?

tgroh · 2017-05-15T18:12:29Z

R: @kennknowles

Add ParDoPayloadTranslator to PTransformTranslator

coveralls · 2017-05-16T00:30:17Z

Coverage decreased (-0.003%) to 70.219% when pulling a1a66d1 on tgroh:par_do_serde_protos into 13e1be2 on apache:master.

kennknowles

This is a really good start.

kennknowles · 2017-05-16T20:14:37Z

...tion-java/src/main/java/org/apache/beam/runners/core/construction/RunnerPCollectionView.java

+import org.apache.beam.sdk.values.TupleTag;
+import org.apache.beam.sdk.values.WindowingStrategy;
+
+/** Created by tgroh on 5/15/17. */


kennknowles · 2017-05-16T20:15:47Z

sdks/common/runner-api/src/main/proto/beam_runner_api.proto

+  string id = 1;
+  Type type = 2;
+
+  enum Type {


Prefer top-level definitions, like StateType, so we don't couple names or generated classes.

Reverted for the time being.

kennknowles · 2017-05-16T20:17:07Z

...e-construction-java/src/main/java/org/apache/beam/runners/core/construction/PTransforms.java

  private static final Map<Class<? extends PTransform>, TransformPayloadTranslator>
      KNOWN_PAYLOAD_TRANSLATORS =
-          ImmutableMap.<Class<? extends PTransform>, TransformPayloadTranslator>builder().build();
+          ImmutableMap.<Class<? extends PTransform>, TransformPayloadTranslator>builder()


You can remove ParDoPayload from the TODO (or just remove the TODO)

kennknowles · 2017-05-16T20:17:40Z

...s/core-construction-java/src/main/java/org/apache/beam/runners/core/construction/ParDos.java

+  /**
+   * The URN for a {@link ParDoPayload}.
+   */
+  public static final String PAR_DO_PAYLOAD_URN = "urn:beam:runnerapi:pardo:v1";


I don't think we need runnerapi in the URNs. This is the spec for a "Beam ParDo transform" (v1).

kennknowles · 2017-05-16T20:18:13Z

...s/core-construction-java/src/main/java/org/apache/beam/runners/core/construction/ParDos.java

+   */
+  public static final String CUSTOM_JAVA_WINDOW_MAPPING_FN_URN =
+      "urn:beam:windowmappingfn:javasdk:0.1";
+


(nit) extraneous newline

kennknowles · 2017-05-16T20:21:59Z

...s/core-construction-java/src/main/java/org/apache/beam/runners/core/construction/ParDos.java

+            return null;
+          }
+        };
+    Type parameterType = parameter.match(runnerApiCases);


(nit) I prefer putting the new Cases inline so it reads like the enhanced switch that it is.

kennknowles · 2017-05-16T20:24:38Z

...s/core-construction-java/src/main/java/org/apache/beam/runners/core/construction/ParDos.java

+    return builder.build();
+  }
+
+  public static PCollectionView<?> fromProto(


At some point fromProto and toProto are going to get unreadable if they depend on a variety of params for override.

It's not a great scene. This one ends up needing a bunch of context because things like the PCollection are inputs of the PTransform and are only referenced by (two-indirection name) within the SideInput.

Some other things are fooFromProto, which I'm not opposed to.

Also reordered the parameters to make the conversion target first, then context

kennknowles · 2017-05-16T20:28:48Z

...s/core-construction-java/src/main/java/org/apache/beam/runners/core/construction/ParDos.java

+            components);
+    Coder<?> elemCoder =
+        Coders.fromProto(components.getCodersOrThrow(inputCollection.getCoderId()), components);
+    Coder<Iterable<WindowedValue<?>>> coder =


This should be keyed on the materialization. This is the appropriate coder for our current "iterable of full windowed values" materialization. It is OK to not implement any others, but there should be a checkArgument or a conditional with a failing else branch with a meaningful message in an Unsupported exception.

Throwing an exception if an unknown type for the time being.

kennknowles · 2017-05-16T20:29:29Z

...s/core-construction-java/src/main/java/org/apache/beam/runners/core/construction/ParDos.java

+                    Any.pack(
+                        BytesValue.newBuilder()
+                            .setValue(
+                                ByteString.copyFrom(SerializableUtils.serializeToByteArray(viewFn)))


I think we could probably add a couple more layers of indirection here...

Couldn't we just, though

kennknowles · 2017-05-16T20:30:37Z

...tion-java/src/main/java/org/apache/beam/runners/core/construction/RunnerPCollectionView.java

+import org.apache.beam.sdk.values.WindowingStrategy;
+
+/** Created by tgroh on 5/15/17. */
+class RunnerPCollectionView<T> extends PValueBase


I suspect this class is a harbinger of things to come in terms of RunnerParDo, etc. These should only include the primitive constructs. It might turn out quite nice, actually.

coveralls · 2017-05-16T23:05:05Z

Coverage increased (+0.5%) to 70.734% when pulling 67bb96c on tgroh:par_do_serde_protos into 13e1be2 on apache:master.

coveralls · 2017-05-17T21:49:50Z

Coverage increased (+0.007%) to 70.759% when pulling 4643106 on tgroh:par_do_serde_protos into 5e3c5c6 on apache:master.

kennknowles

LGTM

kennknowles · 2017-05-18T18:40:41Z

...s/core-construction-java/src/main/java/org/apache/beam/runners/core/construction/ParDos.java

+
+  private static Optional<RunnerApi.Parameter> toProto(Parameter parameter) {
+    return parameter.match(
+        new Cases<Optional<RunnerApi.Parameter>>() {


I think there's a Cases.Default that lets you put all the unimplemented cases in one place a la switch..default:. Not important, just a tidying.

coveralls · 2017-05-18T21:59:00Z

Coverage increased (+0.003%) to 70.789% when pulling 207085c on tgroh:par_do_serde_protos into 8c572ef on apache:master.

tgroh force-pushed the par_do_serde_protos branch 2 times, most recently from 0061a4a to 7250253 Compare May 15, 2017 18:12

Add ParDos

a1a66d1

Add ParDoPayloadTranslator to PTransformTranslator

tgroh force-pushed the par_do_serde_protos branch from 7250253 to a1a66d1 Compare May 15, 2017 23:08

kennknowles reviewed May 16, 2017

View reviewed changes

tgroh force-pushed the par_do_serde_protos branch 2 times, most recently from ea46bb1 to cf52b9c Compare May 17, 2017 19:42

fixup! Add ParDos

4643106

tgroh force-pushed the par_do_serde_protos branch from cf52b9c to 4643106 Compare May 17, 2017 20:31

kennknowles approved these changes May 18, 2017

View reviewed changes

tgroh force-pushed the par_do_serde_protos branch from e543656 to f11c673 Compare May 18, 2017 20:24

fixup! Add ParDos

207085c

tgroh force-pushed the par_do_serde_protos branch from f11c673 to 207085c Compare May 18, 2017 20:45

asfgit closed this in 2e0cf00 May 18, 2017

Conversation

tgroh commented Apr 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davorbonaci commented Apr 27, 2017

Uh oh!

tgroh commented May 15, 2017

Uh oh!

coveralls commented May 16, 2017

Uh oh!

kennknowles 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!

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!

coveralls commented May 16, 2017

Uh oh!

coveralls commented May 17, 2017

Uh oh!

kennknowles 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!

coveralls commented May 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tgroh commented Apr 19, 2017 •

edited

Loading