[BEAM-3759] Add support for PaneInfo in WindowedValues #4763

charlesccychen · 2018-02-27T23:55:13Z

This change allows triggering information to be stored along with WindowedValues.

charlesccychen · 2018-02-27T23:55:22Z

charlesccychen · 2018-02-27T23:56:14Z

sdks/python/apache_beam/utils/windowed_value.py

  """

-  def __init__(self, value, timestamp, windows):
+  def __init__(self, value, timestamp, windows, pane_info=None):


Robert, can you comment on whether this extra kwarg is suitable, performance-wise?

It should be fine, as long as it's not usually passed as a kwarg. (Also, most of the time we create these with .with_value which is more optimized.)

robertwb · 2018-03-08T21:42:28Z

sdks/python/apache_beam/coders/coder_impl.py

@@ -670,6 +670,85 @@ def _construct_from_sequence(self, components):
    return components


+# A PaneInfo descriptor can be encoded in three different ways: (1) with a
+# single byte (PANE_INFO_ENCODING_FIRST), (2) with a single byte followed by
+# a varint describing the a single index


Or perhaps just remove this, as the comment below is sufficient.

robertwb · 2018-03-08T21:50:11Z

sdks/python/apache_beam/utils/windowed_value.py

@@ -104,11 +211,12 @@ def __reduce__(self):


 # TODO(robertwb): Move this to a static method.
-def create(value, timestamp_micros, windows):
+def create(value, timestamp_micros, windows, pane_info=None):


Just let the default value be PANE_INFO_UNKNOWN.

robertwb · 2018-03-08T21:50:47Z

sdks/python/apache_beam/utils/windowed_value.py

@@ -90,7 +196,8 @@ def __cmp__(left, right):  # pylint: disable=no-self-argument
  def _typed_eq(left, right):
    return (left.timestamp_micros == right.timestamp_micros
            and left.value == right.value
-            and left.windows == right.windows)
+            and left.windows == right.windows
+            and left.pane_info == right.pane_info)

  def with_value(self, new_value):
    """Creates a new WindowedValue with the same timestamps and windows as this.


Fix with_value to propagate the pane info (and test).

robertwb · 2018-03-08T21:51:10Z

sdks/python/apache_beam/utils/windowed_value.py

@@ -104,11 +211,12 @@ def __reduce__(self):



Fix __reduce__ to rememeber the PaneInfo (and test).

robertwb · 2018-03-08T21:54:11Z

sdks/python/apache_beam/utils/windowed_value.py

+
+
+def _construct_pane_info_map():
+  result = {}


Make this a list, not a map.

robertwb · 2018-03-08T21:58:49Z

sdks/python/apache_beam/coders/coders.py


  def _create_impl(self):
    return coder_impl.WindowedValueCoderImpl(
        self.wrapped_value_coder.get_impl(),
        self.timestamp_coder.get_impl(),
-        self.window_coder.get_impl())
+        self.window_coder.get_impl(),
+        self.pane_info_coder.get_impl())


Rather than introducing a PaneInfoCoder, just instantiate the PaneInfoCoderImpl in WindowedValueCoderImpl's constructor. (There's a TODO to remove the parameterizablility of the TimestampCoder as well.) Actually, we could consider just making {en,de}code_pane_info methods on WindowedValueCoder itself.

robertwb · 2018-03-08T21:59:21Z

sdks/python/apache_beam/coders/coder_impl.py

@@ -734,13 +814,14 @@ def decode_from_stream(self, in_stream, nested):
    # Read PaneInfo encoded byte.
    # TODO(BEAM-1522): Ignored for now but should be converted to pane info once


You can now remove this TODO.

aaltay · 2018-03-17T00:19:14Z

@charlesccychen any updates on this?

This change allows triggering information to be stored along with WindowedValues.

charlesccychen

Thanks, PTAL.

charlesccychen · 2018-03-19T06:43:29Z

sdks/python/apache_beam/utils/windowed_value.py

