github-actions[bot] commented on code in PR #41062:
URL: https://github.com/apache/beam/pull/41062#discussion_r4498091091
##########
sdks/python/apache_beam/transforms/combiners.py:
##########
@@ -498,21 +501,24 @@ def add_input(self, accumulator, element, *args,
**kwargs):
def merge_accumulators(self, accumulators, *args, **kwargs):
result_heap = None
holds_comparables = None
+ use_comparables = self._compare is not operator.lt or bool(self._key)
+ n = self._n
for accumulator in accumulators:
holds_comparables, heap = accumulator
- if self._compare is not operator.lt or self._key:
+ if use_comparables:
heap = self._hydrated_heap(heap)
holds_comparables = True
else:
assert not holds_comparables
if result_heap is None:
- result_heap = heap
- else:
+ result_heap = list(heap)
+ elif n > 0:
Review Comment:
**🟢 Nit**: If `n <= 0` (e.g. `TopCombineFn(0)`), `add_input` will still push
up to 1 element into `heap` because `len(heap) < self._n` is `False` and `else:
heapq.heappushpop(heap, comparable)` is executed on an empty list (which pushes
the element and pops it, leaving `heap` empty—wait: `heappushpop([], x)`
actually returns `x` and leaves the list empty!). However, if the first
accumulator in `merge_accumulators` somehow had elements or if `n <= 0` is
guarded in `add_input` as well (like in `SampleCombineFn.add_input`), guarding
`if self._n <= 0: return accumulator` in `TopCombineFn.add_input` would make
`TopCombineFn(0)` consistent and avoid unnecessary `ComparableValue`
allocations.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
