Skip to content

Commit e4e8f23

Browse files
authored
fix: make array null argument handling follow SQL semantics (#22508)
## Which issue does this PR close? - Closes #22507. ## Rationale for this change Some array functions were not handling null index or count arguments correctly. A null `size` or `max` value was sometimes treated like `0`, which gave wrong results like `[]` or the original array. `array_element` also depended on Arrow buffer values in null slots. This change makes these functions follow normal SQL null rules. ## What changes are included in this PR? - Make `array_resize` return `NULL` when `size` is `NULL` - Make `array_replace_n` return `NULL` when `max` is `NULL` - Make `array_remove_n` return `NULL` when `max` is `NULL` - Make `array_element` check for null indexes explicitly - Clean up `array_repeat` so null counts stay explicit in offset building - Update `array_remove_n` field nullability so planner metadata matches runtime behavior - Add regression tests for these cases - Update SQL logic test outputs for the changed null behavior ## Are these changes tested? Yes. I added regression tests for the changed Rust paths and updated the SQL logic tests. ## Are there any user-facing changes? These functions now return `NULL` for null index or count arguments instead of returning `[]`, an unchanged array, or relying on accidental behavior.
1 parent bdf8a6d commit e4e8f23

10 files changed

Lines changed: 531 additions & 146 deletions

File tree

datafusion/functions-nested/src/array_any_match.rs

Lines changed: 42 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -171,12 +171,12 @@ impl HigherOrderUDF for ArrayAnyMatch {
171171
&self,
172172
args: HigherOrderReturnFieldArgs,
173173
) -> Result<Arc<Field>> {
174-
let [ValueOrLambda::Value(list), _] =
174+
let [ValueOrLambda::Value(list), ValueOrLambda::Lambda(lambda)] =
175175
take_function_args(self.name(), args.arg_fields)?
176176
else {
177177
return plan_err!("{} expects a value as first argument", self.name());
178178
};
179-
let nullable = list.is_nullable();
179+
let nullable = list.is_nullable() || lambda.is_nullable();
180180
Ok(Arc::new(Field::new("", DataType::Boolean, nullable)))
181181
}
182182

@@ -272,14 +272,14 @@ mod tests {
272272
};
273273
use datafusion_common::{DFSchema, Result};
274274
use datafusion_expr::{
275-
Expr, col,
275+
Expr, HigherOrderReturnFieldArgs, HigherOrderUDF, ValueOrLambda, col,
276276
execution_props::ExecutionProps,
277277
expr::{HigherOrderFunction, LambdaVariable},
278278
lambda, lit,
279279
};
280280
use datafusion_physical_expr::create_physical_expr;
281281

282-
use crate::array_any_match::array_any_match_higher_order_function;
282+
use crate::array_any_match::{ArrayAnyMatch, array_any_match_higher_order_function};
283283

284284
fn run_any_match(
285285
list: impl arrow::array::Array + Clone + 'static,
@@ -413,6 +413,44 @@ mod tests {
413413
Ok(())
414414
}
415415

416+
#[test]
417+
fn test_any_match_return_field_nullability() -> Result<()> {
418+
for list_nullable in [true, false] {
419+
for lambda_nullable in [true, false] {
420+
let list = Arc::new(Field::new(
421+
"list",
422+
DataType::new_list(DataType::Int32, true),
423+
list_nullable,
424+
));
425+
let lambda =
426+
Arc::new(Field::new("predicate", DataType::Boolean, lambda_nullable));
427+
let arg_fields = [
428+
ValueOrLambda::Value(Arc::clone(&list)),
429+
ValueOrLambda::Lambda(Arc::clone(&lambda)),
430+
];
431+
let scalar_arguments = [None, None];
432+
433+
let result = ArrayAnyMatch::new().return_field_from_args(
434+
HigherOrderReturnFieldArgs {
435+
arg_fields: &arg_fields,
436+
scalar_arguments: &scalar_arguments,
437+
},
438+
)?;
439+
440+
assert_eq!(
441+
result,
442+
Arc::new(Field::new(
443+
"",
444+
DataType::Boolean,
445+
list_nullable || lambda_nullable,
446+
))
447+
);
448+
}
449+
}
450+
451+
Ok(())
452+
}
453+
416454
// Predicate must not be evaluated on elements belonging to null rows.
417455
// The 10 in the null row would satisfy x > 5, but the row result must be None.
418456
#[test]

datafusion/functions-nested/src/extract.rs

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -256,8 +256,8 @@ where
256256
let end = offset_window[1];
257257
let len = end - start;
258258

259-
// array is null
260-
if array.is_null(row_index) {
259+
// array or index is null
260+
if array.is_null(row_index) || indexes.is_null(row_index) {
261261
mutable.extend_nulls(1);
262262
continue;
263263
}
@@ -1107,7 +1107,7 @@ mod tests {
11071107
};
11081108
use arrow::array::{ListArray, RecordBatch};
11091109
use arrow::buffer::{NullBuffer, OffsetBuffer, ScalarBuffer};
1110-
use arrow::datatypes::{DataType, Field};
1110+
use arrow::datatypes::{DataType, Field, Int32Type};
11111111
use datafusion_common::{Column, DFSchema, Result, assert_batches_eq};
11121112
use datafusion_expr::expr::ScalarFunction;
11131113
use datafusion_expr::{Expr, ExprSchemable};
@@ -1198,6 +1198,26 @@ mod tests {
11981198
Ok(())
11991199
}
12001200

1201+
#[test]
1202+
fn test_array_element_null_index_with_non_zero_buffer_returns_null() -> Result<()> {
1203+
let list_array = ListArray::from_iter_primitive::<Int32Type, _, _>(vec![
1204+
Some(vec![Some(1), Some(2), Some(3)]),
1205+
Some(vec![Some(4)]),
1206+
Some(vec![Some(5)]),
1207+
]);
1208+
let indexes = Int64Array::new(
1209+
ScalarBuffer::from(vec![1, 1, 1]),
1210+
Some(NullBuffer::from(vec![true, false, true])),
1211+
);
1212+
1213+
let result = general_array_element(&list_array, &indexes)?;
1214+
let expected = Int32Array::from(vec![Some(1), None, Some(5)]);
1215+
1216+
assert_eq!(result.as_primitive::<Int32Type>(), &expected);
1217+
1218+
Ok(())
1219+
}
1220+
12011221
#[test]
12021222
fn test_array_any_null_handling() -> Result<()> {
12031223
let values: ArrayRef = Arc::new(Int32Array::from(vec![1, 2, 3, 4, 5]));

0 commit comments

Comments
 (0)