viirya opened a new issue, #5768:
URL: https://github.com/apache/datafusion-comet/issues/5768
## Is your feature request related to a problem or challenge?
Two places implement the same Spark rule — a field of a null struct is null,
so the parent's
null mask has to reach the children before the children are used — and they
do it differently.
`struct_funcs/get_struct_field.rs`, from #4432:
```rust
fn project_field(struct_array: &StructArray, ordinal: usize) ->
DataFusionResult<ArrayRef> {
let child = struct_array.column(ordinal);
match struct_array.nulls() {
Some(_) => {
let combined = NullBuffer::union(struct_array.nulls(),
child.nulls());
let data =
child.to_data().into_builder().nulls(combined).build()?;
Ok(make_array(data))
}
None => Ok(Arc::clone(child)),
}
}
```
`hash_funcs/utils.rs`, from #5754:
```rust
let columns: Vec<ArrayRef> = match struct_array.nulls() {
Some(nulls) if nulls.null_count() > 0 => struct_array.flatten().1,
_ => struct_array.columns().to_vec(),
};
```
Both are correct today, so this is not a bug report. The concern is drift:
the next time this
rule needs adjusting, or another edge case turns up, only one of them is
likely to be updated,
and the checked-versus-unchecked reasoning lives in two places rather than
one.
## Describe the solution you'd like
A single helper both can call. Three decisions to settle first, which is why
this is filed
separately rather than folded into #5754:
1. **Shape.** `project_field` extracts one field; the hash path needs all of
them. One helper
returning a single child, with the caller mapping over the fields, or one
returning the whole
`Vec<ArrayRef>`, or both built on a shared core.
2. **Checked or unchecked.** `project_field` goes through the checked
`ArrayData` builder;
`flatten` uses `build_unchecked` with the safety argument that the union
only ever adds nulls
so the data buffers are unchanged. Unifying on unchecked means making
that argument for
`project_field` too. Unifying on checked reintroduces revalidation of
every child buffer on
the hash path, which runs once per element for `array<struct<..>>` — for
a string child that
is the whole UTF-8 values buffer.
3. **Whether `project_field` should take the `null_count() > 0` guard.** The
hash path skips the
union when the buffer is present but all valid, which is what slicing
leaves behind.
`project_field` currently does the work whenever a buffer exists.
## Describe alternatives you've considered
Leaving both in place. They work, and the duplication is small; but the
reasoning behind the
unchecked path is only recorded at one of the two call sites, which is the
part likely to be
lost.
## Additional context
Raised by @andygrove reviewing #5754.
--
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]
