andygrove opened a new issue, #6291: URL: https://github.com/apache/datafusion-comet/issues/6291
### 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](https://github.com/apache/arrow-rs/issues/10028)). The fix, [apache/arrow-rs#10030](https://github.com/apache/arrow-rs/pull/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: 1. `NativeUtil.getNextBatch` allocates one `ArrowArray`/`ArrowSchema` pair per column. 2. `Native.executePlan(..., arrayAddrs, schemaAddrs)` runs the plan, and `prepare_output` fills the pairs through `move_to_spark`. 3. 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 `ArrowReader`s: `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. -- 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]
