[BEAM-2863] Add the ability to length prefix unknown coders by lukecwik · Pull Request #4377 · apache/beam

lukecwik · 2018-01-10T01:35:06Z

Follow this checklist to help us incorporate your contribution quickly and easily:

Make sure there is a JIRA issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
Each commit in the pull request should have a meaningful subject line and body.
Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Run mvn clean verify to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

Add the ability to length prefix unknown coders using the portable representation allowing a Runner to not need to know about all coder representations.

This is towards supporting the side inputs over the portability framework but can also be used for the data plane.

tgroh · 2018-01-11T00:28:48Z

...ution/src/main/java/org/apache/beam/runners/fnexecution/graph/LengthPrefixUnknownCoders.java

+      RunnerApi.Components components,
+      boolean replaceWithByteArrayCoder) {
+
+    MessageWithComponents.Builder builder = MessageWithComponents.newBuilder();


the variable name builder is giving me headaches when combined with getSpecBuilder().getSpecBuilder() and other builders flying around; Mind converting into something like resultBuilder?

tgroh · 2018-01-11T00:29:44Z

...ution/src/main/java/org/apache/beam/runners/fnexecution/graph/LengthPrefixUnknownCoders.java

+   * @param replaceWithByteArrayCoder whether to replace an unknown coder with a
+   * {@link ByteArrayCoder}.
+   * @return A {@link MessageWithComponents} with the
+   * {@link MessageWithComponents#getCoder() root coder} and its component coders.


I believe it to be notable that no IDs in the result will collide with any IDs in the input components for different coders.

tgroh · 2018-01-11T00:51:18Z

...ution/src/main/java/org/apache/beam/runners/fnexecution/graph/LengthPrefixUnknownCoders.java

+        builder.setComponents(components);
+      }
+    } else if (WELL_KNOWN_CODER_URNS.contains(currentCoder.getSpec().getSpec().getUrn())) {
+      RunnerApi.Coder.Builder updatedCoder = currentCoder.toBuilder();


This seems sufficiently involved (given the recursive subcomponent rewriting) that it could stand being in a separate method. This might be true for the bulk of the replaceWithByteArrayCoder branches as well

Split out to different methods covering the different cases.

tgroh · 2018-01-11T00:57:55Z

...n/src/test/java/org/apache/beam/runners/fnexecution/graph/LengthPrefixUnknownCodersTest.java

+
+/** Tests for {@link LengthPrefixUnknownCoders}. */
+@RunWith(JUnit4.class)
+public class LengthPrefixUnknownCodersTest {


This seems like it would read better as a Parameterized test (with the [Original, Result, replaceWithByteArray] as your parameters)

tgroh · 2018-01-11T01:00:11Z

...n/src/test/java/org/apache/beam/runners/fnexecution/graph/LengthPrefixUnknownCodersTest.java

+  }
+
+  /** Test replacing unknown coders with {@code LengthPrefixCoder<ByteArray>}. */
+  @Test


Test including LengthPrefixCoder at the top level?

tgroh · 2018-01-11T01:01:31Z

...n/src/test/java/org/apache/beam/runners/fnexecution/graph/LengthPrefixUnknownCodersTest.java

+          LengthPrefixCoder.of(ByteArrayCoder.of())),
+      GlobalWindow.Coder.INSTANCE);
+
+  /** Test wrapping unknown coders with {@code LengthPrefixCoder}. */


I never remember if UTF-8 Strings are well known or not; maybe consider a CustomCoder just for the obviousness of it being unknown?

tgroh

minor style things, otherwise lgtm

tgroh · 2018-01-12T20:50:31Z

...ution/src/main/java/org/apache/beam/runners/fnexecution/graph/LengthPrefixUnknownCoders.java

+    String lengthPrefixComponentCoderId = coderId;
+    if (replaceWithByteArrayCoder) {
+      return createLengthPrefixByteArrayCoder(coderId, components);
+      // lengthPrefixComponentCoderId = generateUniqueId(coderId + "-byte_array",


rm the comments

Done. Forgot that this was here.

tgroh · 2018-01-12T22:16:54Z

...n/src/test/java/org/apache/beam/runners/fnexecution/graph/LengthPrefixUnknownCodersTest.java

+  public static Collection<Object[]> data() {
+    return ImmutableList.of(
+        /** Test wrapping unknown coders with {@code LengthPrefixCoder}. */
+        new Object[] {


I prefer the use of an AutoValue inner class here instead of an Object[]

I'm going to stick with the Object[] for now since it will make the migration to JUnit 5 parameterized tests simpler.

tgroh · 2018-01-12T22:21:06Z

...n/src/test/java/org/apache/beam/runners/fnexecution/graph/LengthPrefixUnknownCodersTest.java

-      Coder<?> original, Coder<?> expected, boolean replaceWithByteArray) throws IOException {
+  @Test
+  public void test() throws IOException {
    MessageWithComponents messageWithComponents = CoderTranslation.toProto(original);


Something like 'originalCoderProto', 'lengthPrefixedCoderProto', etc?

… portable representation allowing a Runner to not need to know about all coder representations. This is towards supporting the side inputs over the portability framework.

lukecwik requested review from aaltay and tgroh January 10, 2018 01:35

lukecwik assigned tgroh Jan 10, 2018

tgroh reviewed Jan 11, 2018

View reviewed changes

tgroh approved these changes Jan 12, 2018

View reviewed changes

lukecwik force-pushed the side_input4 branch from 0bcd4bd to d2ac0b5 Compare January 12, 2018 22:35

[BEAM-2863] Add the ability to length prefix unknown coders using the…

8c61010

… portable representation allowing a Runner to not need to know about all coder representations. This is towards supporting the side inputs over the portability framework.

lukecwik force-pushed the side_input4 branch from d2ac0b5 to 8c61010 Compare January 12, 2018 22:35

lukecwik merged commit 02bd77a into apache:master Jan 12, 2018

Conversation

lukecwik commented Jan 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lukecwik commented Jan 10, 2018 •

edited

Loading