[BEAM-1467] Add cross-SDK implementations and tests of WindowedValueCoder by vikkyrk · Pull Request #2018 · apache/beam

vikkyrk · 2017-02-16T03:01:35Z

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

Tests for GlobalWindowCoder
Cross-sdk implementation / tests for WindowedValueCoder
Note: PaneInfo isn't supported in python SDK yet, so we hard code the value to 0x0F which represents PaneInfo.NO_FIRING
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.

vikkyrk · 2017-02-16T03:03:59Z

R: @dhalperi @aaltay
CC: @robertwb @tgroh

vikkyrk · 2017-02-16T03:28:11Z

sdks/python/apache_beam/coders/coder_impl.py

+        # lower negative number. For ex: -3 / 2 = -2, but we expect it to be -1,
+        # to be consistent across SDKs.
+        self._from_normal_time(
+            restore_sign * (abs(wv.timestamp_micros) / 1000)))


Better ways to fix this?

int(-3/2.0) should work.

Much better! Any performance implications dividing by float and converting to int? (I think not)

I do not know, but I do not think so.

Actually float division loses precision for large long numbers, so this won't work.

coveralls · 2017-02-16T03:34:36Z

Changes Unknown when pulling 61b964d on vikkyrk:common_windowed_value_coder into ** on apache:master**.

asfbot · 2017-02-16T03:40:13Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/7464/
--none--

coveralls · 2017-02-16T04:28:10Z

Changes Unknown when pulling 61b964d on vikkyrk:common_windowed_value_coder into ** on apache:master**.

asfbot · 2017-02-16T04:28:38Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/7467/
--none--

aaltay · 2017-02-16T17:14:28Z

Attach JIRA issues to TODOs, I will defer to R: @charlesccychen for review.

dhalperi

Java and standard_coders LGTM, with minor comments. @aaltay , feel free to merge once you're happy with Python side?

dhalperi · 2017-02-17T07:07:47Z

sdks/java/core/src/test/java/org/apache/beam/sdk/coders/CommonCoderTest.java

              convertValue(element, coderSpec.getComponents().get(0), elementCoder));
        }
        return convertedElements;
+      case "urn:beam:coders:global_window:0.1":


I must have missed this prior for IterableCoder, but it can be useful to put these case statements fully inside of {} (see interval_window case above) -- that lets each case use a different set of local variables.

dhalperi · 2017-02-17T07:12:54Z

sdks/java/core/src/test/java/org/apache/beam/sdk/coders/CommonCoderTest.java

+              windowCoder));
+        }
+        // Note: Until Python SDK supports PaneInfo, we default to PaneInfo.NO_FIRING.
+        return WindowedValue.of(windowValue, timestamp, windows, PaneInfo.NO_FIRING);


Suggest that:

document the Python issue with a JIRA.

Restrict the test values to those with PaneInfo.NO_FIRING, and add a TODO in standard_coders.yaml referencing the JIRA.

return the correct value in Java.

Seems cleaner -- fewer places to clean up once Python has PaneInfo.

dhalperi · 2017-02-17T07:14:02Z

sdks/python/apache_beam/coders/coder_impl.py

+            restore_sign * (abs(wv.timestamp_micros) / 1000)))
    self._windows_coder.encode_to_stream(wv.windows, out, True)
+    # Default PaneInfo encoded byte representing NO_FIRING.
+    # TODO(vikasrk): Remove the hard coding here once PaneInfo is supported.


instead of TODO(vikasrk) make it TODO(BEAM_XXX) to a JIRA. :)

charlesccychen · 2017-02-17T20:12:59Z

sdks/python/apache_beam/coders/coder_impl.py


+  # TODO: Fn Harness only supports millis. Is this important enough to fix?
+  def _to_normal_time(self, value):
+    """Convert "lexicographically ordered unsigned" to signed."""


Can you document what "normal time" means for coders? What is "lexicographically ordered unsigned"?

charlesccychen · 2017-02-17T20:14:26Z

sdks/python/apache_beam/coders/coder_impl.py

-    self._value_coder.encode_to_stream(wv.value, out, True)
    # Avoid creation of Timestamp object.
-    out.write_bigendian_int64(wv.timestamp_micros)
+    restore_sign = -1 if wv.timestamp_micros < 0 else 1


Can you add a comment that this workaround is to maintain compatibility with the Java SDK and point to a jira issue to resolve this in the future?

charlesccychen · 2017-02-17T20:15:18Z

sdks/python/apache_beam/coders/coder_impl.py

+    # of precision while converting to millis.
+    # Note: This is only a best effort here as there is no way to know if these
+    # were indeed MIN/MAX timestamps.
+    # TODO(vikasrk): Clean this up once we have a BEAM wide consensus on


