You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "AdamGS (via GitHub)" <gi...@apache.org> on 2026/09/09 15:36:35 UTC
[PR] Handle nulls correctly when extracting nested arrays from nullable struct/map [datafusion]
AdamGS opened a new pull request, #25122:
URL: https://github.com/apache/datafusion/pull/25122
## Which issue does this PR close?
- Closes #25120.
## Rationale for this change
Fixes null handling when accessing nested map/struct arrays.
## What changes are included in this PR?
Taking parent nulls into account when extracting nested arrays from map/struct arrays.
## What is the testing strategy for this PR?
Additional unit tests.
## Are there any user-facing changes?
Correct behavior.
--
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: [PR] fix: Handle nulls correctly when extracting nested arrays from nullable struct/map [datafusion]
Posted by "AdamGS (via GitHub)" <gi...@apache.org>.
AdamGS commented on code in PR #25122:
URL: https://github.com/apache/datafusion/pull/25122#discussion_r4006909637
##########
datafusion/functions/src/core/getfield.rs:
##########
@@ -182,6 +191,37 @@ fn process_map_with_nested_key(
Ok(ColumnarValue::Array(data))
}
+/// Apply a struct's nulls to one of its fields.
+fn apply_parent_nulls(col: &ArrayRef, parent_nulls: &NullBuffer) -> Result<ArrayRef> {
Review Comment:
done, does make things simpler
##########
datafusion/functions/src/core/getfield.rs:
##########
@@ -733,6 +780,160 @@ mod tests {
Ok(())
}
+ #[test]
+ fn test_get_field_nested_struct_outer_nulls() -> Result<()> {
+ let inner_array = StructArray::new(
+ vec![Field::new("value", DataType::Int32, false)].into(),
+ vec![Arc::new(Int32Array::from(vec![1, 2, 3]))],
+ None,
+ );
+
+ // Only the outer struct marks row 1 as null; its children are all valid.
+ let outer_array = StructArray::new(
+ vec![Field::new("inner", inner_array.data_type().clone(), false)].into(),
+ vec![Arc::new(inner_array)],
+ Some(NullBuffer::from(vec![true, false, true])),
+ );
+
+ let inner = extract_single_field(
+ ColumnarValue::Array(Arc::new(outer_array)),
+ ScalarValue::Utf8(Some("inner".to_string())),
+ )?;
+ let result =
+ extract_single_field(inner, ScalarValue::Utf8(Some("value".to_string())))?
+ .into_array(3)?;
+
+ let expected = Int32Array::from(vec![Some(1), None, Some(3)]);
+ assert_eq!(result.as_ref(), &expected as &dyn Array);
+
+ Ok(())
+ }
+
+ #[test]
+ fn test_get_field_map_parent_nulls() -> Result<()> {
+ use arrow::array::{FixedSizeListArray, MapArray};
+ use arrow_buffer::OffsetBuffer;
+
+ let keys = Arc::new(Int32Array::from(vec![7; 3])) as ArrayRef;
+ let nested_keys = Arc::new(FixedSizeListArray::new(
+ Arc::new(Field::new("item", DataType::Int32, false)),
+ 1,
+ Arc::clone(&keys),
+ None,
+ )) as ArrayRef;
+
+ // Exercise both map lookup paths. The null map has a valid matching entry.
+ for keys in [keys, nested_keys] {
+ let key = ScalarValue::try_from_array(keys.as_ref(), 0)?;
+ let entries = StructArray::new(
+ vec![
+ Field::new("key", keys.data_type().clone(), false),
+ Field::new("value", DataType::Int32, false),
+ ]
+ .into(),
+ vec![keys, Arc::new(Int32Array::from(vec![1, 2, 3]))],
+ None,
+ );
+ let map = MapArray::new(
+ Arc::new(Field::new("entries", entries.data_type().clone(), false)),
+ OffsetBuffer::new(vec![0, 1, 2, 3].into()),
+ entries,
+ Some(NullBuffer::from(vec![true, false, true])),
+ false,
+ );
+ let result = extract_single_field(ColumnarValue::Array(Arc::new(map)), key)?
+ .into_array(3)?;
+ let expected = Int32Array::from(vec![Some(1), None, Some(3)]);
+ assert_eq!(result.as_ref(), &expected as &dyn Array);
+ }
+ Ok(())
+ }
+
+ #[test]
+ fn test_get_field_null_typed_child() -> Result<()> {
+ use arrow::array::{DictionaryArray, NullArray, UInt32Array};
+ use arrow::datatypes::UInt32Type;
+
+ let values = Arc::new(StructArray::new(
+ vec![Field::new("value", DataType::Null, true)].into(),
+ vec![Arc::new(NullArray::new(2))],
+ Some(NullBuffer::from(vec![true, false])),
+ )) as ArrayRef;
+ let dictionary = Arc::new(DictionaryArray::<UInt32Type>::try_new(
+ UInt32Array::from(vec![0, 1]),
+ Arc::clone(&values),
+ )?) as ArrayRef;
+
+ for input in [values, dictionary] {
+ let result = extract_single_field(
+ ColumnarValue::Array(input),
+ ScalarValue::Utf8(Some("value".to_string())),
+ )?
+ .into_array(2)?;
+ assert_eq!(result.logical_null_count(), 2);
+ }
+ Ok(())
+ }
+
+ #[test]
+ fn test_get_field_union_parent_nulls() -> Result<()> {
+ use arrow::array::{DictionaryArray, StringArray, UInt32Array};
+ use arrow::datatypes::UInt32Type;
+
+ let fields = UnionFields::try_new(
+ [3, 7],
+ [
+ Field::new("int", DataType::Int32, true),
+ Field::new("string", DataType::Utf8, true),
+ ],
+ )?;
+ let ints = Int32Array::from(vec![Some(9), Some(1), Some(2), None, Some(4)]);
+ let strings = StringArray::from(vec!["x"; 5]);
+
+ // Dense rows 1 and 2 share a value, but only row 2 has a null parent.
+ for offsets in [None, Some(vec![0, 1, 1, 3, 0].into())] {
+ let child = UnionArray::try_new(
+ fields.clone(),
+ vec![7, 3, 3, 3, 7].into(),
+ offsets,
+ vec![Arc::new(ints.clone()), Arc::new(strings.clone())],
+ )?;
+ let union_type = child.data_type().clone();
+ let parent = StructArray::new(
+ vec![Field::new("u", union_type.clone(), true)].into(),
+ vec![Arc::new(child)],
+ Some(NullBuffer::from(vec![true, true, false, true, true])),
+ );
+ let values = Arc::new(parent.slice(1, 4)) as ArrayRef;
Review Comment:
added that case
--
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: [PR] fix: Handle nulls correctly when extracting nested arrays from nullable struct/map [datafusion]
Posted by "AdamGS (via GitHub)" <gi...@apache.org>.
AdamGS commented on code in PR #25122:
URL: https://github.com/apache/datafusion/pull/25122#discussion_r3988076302
##########
datafusion/functions/src/core/getfield.rs:
##########
@@ -236,9 +251,19 @@ fn extract_single_field(base: ColumnarValue, name: ScalarValue) -> Result<Column
}
(DataType::Struct(_), _, Some(k)) => {
let as_struct_array = as_struct_array(&array)?;
- match as_struct_array.column_by_name(&k) {
- None => exec_err!("Field {k} not found in struct"),
- Some(col) => Ok(ColumnarValue::Array(Arc::clone(col))),
+ let nulls = as_struct_array.nulls();
+ match (as_struct_array.column_by_name(&k), nulls) {
+ (None, _) => exec_err!("Field {k} not found in struct"),
+ (Some(col), None) => Ok(ColumnarValue::Array(Arc::clone(col))),
+ (Some(col), Some(parent_nulls)) => {
+ // NullArray is already entirely null and cannot have a validity bitmap.
+ if col.data_type().is_null() {
+ return Ok(ColumnarValue::Array(Arc::clone(col)));
+ }
+ let nulls = NullBuffer::union(col.nulls(), Some(parent_nulls));
+ let data = col.to_data().into_builder().nulls(nulls).build()?;
Review Comment:
Nice find! I'll add the test and a 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: 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: [PR] fix: Handle nulls correctly when extracting nested arrays from nullable struct/map [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on code in PR #25122:
URL: https://github.com/apache/datafusion/pull/25122#discussion_r4005953654
##########
datafusion/functions/src/core/getfield.rs:
##########
@@ -182,6 +191,37 @@ fn process_map_with_nested_key(
Ok(ColumnarValue::Array(data))
}
+/// Apply a struct's nulls to one of its fields.
+fn apply_parent_nulls(col: &ArrayRef, parent_nulls: &NullBuffer) -> Result<ArrayRef> {
Review Comment:
Might be a net simplification if this took `Option<&NullBuffer>` instead.
##########
datafusion/functions/src/core/getfield.rs:
##########
@@ -733,6 +780,160 @@ mod tests {
Ok(())
}
+ #[test]
+ fn test_get_field_nested_struct_outer_nulls() -> Result<()> {
+ let inner_array = StructArray::new(
+ vec![Field::new("value", DataType::Int32, false)].into(),
+ vec![Arc::new(Int32Array::from(vec![1, 2, 3]))],
+ None,
+ );
+
+ // Only the outer struct marks row 1 as null; its children are all valid.
+ let outer_array = StructArray::new(
+ vec![Field::new("inner", inner_array.data_type().clone(), false)].into(),
+ vec![Arc::new(inner_array)],
+ Some(NullBuffer::from(vec![true, false, true])),
+ );
+
+ let inner = extract_single_field(
+ ColumnarValue::Array(Arc::new(outer_array)),
+ ScalarValue::Utf8(Some("inner".to_string())),
+ )?;
+ let result =
+ extract_single_field(inner, ScalarValue::Utf8(Some("value".to_string())))?
+ .into_array(3)?;
+
+ let expected = Int32Array::from(vec![Some(1), None, Some(3)]);
+ assert_eq!(result.as_ref(), &expected as &dyn Array);
+
+ Ok(())
+ }
+
+ #[test]
+ fn test_get_field_map_parent_nulls() -> Result<()> {
+ use arrow::array::{FixedSizeListArray, MapArray};
+ use arrow_buffer::OffsetBuffer;
+
+ let keys = Arc::new(Int32Array::from(vec![7; 3])) as ArrayRef;
+ let nested_keys = Arc::new(FixedSizeListArray::new(
+ Arc::new(Field::new("item", DataType::Int32, false)),
+ 1,
+ Arc::clone(&keys),
+ None,
+ )) as ArrayRef;
+
+ // Exercise both map lookup paths. The null map has a valid matching entry.
+ for keys in [keys, nested_keys] {
+ let key = ScalarValue::try_from_array(keys.as_ref(), 0)?;
+ let entries = StructArray::new(
+ vec![
+ Field::new("key", keys.data_type().clone(), false),
+ Field::new("value", DataType::Int32, false),
+ ]
+ .into(),
+ vec![keys, Arc::new(Int32Array::from(vec![1, 2, 3]))],
+ None,
+ );
+ let map = MapArray::new(
+ Arc::new(Field::new("entries", entries.data_type().clone(), false)),
+ OffsetBuffer::new(vec![0, 1, 2, 3].into()),
+ entries,
+ Some(NullBuffer::from(vec![true, false, true])),
+ false,
+ );
+ let result = extract_single_field(ColumnarValue::Array(Arc::new(map)), key)?
+ .into_array(3)?;
+ let expected = Int32Array::from(vec![Some(1), None, Some(3)]);
+ assert_eq!(result.as_ref(), &expected as &dyn Array);
+ }
+ Ok(())
+ }
+
+ #[test]
+ fn test_get_field_null_typed_child() -> Result<()> {
+ use arrow::array::{DictionaryArray, NullArray, UInt32Array};
+ use arrow::datatypes::UInt32Type;
+
+ let values = Arc::new(StructArray::new(
+ vec![Field::new("value", DataType::Null, true)].into(),
+ vec![Arc::new(NullArray::new(2))],
+ Some(NullBuffer::from(vec![true, false])),
+ )) as ArrayRef;
+ let dictionary = Arc::new(DictionaryArray::<UInt32Type>::try_new(
+ UInt32Array::from(vec![0, 1]),
+ Arc::clone(&values),
+ )?) as ArrayRef;
+
+ for input in [values, dictionary] {
+ let result = extract_single_field(
+ ColumnarValue::Array(input),
+ ScalarValue::Utf8(Some("value".to_string())),
+ )?
+ .into_array(2)?;
+ assert_eq!(result.logical_null_count(), 2);
+ }
+ Ok(())
+ }
+
+ #[test]
+ fn test_get_field_union_parent_nulls() -> Result<()> {
+ use arrow::array::{DictionaryArray, StringArray, UInt32Array};
+ use arrow::datatypes::UInt32Type;
+
+ let fields = UnionFields::try_new(
+ [3, 7],
+ [
+ Field::new("int", DataType::Int32, true),
+ Field::new("string", DataType::Utf8, true),
+ ],
+ )?;
+ let ints = Int32Array::from(vec![Some(9), Some(1), Some(2), None, Some(4)]);
+ let strings = StringArray::from(vec!["x"; 5]);
+
+ // Dense rows 1 and 2 share a value, but only row 2 has a null parent.
+ for offsets in [None, Some(vec![0, 1, 1, 3, 0].into())] {
+ let child = UnionArray::try_new(
+ fields.clone(),
+ vec![7, 3, 3, 3, 7].into(),
+ offsets,
+ vec![Arc::new(ints.clone()), Arc::new(strings.clone())],
+ )?;
+ let union_type = child.data_type().clone();
+ let parent = StructArray::new(
+ vec![Field::new("u", union_type.clone(), true)].into(),
+ vec![Arc::new(child)],
+ Some(NullBuffer::from(vec![true, true, false, true, true])),
+ );
+ let values = Arc::new(parent.slice(1, 4)) as ArrayRef;
Review Comment:
Might be useful to test struct slicing for the non-union path? e.g., add a `slice` to `test_get_field_nested_struct_outer_nulls` or add a new test.
--
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: [PR] fix: Handle nulls correctly when extracting nested arrays from nullable struct/map [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25122:
URL: https://github.com/apache/datafusion/pull/25122#issuecomment-5609701189
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/25122?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `86.00000%` with `14 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 81.80%. Comparing base ([`4048898`](https://app.codecov.io/gh/apache/datafusion/commit/40488988ad596c9b093ad60e1453430d803ce33c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`8a137f8`](https://app.codecov.io/gh/apache/datafusion/commit/8a137f8b45b9404381758348de7f4673bca07fe9?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/25122?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/functions/src/core/getfield.rs](https://app.codecov.io/gh/apache/datafusion/pull/25122?src=pr&el=tree&filepath=datafusion%2Ffunctions%2Fsrc%2Fcore%2Fgetfield.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9mdW5jdGlvbnMvc3JjL2NvcmUvZ2V0ZmllbGQucnM=) | 84.94% | [3 Missing and 11 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25122?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #25122 +/- ##
==========================================
- Coverage 81.80% 81.80% -0.01%
==========================================
Files 1130 1130
Lines 417754 417843 +89
Branches 417754 417843 +89
==========================================
+ Hits 341754 341820 +66
- Misses 55875 55881 +6
- Partials 20125 20142 +17
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/25122?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: [PR] fix: Handle nulls correctly when extracting nested arrays from nullable struct/map [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway merged PR #25122:
URL: https://github.com/apache/datafusion/pull/25122
--
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: [PR] fix: Handle nulls correctly when extracting nested arrays from nullable struct/map [datafusion]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #25122:
URL: https://github.com/apache/datafusion/pull/25122#discussion_r3988047972
##########
datafusion/functions/src/core/getfield.rs:
##########
@@ -236,9 +251,19 @@ fn extract_single_field(base: ColumnarValue, name: ScalarValue) -> Result<Column
}
(DataType::Struct(_), _, Some(k)) => {
let as_struct_array = as_struct_array(&array)?;
- match as_struct_array.column_by_name(&k) {
- None => exec_err!("Field {k} not found in struct"),
- Some(col) => Ok(ColumnarValue::Array(Arc::clone(col))),
+ let nulls = as_struct_array.nulls();
+ match (as_struct_array.column_by_name(&k), nulls) {
+ (None, _) => exec_err!("Field {k} not found in struct"),
+ (Some(col), None) => Ok(ColumnarValue::Array(Arc::clone(col))),
+ (Some(col), Some(parent_nulls)) => {
+ // NullArray is already entirely null and cannot have a validity bitmap.
+ if col.data_type().is_null() {
+ return Ok(ColumnarValue::Array(Arc::clone(col)));
+ }
+ let nulls = NullBuffer::union(col.nulls(), Some(parent_nulls));
+ let data = col.to_data().into_builder().nulls(nulls).build()?;
Review Comment:
There is one remaining edge case here: extracting a Union child from a nullable Struct now fails with `cannot contain a null bitmask`. Union arrays cannot carry a validity bitmap, so excluding only `DataType::Null` does not cover every child type.
I would treat this as a non-blocking follow-up, since the previous implementation also lost the parent’s nulls for this case.
<details>
<summary>Reproducer</summary>
Tested on `8a137f8b45b9404381758348de7f4673bca07fe9`. Add this to the existing tests in `getfield.rs` and run:
```sh
cargo test -p datafusion-functions --lib review_get_field_union_parent_nulls --locked
```
```rust
#[test]
fn review_get_field_union_parent_nulls() -> Result<()> {
use arrow::array::UnionArray;
use arrow::datatypes::UnionFields;
use arrow_buffer::ScalarBuffer;
let child = UnionArray::try_new(
UnionFields::try_new([0], [Field::new("value", DataType::Int32, true)])?,
ScalarBuffer::from(vec![0_i8, 0, 0]),
None,
vec![Arc::new(Int32Array::from(vec![1, 2, 3]))],
)?;
let parent = StructArray::new(
vec![Field::new("u", child.data_type().clone(), true)].into(),
vec![Arc::new(child)],
Some(NullBuffer::from(vec![true, false, true])),
);
let func = GetFieldFunc::new();
let name = ScalarValue::Utf8(Some("u".to_string()));
let arg_fields = vec![
Arc::new(Field::new("s", parent.data_type().clone(), true)),
Arc::new(Field::new("name", DataType::Utf8, false)),
];
let return_field = func.return_field_from_args(ReturnFieldArgs {
arg_fields: &arg_fields,
scalar_arguments: &[None, Some(&name)],
})?;
let result = func.invoke_with_args(ScalarFunctionArgs {
args: vec![ColumnarValue::Array(Arc::new(parent)), ColumnarValue::Scalar(name)],
arg_fields,
number_rows: 3,
return_field,
config_options: Default::default(),
})?.into_array(3)?;
assert_eq!(result.logical_nulls(), Some(NullBuffer::from(vec![true, false, true])));
Ok(())
}
```
The call errors before reaching the assertion.
</details>
--
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