@@ -90,7 +196,8 @@ def __cmp__(left, right):  # pylint: disable=no-self-argument
  def _typed_eq(left, right):
    return (left.timestamp_micros == right.timestamp_micros
            and left.value == right.value
-            and left.windows == right.windows)
+            and left.windows == right.windows
+            and left.pane_info == right.pane_info)

  def with_value(self, new_value):
    """Creates a new WindowedValue with the same timestamps and windows as this.


robertwb wrote:
Fix with_value to propagate the pane info (and test).

Done.

charlesccychen · 2018-03-19T06:43:29Z

sdks/python/apache_beam/coders/coder_impl.py

@@ -30,8 +30,6 @@

 from types import NoneType

-import six
-
 from apache_beam.coders import observable


robertwb wrote:
Or perhaps just remove this, as the comment below is sufficient.

Done.

charlesccychen · 2018-03-19T06:43:29Z

sdks/python/apache_beam/coders/coders.py

+  def __hash__(self):
+    return hash(type(self))
+
+


robertwb wrote:
Rather than introducing a PaneInfoCoder, just instantiate the PaneInfoCoderImpl in WindowedValueCoderImpl's constructor. (There's a TODO to remove the parameterizablility of the TimestampCoder as well.) Actually, we could consider just making {en,de}code_pane_info methods on WindowedValueCoder itself.

I am going to get rid of PaneInfoCoder but keep the PaneInfoCoderImpl, because it's more natural to keep it separate, as it would clutter the WindowedValueCoderImpl with encode/decode_paneinfo_to_stream and estimate_paneinfo_size.

charlesccychen · 2018-03-19T06:43:29Z

sdks/python/apache_beam/coders/coder_impl.py

robertwb wrote:
You can now remove this TODO.

Done.

charlesccychen · 2018-03-19T06:43:29Z

sdks/python/apache_beam/utils/windowed_value.py

@@ -104,11 +211,12 @@ def __reduce__(self):



robertwb wrote:
Fix __reduce__ to rememeber the PaneInfo (and test).

Done.

charlesccychen · 2018-03-19T06:43:30Z

sdks/python/apache_beam/utils/windowed_value.py

@@ -104,11 +211,12 @@ def __reduce__(self):


 # TODO(robertwb): Move this to a static method.
-def create(value, timestamp_micros, windows):
+def create(value, timestamp_micros, windows, pane_info=None):


robertwb wrote:
Just let the default value be PANE_INFO_UNKNOWN.

Done.

charlesccychen · 2018-03-19T06:43:30Z

sdks/python/apache_beam/utils/windowed_value.py

+
+
+def _construct_pane_info_map():
+  result = {}


robertwb wrote:
Make this a list, not a map.

Done.

robertwb

LGTM, thanks. Just to be sure, could you run the benchmarks at #4741 ?

charlesccychen · 2018-03-19T20:50:39Z

Thanks. The benchmarks seem very comparable.

Without this change (at HEAD^):

[(1, 1.0581729412078857)]
[(1, 1.0581729412078857), (1001, 1.0961029529571533)]
[(1, 1.0581729412078857), (1001, 1.0961029529571533), (2001, 1.304905891418457)]
[(1, 1.0581729412078857), (1001, 1.0961029529571533), (2001, 1.304905891418457), (3001, 1.3804380893707275)]
[(1, 1.0581729412078857), (1001, 1.0961029529571533), (2001, 1.304905891418457), (3001, 1.3804380893707275), (4001, 1.6390268802642822)]
Fixed cost  1.00637614384
Per-element 1.44604301453e-06
R^2         0.94447668637

With this change:

[(1, 1.0614569187164307)]
[(1, 1.0614569187164307), (1001, 1.1248741149902344)]
[(1, 1.0614569187164307), (1001, 1.1248741149902344), (2001, 1.315816879272461)]
[(1, 1.0614569187164307), (1001, 1.1248741149902344), (2001, 1.315816879272461), (3001, 1.3426880836486816)]
[(1, 1.0614569187164307), (1001, 1.1248741149902344), (2001, 1.315816879272461), (3001, 1.3426880836486816), (4001, 1.6559441089630127)]
Fixed cost  1.01865767245
Per-element 1.40678834915e-06
R^2         0.914786773244

robertwb · 2018-03-19T20:57:28Z

Totally within the margin of error. Thanks.

charlesccychen commented Feb 27, 2018

View reviewed changes

robertwb reviewed Mar 8, 2018

View reviewed changes

charlesccychen force-pushed the paneinfo-part1 branch 4 times, most recently from 077b2dd to 79e421b Compare March 19, 2018 06:36

Add support for PaneInfo in WindowedValues

95956ae

This change allows triggering information to be stored along with WindowedValues.

charlesccychen force-pushed the paneinfo-part1 branch from 79e421b to 95956ae Compare March 19, 2018 06:42

charlesccychen commented Mar 19, 2018

View reviewed changes

robertwb approved these changes Mar 19, 2018

View reviewed changes

robertwb merged commit 8139899 into apache:master Mar 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BEAM-3759] Add support for PaneInfo in WindowedValues #4763

[BEAM-3759] Add support for PaneInfo in WindowedValues #4763

charlesccychen commented Feb 27, 2018

charlesccychen commented Feb 27, 2018

charlesccychen Feb 27, 2018

robertwb Mar 8, 2018

robertwb Mar 8, 2018

robertwb Mar 8, 2018

robertwb Mar 8, 2018

robertwb Mar 8, 2018

robertwb Mar 8, 2018 •

edited

robertwb Mar 8, 2018

robertwb Mar 8, 2018

robertwb Mar 8, 2018

aaltay commented Mar 17, 2018

charlesccychen left a comment

charlesccychen Mar 19, 2018

charlesccychen Mar 19, 2018

charlesccychen Mar 19, 2018

charlesccychen Mar 19, 2018

charlesccychen Mar 19, 2018

charlesccychen Mar 19, 2018

charlesccychen Mar 19, 2018

robertwb left a comment

charlesccychen commented Mar 19, 2018

robertwb commented Mar 19, 2018

		@@ -734,13 +814,14 @@ def decode_from_stream(self, in_stream, nested):
		# Read PaneInfo encoded byte.
		# TODO(BEAM-1522): Ignored for now but should be converted to pane info once

[BEAM-3759] Add support for PaneInfo in WindowedValues #4763

[BEAM-3759] Add support for PaneInfo in WindowedValues #4763

Conversation

charlesccychen commented Feb 27, 2018

charlesccychen commented Feb 27, 2018

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

robertwb Mar 8, 2018 • edited

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

aaltay commented Mar 17, 2018

charlesccychen 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

robertwb left a comment

Choose a reason for hiding this comment

charlesccychen commented Mar 19, 2018

robertwb commented Mar 19, 2018

robertwb Mar 8, 2018 •

edited