[BEAM-8575] Added two unit tests to CombineTest class to test that Co… #10190

bumblebee-coming · 2019-11-21T19:07:29Z

…mbinePerKey works with a simple customized CombineFn.

Added two unit tests to CombineTest class to test that CombinePerKey works with a simple customized CombineFn, namely ConcatIntCombineFn.

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.
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.

Post-Commit Tests Status (on master branch)

Lang	SDK	Apex	Dataflow	Gearpump	Samza	Spark
Go		---	---	---	---
Java
Python		---		---	---
XLang	---	---	---	---	---	---

Pre-Commit Tests Status (on master branch)

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

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

bumblebee-coming · 2019-11-21T19:17:26Z

Its Java parity is the
testSimpleCombine
testSimpleCombineEmpty
in file
sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/CombineTest.java

link:
https://github.com/apache/beam/blob/master/sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/CombineTest.java#L660

robertwb · 2019-11-21T22:35:06Z

The mean combine fn already covers this test case completely.

bumblebee-coming · 2019-11-22T19:21:08Z

This test (test_ConcatIntCombineFn_combine) is very similar to #10173 (test_MeanCombineFn_combine), except that it is supposed to be used to test counters.

We can either continue to add this test (test_ConcatIntCombineFn_combine) and then write the counter test using this pipeline,
or skip this test and write the counter test using #10173 (test_MeanCombineFn_combine) instead.
Do you think which one is better?

(An email with more details is sent to the reviewer.)

bumblebee-coming · 2019-12-06T02:27:10Z

As discussed in the email, I re-implemented the tests to set three different kinds of metrics (counter, distribution and gauge) in a CombineFn and then extract and verify their values from the result returned by pipeline.run().

bumblebee-coming · 2019-12-06T02:40:14Z

R: @robertwb

bumblebee-coming · 2019-12-06T22:03:21Z

Run PythonLint PreCommit

robertwb

Thanks. Just some minor comments on non-determinism.

sdks/python/apache_beam/transforms/combiners_test.py

bumblebee-coming · 2019-12-14T00:23:57Z

Run Python PreCommit

chamikaramj · 2020-01-09T00:34:03Z

Retest this please

…mbinePerKey works with a simple customized CombineFn.

…meaningful.

bumblebee-coming · 2020-01-09T01:52:40Z

Retest this please

angoenka · 2020-01-09T02:06:32Z

Retest this please

angoenka · 2020-01-09T02:07:28Z

Run Python PreCommit

chamikaramj · 2020-01-09T18:45:38Z

Retest this please

chamikaramj · 2020-01-09T18:45:41Z

Retest this please

angoenka · 2020-01-09T20:41:27Z

Retest this please

angoenka · 2020-01-09T20:41:43Z

Run Python PreCommit

bumblebee-coming · 2020-01-09T21:02:23Z

Retest this please

tvalentyn · 2020-01-09T21:38:15Z

Retest this please

robertwb

Just some minor changes.

robertwb · 2020-01-10T01:19:35Z

sdks/python/apache_beam/transforms/combiners_test.py

+    result.wait_until_finish()
+
+    # Verify the concatenated strings are correct.
+    expected_concat_per_key = [('c', 'ginoru'), ('d', 'abeemsstt')]


Nit: it's really hard to tell from reading this whether it is correct. Maybe make the input something like

('key1': 'a'), ('key1': 'ab'), .., ('key2', 'xyz'), ...

robertwb · 2020-01-10T01:21:28Z

sdks/python/apache_beam/transforms/combiners_test.py

@@ -50,6 +52,39 @@
 from apache_beam.utils.timestamp import Timestamp


+class CounterIncrememtingCombineFn(beam.CombineFn):


Maybe call this SortedConcatWithCounters?

robertwb · 2020-01-10T01:22:24Z

sdks/python/apache_beam/transforms/combiners_test.py

+    self.last_word_len.set(len(element))
+
+    # ''.join() converts the list to a string.
+    return ''.join(sorted(acc + element))


No need to sort here, just return acc + element.

robertwb · 2020-01-10T01:22:44Z

sdks/python/apache_beam/transforms/combiners_test.py

+    return ''.join(sorted(acc + element))
+
+  def merge_accumulators(self, accs):
+    return ''.join(sorted(''.join(accs)))


Likewise, return sum(accs, '')

sum(accs, ' ') doesn't work, because sum couldn't concatenate strings.

