andygrove commented on PR #5310: URL: https://github.com/apache/datafusion-comet/pull/5310#issuecomment-5441673607
> **Note on this review:** this was generated by an LLM (Claude Code) at my request while I worked through a review backlog. I have not verified the individual findings myself. Please treat everything below as suggestions to evaluate rather than as authoritative review feedback, and push back on anything that is wrong or already handled. This is worth doing. `from_ffi` building string arrays through `new_unchecked` and then arrow-rs handing out `&str` via `from_utf8_unchecked` really is undefined behavior in the default configuration, and closing it at the import boundary is the right place. The zero-copy `Arc::ptr_eq` contract through the nested arms is nicely done, and decoding before `copy_or_unpack_array` so a dictionary decodes its compact values rather than the expanded ones is a good catch. Three things. **`aligned_stream_reader.rs` is a fourth import site and is not covered** `batch_from_ffi` in `native/core/src/execution/operators/aligned_stream_reader.rs` calls `from_ffi_and_data_type`, and its own doc comment says the function "returns the producer's buffers untouched (via `new_unchecked`)". If that stream ever carries a JVM-produced string column, it has exactly the hole this PR exists to close, and it would be the one place left after this merges. Is that path reachable with string data? If it is, it needs `decode_string_arrays` too. If it is not, the description should list all four `from_ffi` sites and say why that one is exempt, so the next person auditing this does not have to rediscover it. **The default arm silently passes through** ```rust _ => Ok(Arc::clone(array)), ``` The comment is honest about the risk and says a view-typed column would silently return the UB. But a comment is not a mechanism. In a function whose entire purpose is to prevent unsoundness, an unknown type should not take the quiet path. Could the default arm return an error for the types that could plausibly contain strings, specifically `Utf8View`, `ListView`, `LargeListView`, and `RunEndEncoded`, and keep the silent clone only for types that provably cannot? A hard error on an unexpected string-bearing type turns a latent memory-safety bug into a clear failure, and if `Utf8View` ever does arrive over FFI, that is much better than what the comment describes. **The performance cost needs an end-to-end number** Every string column crossing the JVM to native boundary now pays a full UTF-8 validation pass per batch. The benchmark measures `decode_string_arrays` in isolation on an 8192-row valid batch, which tells us the function is fast but not what it costs a query. Could you add a before-and-after on something string-heavy end to end, say TPC-H Q1 or a filter over a wide string column with `spark.comet.scan.impl` set to the JVM path? This is a soundness fix so it should land regardless of the number, but users should be told what it costs, and if it turns out to be material there may be a case for validating once per column rather than once per batch, or for trusting a producer that guarantees validity. **One smaller note** `scans.md` is updated, which is right. Does it now say clearly which import boundaries are covered and that Gap A (the native scan) is still open under #4764? A user reading that page should be able to tell whether their configuration is affected. -- 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]
