Repository navigation
[Python] Fix rebatching into multi-column pyarrow Tables (#41042) - #41045
Conversation
When a batched DoFn's input must be rebatched from another batch type, GeneralPurposeConsumerSet.receive_batch passed a generator to BatchConverter.produce_batch, which is declared to take a sequence. The pyarrow Table converter reads its input once per column, so every column after the first was empty and the bundle failed with ArrowInvalid. Fixes apache#41042.
There was a problem hiding this comment.
📋 Review Summary
This PR fixes a bug in GeneralPurposeConsumerSet.receive_batch where the iterator returned by explode_batch was passed directly to BatchConverter.produce_batch without being materialized into a sequence. Because PyarrowBatchConverter.produce_batch iterates over its input once per column, passing a generator caused subsequent columns to be empty and fail with ArrowInvalid. The fix and accompanying regression test are clean, minimal, and well-targeted.
🔍 General Feedback
- Materializing
explode_batch(...)into alistinGeneralPurposeConsumerSet.receive_batchproperly satisfies theSequence[E]contract ofBatchConverter.produce_batch. - Consider hoisting the batch conversion outside the inner
for consumer in consumers:loop if multiple consumers share the sameconsumer_batch_converter(or even hoistingexplode_batchif there are multipleother_batch_consumersentries), though this is a minor optimization on an already-warned fallback path.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
|
Assigning reviewers: R: @damccorm 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). |
damccorm
left a comment
There was a problem hiding this comment.
Thanks, nice find! I'll wait for tests to run, but will merge if they pass
| consumer_batch_converter.produce_batch( | ||
| self.producer_batch_converter.explode_batch( | ||
| windowed_batch.values)))) | ||
| list( |
There was a problem hiding this comment.
I was nervous about this until I did more digging. For future readers, I think that materializing list(explode_batch(...)) is safe from a memory standpoint because:
windowed_batch.valuesis already a single in-memory batch, andproduce_batchmust return a single materialized in-memory batch (pa.Table,pd.DataFrame,np.ndarray, etc.) for the downstream consumer anyway. So we're going to do this sort of materialization immediately anyways.- As mentioned in the description,
BatchConverter.produce_batch(elements: Sequence[E])expects aSequence, which all other call sites already pass. Passing a list here also avoids issues when rebatching into converters likeNumpyBatchConverterorPytorchBatchConverterthat reject generators. So I think it is generically required, and we can't just push this into a consumer.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #41045 +/- ##
============================================
- Coverage 57.44% 57.44% -0.01%
Complexity 2492 2492
============================================
Files 1203 1203
Lines 171109 171109
Branches 2103 2103
============================================
- Hits 98294 98287 -7
- Misses 70161 70168 +7
Partials 2654 2654
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:
|
When a batched DoFn's input must be rebatched from another batch type, GeneralPurposeConsumerSet.receive_batch passed a generator to BatchConverter.produce_batch, which is declared to take a sequence. The pyarrow Table converter reads its input once per column, so every column after the first was empty and the bundle failed with ArrowInvalid.
Fixes #41042.