robertwb · 2020-01-10T01:23:05Z

sdks/python/apache_beam/transforms/combiners_test.py

+    return ''.join(sorted(''.join(accs)))
+
+  def extract_output(self, acc):
+    return acc


I'd do the sorting here.

Once sorted, acc became a list again. So I use ''.join() to convert it back to a string.

robertwb · 2020-01-10T01:23:35Z

sdks/python/apache_beam/transforms/combiners_test.py

+    global_concat = (input
+                     | beam.Values()
+                     | beam.CombineGlobally(CounterIncrememtingCombineFn())
+                     | "sort global result" >> _SortLists)


There's no need for _SortLists anywhere here.

angoenka · 2020-01-10T03:36:32Z

Retest this please

… changes accordingly.

chamikaramj · 2020-01-10T19:10:15Z

Retest this please

chamikaramj · 2020-01-10T19:10:22Z

Retest this please

tvalentyn · 2020-01-10T20:44:09Z

retest this please

robertwb reviewed Dec 9, 2019

View reviewed changes

bumblebee-coming force-pushed the simple_combine branch from f2a93a6 to 04d8a44 Compare December 18, 2019 01:51

bumblebee-coming added 13 commits January 8, 2020 17:51

[BEAM-8575] Added two unit tests to CombineTest class to test that Co…

ee6aded

…mbinePerKey works with a simple customized CombineFn.

Reimplemented the tests to verify customized counters.

0c5fe0c

fixup

f849394

fixup.

ff3f7b5

Reimplemented the tests to verify customized counters.

1a4b31f

fixup

cbd8de9

Fixed Lint errors.

e28d19d

Fixed Lint errors.

3258d5b

Modified code according to reviewer's feedback.

d58116d

Fixed gauges result for test_simple_combine_empty.

54267ac

fixup

36358e2

Added import lines.

40d7684

Fixed the order of assert_that and changed the test names to be more …

d37021a

…meaningful.

bumblebee-coming force-pushed the simple_combine branch from 4a1cf4b to d37021a Compare January 9, 2020 01:51

Fixed lint errors.

620d4df

Fixed lint errors.

0aabb9a

robertwb reviewed Jan 10, 2020

View reviewed changes

Modified the code according to the reviewer's feedback.

c8f3205

Comfirmed the type of acc and element is string, not a list, and made…

b2cc7c8

… changes accordingly.

robertwb approved these changes Jan 10, 2020

View reviewed changes

robertwb merged commit d64759d into apache:master Jan 10, 2020

		@@ -50,6 +52,39 @@
		from apache_beam.utils.timestamp import Timestamp


		class CounterIncrememtingCombineFn(beam.CombineFn):

[BEAM-8575] Added two unit tests to CombineTest class to test that Co… #10190

[BEAM-8575] Added two unit tests to CombineTest class to test that Co… #10190

Conversation

bumblebee-coming commented Nov 21, 2019

Added two unit tests to CombineTest class to test that CombinePerKey works with a simple customized CombineFn, namely ConcatIntCombineFn.

Post-Commit Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

bumblebee-coming commented Nov 21, 2019

robertwb commented Nov 21, 2019

bumblebee-coming commented Nov 22, 2019

bumblebee-coming commented Dec 6, 2019 • edited

bumblebee-coming commented Dec 6, 2019

bumblebee-coming commented Dec 6, 2019

robertwb left a comment

Choose a reason for hiding this comment

bumblebee-coming commented Dec 14, 2019

chamikaramj commented Jan 9, 2020

bumblebee-coming commented Jan 9, 2020

angoenka commented Jan 9, 2020

angoenka commented Jan 9, 2020

chamikaramj commented Jan 9, 2020

chamikaramj commented Jan 9, 2020

angoenka commented Jan 9, 2020

angoenka commented Jan 9, 2020

bumblebee-coming commented Jan 9, 2020

tvalentyn commented Jan 9, 2020

robertwb 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

bumblebee-coming Jan 10, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bumblebee-coming Jan 10, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bumblebee-coming Jan 10, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angoenka commented Jan 10, 2020

chamikaramj commented Jan 10, 2020

chamikaramj commented Jan 10, 2020

tvalentyn commented Jan 10, 2020

bumblebee-coming commented Dec 6, 2019 •

edited

bumblebee-coming Jan 10, 2020 •

edited

bumblebee-coming Jan 10, 2020 •

edited

bumblebee-coming Jan 10, 2020 •

edited