-
Notifications
You must be signed in to change notification settings - Fork 4.6k
[Python] Speed up ApproximateUnique accumulator merging #39921
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -262,11 +262,14 @@ def add_input(self, accumulator, element, *args, **kwargs): | |
| except Exception as e: | ||
| raise RuntimeError("Runtime exception: %s" % e) | ||
|
|
||
| # created an issue https://github.com/apache/beam/issues/19459 to speed up | ||
| # merge process. | ||
| def merge_accumulators(self, accumulators, *args, **kwargs): | ||
| merged_accumulator = self.create_accumulator() | ||
| for accumulator in accumulators: | ||
| accumulator_iter = iter(accumulators) | ||
| try: | ||
| merged_accumulator = next(accumulator_iter) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mentioned this below, but it would likely be more efficient to find the largest accumulator and use that as the starting point. Ideally, we'd look for the accumulator with the largest _sample_heap size. If 2 are tied, then we'd look for the one with the larger _min_hash |
||
| except StopIteration: | ||
| return self.create_accumulator() | ||
|
|
||
| for accumulator in accumulator_iter: | ||
| for i in accumulator._sample_heap: | ||
| merged_accumulator.add(i) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -219,6 +219,40 @@ def test_approximate_unique_combine_fn_requires_compatible_coder(self): | |
|
|
||
| self.assertRegex(e.exception.args[0], 'Runtime exception') | ||
|
|
||
| def test_approximate_unique_merge_accumulators_reuses_first(self): | ||
| sample_size = 16 | ||
| combine_fn = ApproximateUniqueCombineFn(sample_size, coders.VarIntCoder()) | ||
| accumulators = [combine_fn.create_accumulator() for _ in range(3)] | ||
| for accumulator, values in zip( | ||
| accumulators, [range(16), range(8, 24), range(24, 40)]): | ||
| for value in values: | ||
| accumulator.add(value) | ||
|
|
||
| later_accumulator_states = [( | ||
| list(accumulator._sample_heap), | ||
| set(accumulator._sample_set), | ||
| accumulator._min_hash) for accumulator in accumulators[1:]] | ||
|
|
||
| merged_accumulator = combine_fn.merge_accumulators(iter(accumulators)) | ||
|
|
||
| self.assertIs(merged_accumulator, accumulators[0]) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is testing our specific implementation, not correctness. For example, it would be equally valid (and maybe better) to use the largest accumulator available as our starting point. Let's update this test to just test correctness instead of the specific behavior we've baked in. |
||
| self.assertEqual(set(range(24, 40)), merged_accumulator._sample_set) | ||
| self.assertEqual(24, merged_accumulator._min_hash) | ||
|
Comment on lines
+239
to
+240
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Both of these asserts would succeed if the only accumulator merged was the last one. Can we update to avoid this? An easy way to do so would be to make the sample size 30 (and update the asserts) |
||
| self.assertEqual( | ||
| later_accumulator_states, | ||
| [( | ||
| list(accumulator._sample_heap), | ||
| set(accumulator._sample_set), | ||
| accumulator._min_hash) for accumulator in accumulators[1:]]) | ||
|
Comment on lines
+241
to
+246
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is this assert checking? Isn't this just running against our test data that we constructed? |
||
|
|
||
| def test_approximate_unique_merge_accumulators_empty(self): | ||
| combine_fn = ApproximateUniqueCombineFn(16, coders.VarIntCoder()) | ||
|
|
||
| merged_accumulator = combine_fn.merge_accumulators(iter(())) | ||
|
|
||
| self.assertEqual([], merged_accumulator._sample_heap) | ||
| self.assertEqual(set(), merged_accumulator._sample_set) | ||
|
|
||
| def test_get_sample_size_from_est_error(self): | ||
| # test if get correct sample size from input error. | ||
| assert beam.ApproximateUnique._get_sample_size_from_est_error(0.5) == 16 | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this is an improvement, I'll note that it does not actually address the core issue in #19459
This PR avoids creating an extra accumulator and doing one additional merge operation, but it doesn't handle the efficient merging of 2 accumulators.
With that said, I don't think a fast merge is possible here because of the uniqueness constraint, so we can still probably call it fixed