Skip to content

fix: [branch-1.2] decode invalid UTF-8 in nested Parquet BINARY values read as strings (#7106) - #7339

Merged
andygrove merged 1 commit into
apache:branch-1.2from
manuzhang:backport-7106-1.2
Oct 30, 2026
Merged

fix: [branch-1.2] decode invalid UTF-8 in nested Parquet BINARY values read as strings (#7106)#7339
andygrove merged 1 commit into
apache:branch-1.2from
manuzhang:backport-7106-1.2

Conversation

@manuzhang

@manuzhang manuzhang commented Oct 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Which issue does this PR close?

Backport of #7106 to branch-1.2. Part of #7032 (the nested values) and #4764.

Rationale for this change

When a Parquet BINARY column without the UTF8 annotation is read as a string, through a read schema or spark.sql.parquet.binaryAsString=true, the native scan converts nested values (struct fields, list elements, map keys and values) in parquet_convert_array. That fell through to Arrow's safe cast, which turns invalid UTF-8 into NULL, so those values silently became NULL where Spark keeps the bytes. branch-1.2 has this bug. See #7106 for the full rationale.

The fix also goes to branch-1.1 in #7340, opened together with this one. Per the backporting guide, #7340 should not merge before this one is ready.

What changes are included in this PR?

The cherry-pick of 621b9cc from main, with one adaptation:

  • native/core/src/parquet/parquet_support.rs: the import block conflicted because main also imports SparkError there (added after the 1.2 cut by the timestamp-overflow change). The backport only takes the new spark_cast and SparkCastOptions imports and leaves SparkError out, since nothing on this branch uses it.

Everything else, including the docs changes to compatibility/index.md and parquet.md, applied cleanly.

How are these changes tested?

The tests from #7106: the Rust unit tests in parquet_support.rs, parquet_exec.rs and cast.rs, and the new ParquetReadSuite test "native scan decodes invalid UTF-8 in nested BINARY read as string".

Locally on this branch:

  • cargo check --all-targets, cargo clippy --all-targets --workspace -- -D warnings and cargo fmt --check in native/ pass.
  • The parquet_support and parquet_exec tests in datafusion-comet (28 tests) and the cast tests in datafusion-comet-spark-expr (111 tests) pass.
  • ./mvnw test -Dtest=none -Dsuites="org.apache.comet.parquet.ParquetReadV1Suite native scan decodes" passes after make core.

🤖 Generated with Claude Code

@manuzhang @claude
fix: decode invalid UTF-8 in nested Parquet BINARY values read as str…
944cd3d 
…ings (apache#7106)

* fix: decode invalid UTF-8 in nested Parquet BINARY values read as strings

When a Parquet BINARY column without the UTF8 annotation is read as a
string, through a read schema or spark.sql.parquet.binaryAsString, the
native scan converts nested values (struct fields, list elements, map keys
and values) in parquet_convert_array. That fell through to Arrow's safe
cast, which turns invalid UTF-8 into NULL, so those values silently became
NULL where Spark keeps the bytes.

Convert Binary to Utf8 with Spark's cast instead. It replaces each
ill-formed sequence with U+FFFD, the decode CAST(binary AS string) and
native shuffle already share (apache#4764). Top-level columns are unchanged:
DataFusion's file-schema coercion still decodes them straight into Utf8
(apache#7032).

To keep the zero-copy conversion Arrow's cast gave valid nested values,
cast_binary_to_string checks the bytes the rows span once for the default
and UTF8 styles. It doesn't check the whole value buffer, which a slice
shares with its parent. When the bytes are valid, it returns the input's
value buffer narrowed to them, with a slice's offsets rebased. Otherwise
rows inside the valid prefix are copied without being checked again, and
only the rest go through the U+FFFD decoder. This also speeds up plain
CAST(binary AS string).

Split out of apache#6246.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix: skip the UTF-8 fast path when nulls hide most of the payload

cast_binary_to_string validated every byte the rows span, including the
payload of null rows, which the row-by-row decoder skipped. After nullif
or a CASE with column branches, a mostly-null batch can keep most of its
bytes under nulls, so the cast scanned all of them and the reused buffer
kept them alive downstream.

Count the bytes of the non-null rows first. When null rows span more than
half of the bytes, skip the check and decode the live rows one at a time
as before, into a value buffer sized by the live bytes instead of the
whole span.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix: decode invalid UTF-8 in nested LargeBinary and BinaryView values

The Parquet reader keeps the types a file's ARROW:schema stores, and
DataFusion's file-schema coercion only changes top-level columns. So when
an Arrow writer stores a list element, struct field or map value as
LargeBinary or BinaryView, the native scan hands it to
parquet_convert_array as that type. Only the Binary arm used Spark's cast,
so these values still went through Arrow's safe cast and a malformed
value became NULL.

Convert LargeBinary and BinaryView to Binary first and use the same
Spark cast. For LargeBinary that rewrites the offsets and keeps the value
buffer.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix: copy a valid slice that fills little of its value buffer

A slice shares its parent's value buffer, and Buffer::slice_with_length
narrows the view but keeps the whole allocation. When
cast_binary_to_string reused a valid slice's bytes, every output batch
cut from one large array, such as aggregate output, held that array's
whole allocation. DataFusion's sort charges each batch the capacity of
its buffers, so it counted the allocation once per batch and spilled
where a compact copy didn't.

Reuse the buffer only when the rows' bytes fill at least half of its
allocation. Otherwise copy those already validated bytes into a compact
buffer with fresh offsets, which still skips the row-by-row decoder.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* refactor: share offsets when casting valid binary to string

cast_binary_to_string rebuilt the offsets whenever it copied the values
of valid input, even for an unsliced array whose offsets needed no
rebasing. Decide each buffer on its own: reuse the offsets when they
start at zero and fill at least half of their allocation, and the values
when the rows' bytes fill at least half of theirs. An unsliced array
keeps its offsets when only its value buffer is oversized, while the
first slice of a large array still gets offsets that don't hold the
parent's allocation.

Also update the function's doc comment, which still said the bytes are
taken as is, and pass parquet_convert_array_impl's Binary to Utf8 cast
only the options it reads.

Add rows to the nested ParquetReadSuite fixture: a null list element,
struct field and map value ahead of the invalid row, and a row whose
list, struct and map are null.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix: decode invalid UTF-8 in nested dictionary-encoded BINARY values

A file's ARROW:schema can dictionary-encode a nested BINARY leaf, which the
reader then returns as Dictionary(_, Binary | LargeBinary). Those fell through
to Arrow's safe cast and turned malformed values into NULL. Decode the
dictionary values through the Binary arm and expand the keys.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix: copy the validity bitmap a small slice shares with its parent

The fast path for valid input compacted the offsets and values of a slice
that fills little of their allocations, but cloned the input's validity
bitmap, which still held the parent's whole allocation. A sort that charges
every batch for what it retains then spilled where the base did not, for
example on nullable binary group keys emitted in small slices. Apply the
same rule to the bitmap: reuse it when the slice's bits fill at least half
of its allocation, and copy them otherwise.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix: keep the bytes of a map key read from unannotated BINARY

Decoding turned the keys FF and EF BF BD into the same U+FFFD, so a lookup of one
returned the other's value and map_from_entries failed with DUPLICATED_MAP_KEY where
Spark, which tells keys apart by their bytes, succeeds. A key now keeps its bytes at any
depth under a map's key field, unchecked, as a top-level value does. Map values still
decode.

Decoding a dictionary now touches only the entries the batch references, so a wide
malformed entry that one batch uses is not decoded again for every other batch of the
row group. A batch whose decoded values would not fit a string column is reported with
the byte count instead of an offset overflow.

In the binary-to-string fallback, count the live bytes over runs of non-null rows instead
of row by row, so sparse nulls don't cost a pass over the batch. In the row that holds
the first ill-formed sequence, copy its validated prefix and decode only the rest. Size
the builder for the validated prefix plus three bytes per remaining byte, and copy the
result compact when a replacement still left it filling under half of its allocation.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* fix: copy the bytes of a valid binary cast outside a scan

A plain CAST(binary AS string) can run on a batch imported over the Arrow C
stream, where every buffer keeps the whole batch alive, wide sibling columns
included, for as long as a sort or a join holds the result. Share the input's
buffers only when a scan adapts its schema, where the reader owns them, and
copy the validated bytes once otherwise, as the cast did before.

Also cover a Dictionary(Int32, Binary) map key, link the tracking issue for
the per-batch dictionary overflow from the code, and list that limit under
the known issues with the batch size and the growth of ill-formed bytes.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit 621b9cc)
@manuzhang manuzhang mentioned this pull request Oct 30, 2026
Merged
@github-actions github-actions Bot added bug Something isn't working area:scan Parquet scan / data reading area:expressions Expression evaluation labels Oct 30, 2026
andygrove
andygrove approved these changes Oct 30, 2026
View reviewed changes

@andygrove andygrove left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @manuzhang. I compared the changed lines with 621b9cc and they match the source commit exactly. The docs link into parquet.md resolves on this branch. Approving. Please let the pending Spark SQL jobs finish before merging, and merge this one before #7340.

@andygrove
andygrove merged commit 7dcae8e into apache:branch-1.2 Oct 30, 2026
140 of 141 checks passed
@andygrove

andygrove commented Oct 30, 2026

Copy link
Copy Markdown
Member

Thanks @manuzhang

@andygrove andygrove mentioned this pull request Oct 30, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andygrove andygrove andygrove approved these changes

Assignees

No one assigned

Labels

area:expressions Expression evaluation area:scan Parquet scan / data reading bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@manuzhang @andygrove