viirya commented on code in PR #7234:
URL: https://github.com/apache/datafusion-comet/pull/7234#discussion_r4391500111
##########
native/shuffle/src/writers/buf_batch_writer.rs:
##########
@@ -118,7 +154,15 @@ impl<S: Borrow<ShuffleBlockWriter>, W: Write>
BufBatchWriter<S, W> {
write_time: &Time,
) -> datafusion::common::Result<usize> {
self.check_scratch(scratch);
- let batch_size = self.batch_size;
+ let Some(batch_size) = self.coalesce_batch_size else {
+ return self.write_batch_to_buffer(
+ batch,
+ scratch,
+ codec_context,
+ encode_time,
+ write_time,
+ );
+ };
let coalescer = self.coalescer.get_or_insert_with(|| {
// Enable BatchCoalescer's zero-copy passthrough for batches that
are already big
Review Comment:
This comment went back to main's wording in c5ce2b151, but it no longer
matches who reaches this code. It explains the `batch_size - 1` limit with the
full chunks `PartitionedBatchIterator` emits, which "removes a full copy of the
shuffle payload". After this PR, every multi-partition writer goes through
`new_for_partition`, so `PartitionedBatchIterator` output never gets here. The
only coalescing writer left is the single-partition one, and its batches stream
straight in from `SinglePartitionShufflePartitioner`. Could the comment give
the reason in terms of that caller? Something like: input batches that are
already at least `batch_size` rows skip the builders and are written as one
block. Otherwise a reader may think the multi-partition path still coalesces.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
