andygrove commented on issue #4764: URL: https://github.com/apache/datafusion-comet/issues/4764#issuecomment-4905467835
## Investigation notes: Gap B (FFI import UTF-8 validation) Mapping the JVM -> native Arrow FFI import surface for Gap B (imported `Utf8`/`LargeUtf8` arrays are built with arrow's unchecked `from_ffi` / `from_ffi_and_data_type` via `ArrayData::new_unchecked`, so invalid UTF-8 enters native as an Arrow string array that lies about its validity). ### Import sites (the complete set) Three distinct JVM -> native import points, none of which validate UTF-8 today: | Site | Location | Hotness | Carries | |------|----------|---------|---------| | A. ScanExec stream | `batch_from_ffi` -> `from_ffi_and_data_type`, `aligned_stream_reader.rs:95-98` | Hottest: per batch, per column, for all query input | native_comet JVM Parquet reader, Spark columnar handoff, shuffle reads, `mapInArrow` (everything `ScanExec` pulls) | | B. columnarToRow | `from_ffi`, `jni_api.rs:1296` | Per batch/column, only for columnar->row queries | JVM columnar batch -> Spark `UnsafeRow` | | C. JVM UDF result | `from_ffi`, `jvm_udf/mod.rs:239` | Per UDF eval | Result of a JVM-side Spark UDF | ### The existing safeguard does not cover this `prepare_output`'s `validate_full()` (`jni_api.rs:685`) is gated on `COMET_DEBUG_ENABLED`, runs only on the export (native -> JVM) path, and checks structural offsets rather than UTF-8 well-formedness of imported data. It does not touch any import site. ### What a fix needs 1. **Expose the decoder** (shared with Gap A): `decode_utf8_spark_lossy` is `pub(crate)` in the shuffle crate (`native/shuffle/src/spark_unsafe/unsafe_object.rs:40`) and is not reachable from `native/core`. Promote to `pub` or relocate to `datafusion-comet-spark-expr`. 2. **A nested-aware string-decode walker** (new code): nothing in `scan.rs`/`copy.rs` recurses into nested types today (only top-level dictionaries are unpacked). It must handle `Utf8`, `LargeUtf8`, `Dictionary(_, Utf8)`, `List`/`LargeList`/`FixedSizeList`, `Struct`, and `Map`. The fast path is a single `str::from_utf8` SIMD pass over the values buffer with a zero-copy borrow; it rebuilds/allocates only on genuinely invalid bytes. 3. **Apply it at all three boundaries.** For the dominant path A, the natural spot is `copy_or_unpack_array` / `copy_array` (`native/core/src/execution/operators/copy.rs`), already the per-column choke point invoked at `scan.rs:159`. Plain `Utf8`/`LargeUtf8` currently falls through to a bare `Arc::clone` there, so a decode branch has to be inserted (dictionary columns are already rebuilt at that point). ### Two subtleties for the design - **Fast-path soundness:** a whole-buffer `from_utf8` being valid does not by itself guarantee each element's `[offset[i], offset[i+1])` slice is on a char boundary (a malformed producer could split a codepoint), and `value()` slices per offset with an unchecked decode. To stay both fast and sound, the valid path should also confirm offsets do not land on UTF-8 continuation bytes (cheap O(number of strings) check), otherwise fall back to per-element decode. Legitimate Spark producers always satisfy this; the guard is for the adversarial-bytes case that motivates this gap. - **Performance:** this adds a validation pass per string column per batch on the hottest path (A). Consistent with the EPIC's benchmarking task, the change should include a string-heavy scan benchmark. ### Scope Separate PR from Gap A, but shares the decoder-exposure change and the same decode-via-`decode_utf8_spark_lossy` policy. If Gap A lands first, exposing the decoder is already done. -- 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]
