andygrove opened a new pull request, #5823:
URL: https://github.com/apache/datafusion-comet/pull/5823
Backport of #5754 to `branch-1.0`.
Cherry-picked from `bb9e74020adc228e486f6f4d0fa68292b30bff31` with no
conflicts and no
modifications — the diff is byte-identical to the original PR.
#5815 lists this one as "apply after #5757", and #5757 landed on
`branch-1.0` as #5817, so that
ordering is already satisfied. The two touch the same three files but not
the same hunks, and the
cherry-pick applied cleanly on top.
## Which issue does this PR close?
Closes #5753 on `branch-1.0`.
## 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. What the tests
demonstrate directly is a
wrong answer from `hash` and `xxhash64` in a Spark query. The same hash
decides shuffle partition
assignment, so equal keys reaching different partitions follows from it, but
that path is not
exercised by these tests.
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`: use `StructArray::flatten`, which unions the parent's nulls
into each child, and does
it without revalidating the child data buffers — the union only ever adds
nulls, so the buffers
are unchanged. Going through the checked `ArrayData` builder instead would
rescan every child
buffer on each call, and for a string child that is the whole UTF-8 values
buffer.
- `flatten` is called only when the parent actually has a null, since with a
buffer present it
builds a fresh `Fields` with every non-nullable field re-marked nullable,
which this call site
discards.
- Regression tests for both hash algorithms, since the struct branch is
shared through the macro.
## How are these changes tested?
Same tests as the original PR, verified locally on `branch-1.0`:
- `cargo test -p datafusion-comet-spark-expr` passes 619 + 3 tests.
- `cargo clippy -p datafusion-comet-spark-expr --all-targets -- -D warnings`
and
`cargo fmt --all -- --check` are clean.
- `CometHashExpressionSuite` passes 40 on the default profile (Spark 4.1.3 /
Scala 2.13).
The defect is present on `branch-1.0`, so this backport is what removes it.
With the `utils.rs`
change reverted and the tests kept, exactly the seven new tests fail and
nothing else does:
- The four unit tests reproduce the original PR's values — under murmur3 the
null row hashes
`3319311472` rather than the `42` seed. The other 17 hash unit tests still
pass.
- The three end-to-end tests fail and the other 37 in the suite pass. For
the scalar-child case,
Comet returns `[-1823081949, 6258084186791473711]` for a row where Spark
returns the seed pair
`[42, 42]`.
The patch is byte-identical to `bb9e7402`, but the dependency environment is
not the one it was
merged into: `branch-1.0` resolves DataFusion 54.1.0 and Arrow 58.4.0,
whereas `main` at that commit
resolves DataFusion 55.0.0 and Arrow 59.3.0. It compiled unmodified because
`StructArray::flatten`
has the same signature and semantics in Arrow 58.4 — but that is an
observation about this one API,
not a general property of the two branches.
## Are there any user-facing changes?
A null struct now hashes to the incoming seed, matching Spark, instead of
hashing whatever its child
buffers happened to hold. Hash values change only for that case, which is
the point of the fix.
--
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]
