Repository navigation
Conversation
Indexing the result of a Partition applied to a list with an int, such as partitions[0], raised KeyError, because _MaterializedDoOutputsTuple.__getitem__ did not convert int tags to strings the way DoOutputsTuple.__getitem__ does for pipelines. Fixes apache#19154
There was a problem hiding this comment.
📋 Review Summary
This PR resolves #19154 by converting integer tags to strings in _MaterializedDoOutputsTuple.__getitem__, matching the existing behavior of DoOutputsTuple.__getitem__. The implementation is clean, minimal, and properly covered by a unit test.
🔍 General Feedback
- The change accurately aligns eager materialization output indexing with deferred pipeline
DoOutputsTupleindexing. - The unit test in
ptransform_test.pyand theCHANGES.mdentry are well-structured and complete.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #40681 +/- ##
=============================================
+ Coverage 56.16% 80.01% +23.84%
=============================================
Files 1124 490 -634
Lines 178336 82813 -95523
Branches 1489 0 -1489
=============================================
- Hits 100170 66261 -33909
+ Misses 75645 16552 -59093
+ 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:
|
|
Assigning reviewers: R: @claudevdm for label python. This pull request likely touches a core component ("core" label). Please review with scrutiny. 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). |
Fixes #19154
When
beam.Partitionis applied to a list instead of a PCollection, the result can't be indexed with an int:Inside a pipeline
partitions[0]works, becauseDoOutputsTuple.__getitem__converts int tags to strings so that partitions can be looked up with the same ints the partition function returns. The result of applying a transform to a list is a_MaterializedDoOutputsTuple, whose__getitem__overrides that method without the conversion, so onlypartitions['0']works there.This change does the same int to string conversion in
_MaterializedDoOutputsTuple.__getitem__.I added
test_partition_applied_to_listtoptransform_test.py. It fails with theKeyErrorabove before the change and passes after it.Testing:
pytest apache_beam/transforms/ptransform_test.py apache_beam/pvalue_test.py apache_beam/pipeline_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.