viirya opened a new pull request, #5754:
URL: https://github.com/apache/datafusion-comet/pull/5754
## Which issue does this PR close?
Closes #5753.
## Rationale for this change
Arrow keeps a `StructArray`'s children validity independent of the parent's,
so at a
row where the struct itself is null a child buffer can still hold a value.
The struct
branch of `create_hashes_internal!` recursed straight into
`struct_array.columns()`
without consulting the parent's null buffer, so a null struct hashed
whatever happened
to sit in the child slot instead of leaving the seed alone as Spark does
(`case null => seed`).
Two rows with the same logical key then hash differently. For a shuffle
partitioning
key that means equal keys can reach different partitions, which breaks
grouping and
joins; it also affects `hash()` and `xxhash64()`. The `List` and `Map`
branches in the
same match already guard on `is_null`; only `Struct` did not.
## What changes are included in this PR?
- `utils.rs`: push the parent's nulls into each child with
`NullBuffer::union` before
recursing, which is how #4432 fixed the same null-mask propagation problem
in
`GetStructField`. When the struct has no null buffer the children are
passed through
untouched, as before.
- Regression tests for both hash algorithms. The struct branch is shared
through the
macro, so murmur3 and xxhash64 are both affected and both are covered.
## How are these changes tested?
`cargo test -p datafusion-comet-spark-expr` passes 712 + 5 tests.
The two new tests build a struct whose row 1 is null while the child buffer
still
holds 999, and assert it hashes the same as the equivalent struct whose
child slot is
also null, and that the seed survives. Both fail without the change — under
murmur3
the null row hashes `3319311472` rather than the `42` seed.
Worth noting that none of the 18 pre-existing hash tests change, which is
the signal
that this only moves the null-struct case that was already wrong.
## Additional context
Found while reviewing #5567, which makes this branch reachable from shuffle
partitioning, but the defect predates it and reproduces on `main` unchanged.
Thanks to
@andygrove for pointing at this branch during that review.
--
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]
