apache / apache/datafusion-comet
[EPIC] Consistent handling of invalid UTF-8 in native StringType (ingress policy)
- Dominant language
- Scala
- Stars
- 1.3k
- Forks
- 373
- Avg merge
- 2d 4h
- Merged PRs (30d)
- 198
Description
## Background
Spark's `StringType` can hold arbitrary bytes, including sequences that are not valid UTF-8 (Spark stores them verbatim via `UTF8String`). Arrow's `Utf8`/`LargeUtf8` types require valid UTF-8, and arrow-rs `StringArray::value` hands out a `&str` via an *unchecked* decode that trusts the array's UTF-8 invariant. As a result, **any native string kernel that reads an Arrow string array containing invalid UTF-8 (via `value()`) materializes an invalid `&str`, which is undefined behaviour** (iterating chars, slicing on char boundaries, etc. can misbehave, panic, or be miscompiled).
Comet currently handles invalid-UTF-8 strings **inconsistently** across sites. This EPIC tracks giving Comet a single, coherent policy.
## Current state
**String-*producing* sites (resolved -- decode JVM-compatibly to valid UTF-8):**
| Site | Behavior |
|------|----------|
| Native shuffle `get_string` | Decodes via `decode_utf8_spark_lossy` (matches the JVM's `new String(bytes, UTF_8)` byte-for-byte, including surrogate-range cases). #4521 |
| `CAST(binary AS string)` | Same `decode_utf8_spark_lossy` (shared with shuffle). #4488 / PR #4763 |
Both produce valid UTF-8 that matches Spark's rendered output, so neither feeds the downstream-kernel UB.
**String-*ingress* sites (the gaps):**
| Site | Behavior | Problem |
|------|----------|---------|
| Native (Rust) Parquet reader | **Rejects** invalid UTF-8 (`encountered non UTF-8 data`) | Comet errors where Spark succeeds (#4121) |
| JVM -> native Arrow FFI import (`jni_api.rs` `from_ffi`) | **Passes bytes through unchecked** | Latent UB: invalid UTF-8 enters the native pipeline and downstream `value()` is unsound |
The two ingress paths are simultaneously **too strict** (reject) and **too loose** (unchecked) for the *same* input.
### Gap A -- native scan rejects invalid UTF-8 (#4121)
The arrow-rs Parquet reader validates UTF-8 and errors on non-UTF-8 `STRING` columns. Spark reads them fine. Surfaces in Spark 4.1.1's `hll.sql`; currently worked around by disabling Comet for that file.
### Gap B -- FFI import does not validate (confirmed)
`arrow::ffi::from_ffi` builds the imported array with `ArrayData::new_unchecked` and does **not** validate UTF-8 (arrow-array `src/ffi.rs` even carries a `// Should FFI be checking validity?` comment). Comet adds no validation on the import side (`prepare_output`'s `validate_full()` runs only on the *output/export* path under a debug flag). So invalid-UTF-8 string bytes arriving from a JVM-side source (a JVM-side scan, or any Spark -> Comet columnar handoff) enter the native pipeline as an Arrow `Utf8` array that lies about its validity.
**Severity:** a latent default-config UB hazard. It requires (1) genuinely non-UTF-8 string bytes, (2) flowing through the unchecked FFI path rather than the validating native reader, and (3) consumption by a native string kernel that reads `&str`. That conjunction is uncommon, which is why it has not surfaced as crashes -- but it is unsound by default.
## Proposed direction
Establish a single ingress policy: **native string data must be valid UTF-8.** Decode (not reinterpret) at the boundary using the existing `decode_utf8_spark_lossy` (in `spark-expr/src/utils.rs`), which is zero-copy for the valid-UTF-8 common case:
- **Gap A:** read the Parquet `STRING` column as `Binary`, then convert to `Utf8` via `decode_utf8_spark_lossy` instead of erroring -- consistent with cast/shuffle and with Spark.
- **Gap B:** decode imported `Utf8`/`LargeUtf8` columns through `decode_utf8_spark_lossy` at the FFI import boundary.
This unifies all four sites on one decoder and removes the UB. **It touches the scan/FFI hot path, so it needs a perf evaluation** (the `from_utf8` validation pass is SIMD-fast and the native Rust reader already pays it, but string-heavy workloads should be benchmarked) and belongs in its own PR, not bundled with the cast fix.
### Alternatives considered
- Document + accept as a rare known limitation (leaves the UB in default config).
- Validate-and-error at import like the native scan (trades rare UB for rare query failures / Spark-incompatibility).
- Harden every string kernel to be byte-safe (impractically large surface).
## Observable caveat (already documented)
Decoding (rather than preserving raw bytes) diverges from Spark only under byte-level round-trips: `CAST(CAST(X'FF' AS STRING) AS BINARY)` returns `X'FF'` in Spark but `X'EFBFBD'` (UTF-8 of U+FFFD) in Comet; likewise `octet_length`/hashing on the raw bytes. This is noted in the [compatibility guide](https://datafusion.apache.org/comet/user-guide/latest/compatibility/index.html) and the contributor guide.
## Tasks
- [ ] Decide the ingress policy (decode-at-boundary vs alternatives)
- [ ] Gap A: fix the native scan to read+decode invalid-UTF-8 `STRING` columns (#4121); re-enable `hll.sql`
- [ ] Gap B: decode imported string columns at the FFI boundary
- [ ] Benchmark the validation/decode cost on string-heavy scans and FFI handoffs
- [ ] Confirm there are no other ingress points (e.g. row -> columnar conversion)
## Related
- #4488 / PR #4763 -- `CAST(binary AS string)` decode (done)
- #4521 -- native shuffle `get_string` decode (done)
- #4121 -- native scan rejects invalid UTF-8 (Gap A)
Contributor guide
Research direction
Start by reviewing spark-expr/src/utils.rs for decode_utf8_spark_lossy, the native Parquet reader path for STRING columns, and jni_api.rs from_ffi for imported Utf8/LargeUtf8 arrays. Trace both ingress paths, inspect hll.sql, and benchmark string-heavy scans and FFI handoffs. Done means the chosen policy is implemented at both gaps, hll.sql can be re-enabled, and other ingress points are checked.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- data-engineering
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100