andygrove commented on code in PR #7234:
URL: https://github.com/apache/datafusion-comet/pull/7234#discussion_r4389135808
##########
native/shuffle/src/writers/buf_batch_writer.rs:
##########
@@ -46,32 +56,68 @@ pub(crate) struct BufBatchWriter<S:
Borrow<ShuffleBlockWriter>, W: Write> {
writer: W,
buffer_max_size: usize,
/// Coalesces small batches into target_batch_size before serialization.
- /// Lazily initialized on first write to capture the schema.
+ /// Lazily initialized on first write to capture the schema. Never set in
passthrough mode.
coalescer: Option<BatchCoalescer>,
- /// Target batch size for coalescing
- batch_size: usize,
+ /// Target batch size for coalescing; `None` selects passthrough mode.
+ coalesce_batch_size: Option<usize>,
/// Address of the scratch `Vec` seen on first use; every later call must
pass the same
/// one, or unflushed bytes in the other buffer would be silently
abandoned.
#[cfg(debug_assertions)]
scratch_addr: Option<usize>,
- /// Running total of bytes serialized through this writer, used to report
spilled bytes when
- /// the underlying writer does not implement [`Seek`] (e.g. a `Box<dyn
SpillWriter>`).
+ /// Running total of bytes serialized through this writer, used to report
spilled bytes and
+ /// to track output offsets without a `Seek` on the underlying writer.
total_bytes_written: u64,
}
impl<S: Borrow<ShuffleBlockWriter>, W: Write> BufBatchWriter<S, W> {
+ /// A coalescing writer; see the type-level docs for when to use which
mode.
pub(crate) fn new(
shuffle_block_writer: S,
writer: W,
buffer_max_size: usize,
batch_size: usize,
+ ) -> Self {
+ Self::with_mode(
+ shuffle_block_writer,
+ writer,
+ buffer_max_size,
+ Some(batch_size),
+ )
+ }
+
+ /// A writer for one output partition of a multi-partition shuffle:
passthrough, except for
+ /// schemas with view types, which still need the coalescer to compact
sparse views (see
+ /// [`ShuffleBlockWriter::has_view_types`]).
+ pub(crate) fn new_for_partition(
Review Comment:
Agreed on all three points, thanks for tracing it. Full chunks skip the
coalescer either way, the coalescer only compacts top-level view columns, and
the planner already casts view results back to `Utf8`. I took the simpler
option: `new_for_partition` always passes through, and `has_view_types`, the
coalescing branch and `for_partition_coalesces_only_view_schemas` are gone.
`contains_view_type` is back in `shuffle_writer.rs` as on main, so the
round-robin check is untouched. The `BufBatchWriter` docs now say passthrough
does not compact view arrays and why that is acceptable. Since the
multi-partition writers no longer need a `batch_size`, I removed that plumbing
too (c5ce2b151d).
##########
native/shuffle/src/writers/shuffle_block_writer.rs:
##########
@@ -154,6 +192,17 @@ impl ShuffleBlockWriter {
}
}
+ /// True when the schema contains a view type at any depth; see
[`contains_view_type`].
+ ///
+ /// Arrow's IPC writer serializes every data buffer a view array
references, whatever the
+ /// views actually point at. A batch gathered by `interleave` from many
buffered batches
Review Comment:
That doc went away with `has_view_types` in c5ce2b151d, so the overstated
wording is gone too.
##########
native/shuffle/src/writers/local/spill.rs:
##########
@@ -334,7 +321,8 @@ mod tests {
let block_writer =
ShuffleBlockWriter::try_new(batch.schema_ref().as_ref(),
CompressionCodec::None)
.unwrap();
- // batch_size below the row count so a write serializes into the
scratch
+ // batch_size below the row count so a view-schema write, which
coalesces, serializes into
Review Comment:
Good catch. With passthrough `batch_size` no longer reaches
`PartitionedSpill` at all, so I dropped the parameter and the comment
(c5ce2b151d).
--
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]