Please point to jira.

charlesccychen · 2017-02-17T20:16:29Z

sdks/python/apache_beam/coders/coder_impl.py

+    # were indeed MIN/MAX timestamps.
+    # TODO(vikasrk): Clean this up once we have a BEAM wide consensus on
+    # precision of timestamps.
+    if timestamp == -(abs(MIN_TIMESTAMP.micros) / 1000):


Do we want to do <= and >= here and below? This is somewhat fragile. Can you also document the Java min and max values?

This should be go away once the BEAM-1524 is fixed. Added a jira.

charlesccychen · 2017-02-17T20:16:58Z

sdks/python/apache_beam/coders/coder_impl.py

+
+    windows = self._windows_coder.decode_from_stream(in_stream, True)
+    # Read PaneInfo encoded byte.
+    in_stream.read_byte()


Please make it clearer in the comment that this is ignored for now.

This reverts commit 359017c.

coveralls · 2017-02-21T23:43:33Z

Coverage increased (+0.02%) to 69.307% when pulling f3cc9f8 on vikkyrk:common_windowed_value_coder into 154c543 on apache:master.

asfbot · 2017-02-21T23:44:40Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/7656/

Failed Tests: 1

beam_PreCommit_Java_MavenInstall/org.apache.beam:beam-runners-spark: 1

org.apache.beam.runners.spark.WatermarkTest.testInDoFn

--none--

vikkyrk · 2017-02-21T23:44:42Z

R: @charlesccychen addressed comments, PTAL.

charlesccychen · 2017-02-21T23:51:09Z

Thanks! LGTM.

coveralls · 2017-02-22T00:00:55Z

Coverage increased (+0.03%) to 69.314% when pulling f3cc9f8 on vikkyrk:common_windowed_value_coder into 154c543 on apache:master.

asfbot · 2017-02-22T00:01:24Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/7658/

Failed Tests: 1

beam_PreCommit_Java_MavenInstall/org.apache.beam:beam-runners-spark: 1

org.apache.beam.runners.spark.WatermarkTest.testInDoFn

--none--

vikkyrk · 2017-02-22T00:29:56Z

Retest this please.

coveralls · 2017-02-22T01:21:17Z

Coverage increased (+0.04%) to 69.323% when pulling f3cc9f8 on vikkyrk:common_windowed_value_coder into 154c543 on apache:master.

asfbot · 2017-02-22T01:22:01Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/7664/
--none--

vikkyrk changed the title ~~[BEAM-1467] dd cross-SDK implementations and tests of WindowedValueCoder~~ [BEAM-1467] Add cross-SDK implementations and tests of WindowedValueCoder Feb 16, 2017

vikkyrk commented Feb 16, 2017

View reviewed changes

dhalperi approved these changes Feb 17, 2017

View reviewed changes

charlesccychen reviewed Feb 17, 2017

View reviewed changes

vikkyrk added 6 commits February 21, 2017 15:07

Add cross-SDK implementations and tests of WindowedValueCoder

7bf481f

Fix lint errors

0305e5f

Cleanup

b89d527

Handle negative int division

55d58db

Revert "Handle negative int division"

90a7d8e

This reverts commit 359017c.

Add jiras and address comments

f3cc9f8

asfgit closed this in ede77c1 Feb 22, 2017

Conversation

vikkyrk commented Feb 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vikkyrk commented Feb 16, 2017 • 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!

coveralls commented Feb 16, 2017

Uh oh!

asfbot commented Feb 16, 2017

Uh oh!

coveralls commented Feb 16, 2017

Uh oh!

asfbot commented Feb 16, 2017

Uh oh!

aaltay commented Feb 16, 2017

Uh oh!

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

charlesccychen Feb 17, 2017 • 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!

coveralls commented Feb 21, 2017

Uh oh!

asfbot commented Feb 21, 2017

Failed Tests: 1

beam_PreCommit_Java_MavenInstall/org.apache.beam:beam-runners-spark: 1

Uh oh!

vikkyrk commented Feb 21, 2017

Uh oh!

charlesccychen commented Feb 21, 2017

Uh oh!

coveralls commented Feb 22, 2017

Uh oh!

asfbot commented Feb 22, 2017

Failed Tests: 1

beam_PreCommit_Java_MavenInstall/org.apache.beam:beam-runners-spark: 1

Uh oh!

vikkyrk commented Feb 22, 2017

Uh oh!

vikkyrk commented Feb 16, 2017 •

edited

Loading

vikkyrk commented Feb 16, 2017 •

edited

Loading

charlesccychen Feb 17, 2017 •

edited

Loading