You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "viirya (via GitHub)" <gi...@apache.org> on 2026/09/08 02:25:49 UTC
[I] Share one helper for pushing a struct's null mask into its children [datafusion-comet]
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: github-unsubscribe@datafusion.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Share one helper for pushing a struct's null mask into its children [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove closed issue #5768: Share one helper for pushing a struct's null mask into its children
URL: https://github.com/apache/datafusion-comet/issues/5768
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Share one helper for pushing a struct's null mask into its children [datafusion-comet]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on issue #5768:
URL: https://github.com/apache/datafusion-comet/issues/5768#issuecomment-5578315579
Thanks for opening this! I was chatting with Codex about how to make this general enough to also cover:
- https://github.com/apache/datafusion-comet/pull/5740 (its `repeated_visibility` in `parquet_support.rs`)
- https://github.com/apache/datafusion-comet/pull/5457 (its `prepare_nested_cast_input` in `cast.rs`)
Just wanted to share this for context, though I realize these might be tricky to fold into the same helper.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Share one helper for pushing a struct's null mask into its children [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on issue #5768:
URL: https://github.com/apache/datafusion-comet/issues/5768#issuecomment-5581379179
Opened #5769.
One correction to this issue while doing it: there is a third site, `get_array_struct_fields`,
and its guard is not merely different but wrong. It skipped the union when the parent and child
null **counts** matched, which is not the same question — equal counts can still sit at
different rows, and then a null of the parent's was dropped. So the PR carries a behaviour fix
alongside the refactor, with a test for that shape.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Share one helper for pushing a struct's null mask into its children [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on issue #5768:
URL: https://github.com/apache/datafusion-comet/issues/5768#issuecomment-5578189527
I plan to work on this once #5754 merges, since the helper would sit on top of the change there.
Flagging it so nobody duplicates the effort in the meantime — if anyone would rather take it,
say so and it is yours.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org