hassaanch23 opened a new pull request, #11144: URL: https://github.com/apache/arrow-rs/pull/11144
# Which issue does this PR close? - Closes #11140. This is also the root cause of [apache/datafusion#25509](https://github.com/apache/datafusion/issues/25509), where a query with `binary_as_string` segfaults. # Rationale for this change `ByteArrayColumnValueDecoder`, `ByteViewArrayColumnValueDecoder` and `DictionaryDecoder` each decide whether to validate UTF-8 from the Parquet annotation alone: ```rust let validate_utf8 = col.converted_type() == ConvertedType::UTF8; ``` But the annotation does not decide whether a string array will be built. A schema passed to `ArrowReaderOptions::with_schema` can map a plain `BYTE_ARRAY` column to `Utf8`, `LargeUtf8`, `Utf8View` or a dictionary of those, and that is a supported conversion — `test_read_binary_as_utf8` covers it. On that path validation was skipped entirely, so the reader built a string array directly over whatever bytes the column held. The result is not a wrong value, it is a broken invariant. `OffsetBuffer::into_array` finishes with `build().unwrap()` in debug and `build_unchecked()` in release, so on arbitrary bytes: - **debug**: panics inside the reader — `InvalidArgumentError("Invalid UTF8 sequence at string index 0 ...")`, an unwrap rather than a returned error; - **release**: returns `Ok` with a `StringArray` whose buffer is not valid UTF-8. Anything that then reads it as `&str` is UB. Running the test below in release on `main` aborts the process with SIGABRT while merely formatting the batch. # What changes are included in this PR? - `ColumnValueDecoder` gains `set_validate_utf8`, defaulting to a no-op so decoders that never produce string data are unaffected. - The three byte-array decoders override it, and the corresponding `make_*_reader` functions turn it on from the Arrow type they are about to build (`Utf8`/`LargeUtf8`, `Utf8View`, and a `Dictionary` whose value type is one of those). That type already accounts for the supplied schema, so the hint is what drives the flag. - `GenericRecordReader` carries the flag and applies it in `set_page_reader`, which is the single place decoders are constructed — so it also reaches decoders created for later pages, not just the first. The flag is only ever turned on, never off (`|=`), so a column annotated as a string is still validated when it is read as `Binary`. That keeps the change strictly additive with respect to existing behaviour. # Are these changes tested? Yes. `test_read_non_utf8_binary_as_utf8` already existed but asserted the bug: it was marked `#[should_panic(expected = "Invalid UTF8 sequence at")]`, i.e. it pinned the debug-mode panic as expected behaviour. It now asserts a returned error instead, and covers all four affected hints — `Utf8`, `LargeUtf8`, `Utf8View` and `Dictionary(Int32, Utf8)`. Verified in both directions: - with the fix, the test passes; reverting only the source changes and keeping the test makes it fail on the old `build().unwrap()` panic; - in release without the fix, the same test aborts the process (signal 6), which is the undefined behaviour above. Full suite on this branch: `cargo test -p parquet` (1584 tests) and `cargo test -p parquet --all-features` (1902 tests) pass, and `cargo clippy -p parquet --all-targets --all-features -- -D warnings` is clean. # Are there any user-facing changes? Reading a non-string column as a string type through a supplied schema now returns `ParquetError::General("encountered non UTF-8 data")` instead of panicking in debug or producing an invalid array in release. No public API is removed; `ColumnValueDecoder` gains one method with a default implementation, so external implementors are unaffected. # AI disclosure Per [CONTRIBUTING.md](https://github.com/apache/arrow-rs/blob/main/CONTRIBUTING.md#ai-generated-submissions): this PR was written with AI assistance (Claude Code). The diagnosis, the design, the verification steps above and this description were all reviewed by me line by line, and I ran every command quoted here locally. I am able to explain and own every line of the change. 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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]
