Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@
## New Features / Improvements

* X feature added (Java/Python) ([#X](https://github.com/apache/beam/issues/X)).
* (Python) Reduced the cost of merging `ApproximateUnique` accumulators by avoiding an unnecessary copy and heap rebuild ([#19459](https://github.com/apache/beam/issues/19459)).
* (Java/Python) `Watch` can bound its deduplication state by event time, retiring an output key once the greatest emitted timestamp has moved more than the allowed lateness past it. Java adds `Watch.growthOf(...).withTimestampCursor()`. Python adds `allowed_lateness` for the existing `timestamp_cursor` option ([#18459](https://github.com/apache/beam/issues/18459)).
* (Java) Spark Structured Streaming runner: stateful ParDo with state, timers, `@RequiresTimeSortedInput` and tagged outputs is now supported in batch mode ([#39779](https://github.com/apache/beam/issues/39779)).
* (Python) Added support for Vertex AI Model Monitoring V2 in RunInference ([#39738](https://github.com/apache/beam/issues/39738)).
Expand Down
11 changes: 7 additions & 4 deletions sdks/python/apache_beam/transforms/stats.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Copy link
Copy Markdown
Contributor

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

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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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)

Expand Down
34 changes: 34 additions & 0 deletions sdks/python/apache_beam/transforms/stats_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -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])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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
Expand Down
Loading