Repository navigation
Conversation
When every SourceBundle passed to a ConcatSource had weight 0, the average used to fill in missing weights was also 0, so computing the cumulative weights divided by zero. Treat zero weights like unknown weights when computing that average. Fixes apache#21316
|
Assigning reviewers: R: @jrmccluskey for label python. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
Thanks, this was an old issue for an edge case. To understand if it's still relavant, is there any example pipeline that could trigger this issue? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #40549 +/- ##
=============================================
+ Coverage 56.16% 79.92% +23.75%
=============================================
Files 1124 487 -637
Lines 178336 82297 -96039
Branches 1489 0 -1489
=============================================
- Hits 100170 65776 -34394
+ Misses 75645 16521 -59124
+ Partials 2521 0 -2521
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jrmccluskey
left a comment
There was a problem hiding this comment.
+1 to Yi's request for a repro pipeline as a regression test.
|
Thanks for taking a look. Built-in file sources don't hit this anymore (empty files are skipped and their bundles have no weights), but reading any source = ConcatSource([
iobase.SourceBundle(0, RangeSource(0, 4), None, None),
iobase.SourceBundle(0, RangeSource(4, 8), None, None),
])
with TestPipeline() as p:
assert_that(p | beam.io.Read(source), equal_to(list(range(8))))On master this raises I replaced the unit test with this pipeline as the regression test. It fails without the change and passes with it. |
Fixes #21316
ConcatRangeTracker._compute_cumulative_weightsfills in missing bundle weights with the average of the known weights. A weight of 0 is already treated as missing (s.weight or avg), but zeros were still counted when computing the average. If every bundle has weight 0, the average is 0, the total is 0, and this line raisesZeroDivisionError: float division by zero:This happens when a source's
split()returns bundles with weight 0, for example when the weights are sizes rounded down to whole MB for bundles smaller than 1 MB, which was the case in the issue. #12142 fixed the MongoDB source on its own, butConcatSourcestill fails for any source that does this.This change leaves zero weights out of the average, in the same way as
Noneweights. When every weight is 0 or missing, the tracker falls back to equal weights of 1.0, which it already does when no weights are given.I added
test_zero_weight_bundlestoconcat_source_test.py. It raisesZeroDivisionErrorbefore the change and passes after it.Testing:
pytest apache_beam/io/concat_source_test.py apache_beam/io/iobase_test.py apache_beam/io/filebasedsource_test.py apache_beam/io/textio_test.pyyapf --diffandruff checkon the changed filesThank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123orfixes #456).CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.