You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Comet currently has several Spark-to-Arrow conversion paths:
RowArrowReader
SparkColumnarArrowReader
CometArrowConverters
ArrowWriter / ArrowFieldWriter
These paths have different ownership and lifecycle requirements, so the readers and converters should remain separate. However, they share the same lower-level responsibility: writing Spark rows or ColumnVector slices into Arrow vectors.
PR #5051 introduced CometArrowConverters.writeColumns to share the column-copy loop between SparkColumnarArrowReader and the cache conversion path. This removes duplication, but it also places low-level Arrow encoding inside a higher-level converter that is otherwise responsible for allocating independently owned batches.
The current structure also converts each source column into a ColumnarArray before dispatching to ArrowWriter. This hides the original ColumnVector, slice offset, and length from the field writer, making it difficult to implement specialized bulk-copy paths such as #5299.
Row count is also currently derived indirectly from column writes. This previously caused zero-column batches with numRows > 0 to be emitted with zero rows, as fixed in #4795.
Proposed change
Centralize Spark-to-Arrow value encoding in ArrowWriter and ArrowFieldWriter, while keeping batching, ownership, and transport logic in the existing readers and converters.
Retain access to the original ColumnVector, startRow, and numRows.
Dispatch to an optimized implementation when available.
Fall back to the existing ColumnarArray plus writeCol / writeColNoNull behavior.
Keep capacity checks and non-resizing writes coupled inside the writer implementation.
finish(rowCount) should set the logical row count explicitly instead of deriving it from the last column written. This naturally handles zero-column batches.
After this change:
SparkColumnarArrowReader remains responsible for selecting input batches and slices.
RowArrowReader remains responsible for consuming and batching InternalRows.
CometArrowConverters remains responsible for allocating fresh roots, transferring ownership, and cleaning up on failure.
ColumnarBatchArrowReader remains the separate Arrow-backed retain/transfer path.
ArrowWriter becomes the single implementation point for Spark-value-to-Arrow-buffer encoding.
Why is this useful?
This should:
remove the remaining duplicated column-writing behavior;
make the distinction between encoding and ownership clearer;
centralize row-count and capacity invariants;
make the conversion code easier to review and maintain;
What problem does this solve?
Comet currently has several Spark-to-Arrow conversion paths:
RowArrowReaderSparkColumnarArrowReaderCometArrowConvertersArrowWriter/ArrowFieldWriterThese paths have different ownership and lifecycle requirements, so the readers and converters should remain separate. However, they share the same lower-level responsibility: writing Spark rows or
ColumnVectorslices into Arrow vectors.PR #5051 introduced
CometArrowConverters.writeColumnsto share the column-copy loop betweenSparkColumnarArrowReaderand the cache conversion path. This removes duplication, but it also places low-level Arrow encoding inside a higher-level converter that is otherwise responsible for allocating independently owned batches.The current structure also converts each source column into a
ColumnarArraybefore dispatching toArrowWriter. This hides the originalColumnVector, slice offset, and length from the field writer, making it difficult to implement specialized bulk-copy paths such as #5299.Row count is also currently derived indirectly from column writes. This previously caused zero-column batches with
numRows > 0to be emitted with zero rows, as fixed in #4795.Proposed change
Centralize Spark-to-Arrow value encoding in
ArrowWriterandArrowFieldWriter, while keeping batching, ownership, and transport logic in the existing readers and converters.A possible API shape is:
writeColumnSliceshould:ColumnVector,startRow, andnumRows.ColumnarArraypluswriteCol/writeColNoNullbehavior.finish(rowCount)should set the logical row count explicitly instead of deriving it from the last column written. This naturally handles zero-column batches.After this change:
SparkColumnarArrowReaderremains responsible for selecting input batches and slices.RowArrowReaderremains responsible for consuming and batchingInternalRows.CometArrowConvertersremains responsible for allocating fresh roots, transferring ownership, and cleaning up on failure.ColumnarBatchArrowReaderremains the separate Arrow-backed retain/transfer path.ArrowWriterbecomes the single implementation point for Spark-value-to-Arrow-buffer encoding.Why is this useful?
This should:
Non-goals
This issue should not:
ArrowReaderimplementations;CometVectorbatches;Acceptance criteria
SparkColumnarArrowReaderand fresh-batch conversion use one shared writer API.CometArrowConverters.ColumnVector, slice offset, and row count before falling back toColumnarArray.Related work