viirya opened a new pull request, #5769:
URL: https://github.com/apache/datafusion-comet/pull/5769
## Which issue does this PR close?
Closes #5768.
## Rationale for this change
Three places implemented the same Spark rule — a field of a null struct is
null, so the
parent's nulls have to reach the children before the children are read — and
each did it
slightly differently:
| | union when |
|---|---|
| `GetStructField::project_field` | a null buffer is present, via the
checked `ArrayData` builder |
| the hash kernels' struct branch | `null_count() > 0`, via
`StructArray::flatten` |
| `get_array_struct_fields` | the parent and child null **counts** differ |
The issue was filed as a refactor, on the grounds that all three were
correct and the risk
was drift. That turned out to be half right: **the third guard is wrong**.
Equal null counts
do not imply the nulls sit at the same rows, so whenever the child had a
null of its own at a
different row the counts could match and the parent's null was dropped.
There is a test for
exactly that shape, which a count-based check fails.
## What changes are included in this PR?
`datafusion_comet_common::struct_nulls`, with `child_with_parent_nulls` for
one field and
`children_with_parent_nulls` for all of them, and the three call sites
switched over. Net -41
lines at the call sites.
The three decisions the issue asked about:
- **Shape.** Both, with the plural built on the singular, since
`project_field` wants one
field and the hash path wants all of them.
- **Checked or unchecked.** Unchecked, as `flatten` already did. The union
only adds nulls, so
every data buffer is carried over unchanged and revalidating them is
wasted work. This
matters asymmetrically: `project_field` runs once per batch in `evaluate`
and did not care
either way, while the hash path reaches this **once per element** of an
`array<struct<..>>`,
where re-checking a string child means rescanning its whole UTF-8 values
buffer. Unifying on
checked would have pushed the batch-level caller's cost onto the
element-level one.
- **The `null_count() > 0` guard.** Applied to all three. It skips the union
when the parent
has no null to contribute, which covers both no buffer at all and a
present-but-all-valid
buffer — the latter is what slicing leaves behind. `NullBuffer` stores its
null count, so the
test is O(1).
It lives in its own module rather than in `schema.rs`, which is about
reconciling declared and
runtime *types* rather than null masks.
## How are these changes tested?
Six unit tests on the helper: a hidden child value under a null parent, no
parent null buffer
(asserting the same array comes back rather than a rebuilt one), a
present-but-all-valid
buffer, a child's own null surviving the union, an empty struct, and the
equal-counts-different-rows
case that pins the guard fix.
Existing coverage for the three call sites passes unchanged: `cargo test`
gives 720 + 5 in
`spark-expr` and 40 in `common`, and on the Spark side
`CometHashExpressionSuite` (40),
`CometArrayExpressionSuite` (59) and `CometExpressionSuite` (141). `cargo
fmt --check` and
`clippy -D warnings` are clean.
## Additional context
Raised by @andygrove reviewing #5754. The two earlier fixes in this area
were #4432
(`GetStructField`) and #5754 (the hash kernels); this unifies them with the
third site, which
the issue had not accounted for.
--
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]
