viirya opened a new issue, #5753:
URL: https://github.com/apache/datafusion-comet/issues/5753
### Describe the bug
Hashing a null struct reads whatever its child buffer happens to hold,
instead of
leaving the seed alone as Spark does.
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!` in
`native/spark-expr/src/hash_funcs/utils.rs`
recurses straight into the children without consulting the parent's null
buffer:
```rust
DataType::Struct(_) => {
let struct_array = col.as_any().downcast_ref::<StructArray>().unwrap();
// Hash each field of the struct - Spark hashes all fields recursively
let columns: Vec<ArrayRef> = struct_array.columns().to_vec();
if !columns.is_empty() {
$recursive_hash_method(&columns, $hashes_buffer)?;
}
}
```
The `List` and `Map` branches in the same match both guard on
`is_null(row_idx)`.
Only `Struct` does not.
Spark hashes a null struct as the seed (`case null => seed`), so two rows
with the
same logical key can hash differently depending on leftover child values.
For a
shuffle partitioning key that means equal keys can land in different
partitions,
which breaks grouping and joins. It also affects `hash()` and `xxhash64()`,
since
both go through this macro.
This is the same null-mask propagation problem that #4432 fixed for
`GetStructField`, in a different code path.
### Steps to reproduce
A struct whose row 1 is null while the child buffer still holds 999:
```rust
let fields: Fields = vec![Arc::new(Field::new("a", DataType::Int32,
true))].into();
let child: ArrayRef = Arc::new(Int32Array::from(vec![Some(1), Some(999)]));
let nulls = NullBuffer::from(vec![true, false]);
let array: ArrayRef = Arc::new(StructArray::new(fields, vec![child],
Some(nulls)));
let mut hashes = vec![42u32; 2];
create_murmur3_hashes(&[array], &mut hashes).unwrap();
```
Row 1 hashes to `3319311472`. With the child slot null instead, the same
logically
null struct hashes to `42`, the untouched seed.
### Expected behavior
A null struct should hash to the seed regardless of what its child buffers
hold, so
the two cases above agree.
### Additional context
Noticed while reviewing #5567, which admits nested types as native shuffle
hash
partitioning keys and so makes the struct branch reachable from shuffle
partitioning.
The defect itself predates that change and reproduces on `main` with no
configuration
changes. Credit to @andygrove for pointing at this branch during that
review, and to
Codex for the reproduction approach of varying only the values hidden under
the null.
--
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]
