Repository navigation
fix(parquet): honor requested dictionary value type - #12161
Conversation
adriangb
left a comment
adriangb
left a comment
There was a problem hiding this comment.
This looks good to me. I checked interaction w/ the existing bugs where we don't validate data as utf8, this makes things no better or worse. It fixes the bug and I could find no regressions.
adriangb
left a comment
adriangb
left a comment
There was a problem hiding this comment.
I think one case is still open: plain-encoded pages read as Dict<_, Utf8>. Details and a suggested fix are inline. This is the dictionary-reader form of #11140. Could also be a followup.
End-to-end reproduction with datafusion-cli
DataFusion main does not yet ask the Parquet reader for Dictionary(Int32, Utf8) on a BINARY column. It reads Binary and casts, and the cast validates. To show the path DataFusion would use once it relies on this PR (see apache/datafusion#26976), I built datafusion-cli from DataFusion main with [patch.crates-io] pointing at this branch, plus one coercion arm in datafusion/datasource-parquet/src/schema_coercion.rs:
(Dictionary(_, v), Binary) if enable_rle && binary_to_string && **v == Utf8 => {
Some(table_type.clone())
}-- 2000 distinct values overflow a 64-byte dictionary page, so the writer
-- falls back to plain pages. The last value (0xFF) is not valid UTF-8.
COPY (
SELECT CASE WHEN v = 2000 THEN decode('ff', 'hex')
ELSE arrow_cast(CAST(v AS VARCHAR), 'Binary') END AS b
FROM generate_series(1, 2000) t(v) ORDER BY v
) TO 'mre.parquet' STORED AS PARQUET
OPTIONS ('format.dictionary_page_size_limit' '64');
SET datafusion.execution.parquet.binary_as_string = true;
SET datafusion.execution.parquet.enable_rle_to_dictionary = true;
SELECT arrow_typeof(b) AS t, count(*) FROM 'mre.parquet' GROUP BY 1;
SELECT length(b) FROM 'mre.parquet' ORDER BY length(b) LIMIT 1;On this branch, the scan returns Dictionary(Int32, Utf8) with no error. Printing the invalid value then aborts the process:
unsafe precondition(s) violated: hint::unreachable_unchecked must never be reached
With the inline suggestion applied, the same query fails cleanly:
Arrow error: Parquet argument error: External: Invalid argument error: Encountered non UTF-8 data: invalid utf-8 sequence of 1 bytes from index 6889
9eccacf to
eff59bb
Compare
etseidl
left a comment
etseidl
left a comment
There was a problem hiding this comment.
Thanks @Rich-T-kid. I worked my way through the logic and it seems correct to me. I let my agent handle the test coverage and that was good 👍.
One nit my agent was concerned with is convert_dictionary_values doesn't work in the reverse direction (utf -> binary), but admits that's not an issue in actual practice, it's only reachable via the factory, not through ArrowReaderOptions::with_schema.
apache#12161 decodes an unannotated BYTE_ARRAY dictionary column as Binary and converts the values with StringArray::try_from_binary, which validates UTF-8. That covers the dictionary reader this PR used to change, so the merge takes main's byte_array_dictionary.rs unchanged. The remaining changes are in the non-dictionary readers. test_read_non_utf8_binary_as_utf8 now matches "non UTF-8 data", like apache#12161's tests, because the dictionary path reports "Encountered non UTF-8 data" and the decoders "encountered non UTF-8 data". Co-Authored-By: Claude Opus 5.5 <[email protected]>
Which issue does this PR close?
Rationale for this change
A Parquet
BYTE_ARRAYcolumn without a UTF8 annotation is physically decoded asBinary. When a caller supplies a schema requestingDictionary(Int32, Utf8), the dictionary reader previously still returnedDictionary(Int32, Binary), requiring the scan consumer to cast every emitted batch.For a dictionary page containing potentially thousands of unique values:
Before this PR, batches from the same dictionary page repeatedly converted and allocated the entire dictionary values array:
After this PR, the reader converts the unique values once per dictionary page and shares the result with every batch:
The potentially millions of keys are preserved; only the unique dictionary values are converted. Invalid UTF-8 still returns an error. Plain-encoded fallback pages retain the existing dictionary-building path.
Are these changes tested?
Coverage includes binary dictionary pages requested as
Utf8,LargeUtf8, andUtf8View, reuse across batches, nulls, invalid UTF-8 rejection, and plain-encoded fallback pages.End-to-end hosted benchmark evidence is recorded in apache/datafusion#27312. With RLE dictionary reads enabled on both the exact parent and dependency head, total time across TPC-H, TPC-DS, and partitioned ClickBench improved by 22.40% (49.147s to 38.140s); partitioned ClickBench improved by 27.60%. All suites completed without query failures.
Are there any significant user-facing changes?
No.