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: [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]
