What is the problem the feature request solves?
Three leftovers on the JVM/native Arrow boundary are now dead code or wrong documentation.
AlignedArrowStreamReader no longer does anything the stock reader doesn't. It exists because arrow 58's from_ffi_and_data_type passed JVM-allocated Decimal128 buffers through unaligned (apache/arrow-rs#10028). The fix, apache/arrow-rs#10030, shipped in 59.0.0. Comet has been on 59.x since #5262 and is on 59.3.0 now, where from_ffi and from_ffi_and_data_type call align_buffers themselves (arrow-array-59.3.0/src/ffi.rs, lines 296 and 321). So batch_from_ffi's own align_buffers call is a second, redundant pass over every buffer of every input batch. The code comment in aligned_stream_reader.rs, the "Buffer Alignment" section of docs/source/contributor-guide/ffi.md, and item 4 of the review-comet-ffi-pr skill all say the reader can be replaced once Comet is on arrow 59 or newer.
The Native → JVM half of ffi.md documents an API that has never existed. The "FFI Transfer Process" samples call Native.getNextBatch(nativeHandle) and a per-column Native.exportVector(batchHandle, i, ...). The lifecycle table and the "Release Callbacks" sample describe a batch handle and a hand-written release_batch. All of it has been there since the page was added in #2668. The actual flow is:
NativeUtil.getNextBatch allocates one ArrowArray/ArrowSchema pair per column.
Native.executePlan(..., arrayAddrs, schemaAddrs) runs the plan, and prepare_output fills the pairs through move_to_spark.
- The JVM imports each column with
ArrowImporter.importVector (one shared SchemaImporter, so dictionary ids do not collide) and wraps it with CometVector.getVector.
That section should also record the offset normalization that prepare_output does for #2051, because Arrow Java ignores ArrowArray.offset, and the nested cases it misses (#6288).
ScanExec's dictionary unpack is unreachable for real inputs. import_column unpacks a Dictionary column and then deep-copies the result. The comment justifies the copy with the unpack kernel possibly reusing the input's null buffer. That mattered when a JVM producer could reuse its buffers across batches (the old arrow_ffi_safe flag). The C Stream input path in #4572 removed that flag, and it already hands every non-dictionary column to native with no copy. And no input stream carries a dictionary any more. There are three ArrowReaders: RowArrowReader and SparkColumnarArrowReader write plain vectors, and ColumnarBatchArrowReader decodes dictionaries on the JVM before export, which is also why reconcileStreamSchema advertises the value type. The "copy only to unpack dictionaries" row in ffi.md's ownership table is stale for the same reason.
Describe the potential solution
- Replace
AlignedArrowStreamReader with arrow::ffi_stream::ArrowArrayStreamReader. Keep the realigns_under_aligned_decimal128 test, pointed at the stock reader, as a guard against an arrow downgrade.
- Rewrite the Native → JVM section of
ffi.md from the current code, and drop the Buffer Alignment section.
- Drop the dictionary branch's extra
copy_array in copy_or_unpack_array, or the branch itself if the unit tests that seed dictionary input through set_input_batch can go too. ScanExec is its only caller, and CopyMode::UnpackOrDeepCopy has no production caller at all.
- Update
.ai/skills/review-comet-ffi-pr/SKILL.md to match.
NativeUtil.takeRows has no callers and could go in the same change.
Additional context
Found during an audit of the FFI paths. No behavior change is intended.
What is the problem the feature request solves?
Three leftovers on the JVM/native Arrow boundary are now dead code or wrong documentation.
AlignedArrowStreamReaderno longer does anything the stock reader doesn't. It exists because arrow 58'sfrom_ffi_and_data_typepassed JVM-allocatedDecimal128buffers through unaligned (apache/arrow-rs#10028). The fix, apache/arrow-rs#10030, shipped in 59.0.0. Comet has been on 59.x since #5262 and is on 59.3.0 now, wherefrom_ffiandfrom_ffi_and_data_typecallalign_buffersthemselves (arrow-array-59.3.0/src/ffi.rs, lines 296 and 321). Sobatch_from_ffi's ownalign_bufferscall is a second, redundant pass over every buffer of every input batch. The code comment inaligned_stream_reader.rs, the "Buffer Alignment" section ofdocs/source/contributor-guide/ffi.md, and item 4 of thereview-comet-ffi-prskill all say the reader can be replaced once Comet is on arrow 59 or newer.The Native → JVM half of
ffi.mddocuments an API that has never existed. The "FFI Transfer Process" samples callNative.getNextBatch(nativeHandle)and a per-columnNative.exportVector(batchHandle, i, ...). The lifecycle table and the "Release Callbacks" sample describe a batch handle and a hand-writtenrelease_batch. All of it has been there since the page was added in #2668. The actual flow is:NativeUtil.getNextBatchallocates oneArrowArray/ArrowSchemapair per column.Native.executePlan(..., arrayAddrs, schemaAddrs)runs the plan, andprepare_outputfills the pairs throughmove_to_spark.ArrowImporter.importVector(one sharedSchemaImporter, so dictionary ids do not collide) and wraps it withCometVector.getVector.That section should also record the offset normalization that
prepare_outputdoes for #2051, because Arrow Java ignoresArrowArray.offset, and the nested cases it misses (#6288).ScanExec's dictionary unpack is unreachable for real inputs.
import_columnunpacks aDictionarycolumn and then deep-copies the result. The comment justifies the copy with the unpack kernel possibly reusing the input's null buffer. That mattered when a JVM producer could reuse its buffers across batches (the oldarrow_ffi_safeflag). The C Stream input path in #4572 removed that flag, and it already hands every non-dictionary column to native with no copy. And no input stream carries a dictionary any more. There are threeArrowReaders:RowArrowReaderandSparkColumnarArrowReaderwrite plain vectors, andColumnarBatchArrowReaderdecodes dictionaries on the JVM before export, which is also whyreconcileStreamSchemaadvertises the value type. The "copy only to unpack dictionaries" row inffi.md's ownership table is stale for the same reason.Describe the potential solution
AlignedArrowStreamReaderwitharrow::ffi_stream::ArrowArrayStreamReader. Keep therealigns_under_aligned_decimal128test, pointed at the stock reader, as a guard against an arrow downgrade.ffi.mdfrom the current code, and drop the Buffer Alignment section.copy_arrayincopy_or_unpack_array, or the branch itself if the unit tests that seed dictionary input throughset_input_batchcan go too.ScanExecis its only caller, andCopyMode::UnpackOrDeepCopyhas no production caller at all..ai/skills/review-comet-ffi-pr/SKILL.mdto match.NativeUtil.takeRowshas no callers and could go in the same change.Additional context
Found during an audit of the FFI paths. No behavior change is intended.