fix: array_position nested lists handling & check index for nulls

[Jefffrey]

Which issue does this PR close?

• Closes array_position doesn't check nulls in array index & fails to handle nulls properly #21792

Rationale for this change

Index nulls

When index is a scalar, we ensure it can't be null:

datafusion/datafusion/functions-nested/src/position.rs

Lines 209 to 214 in 766dff1

	Some(ColumnarValue::Scalar(ScalarValue::Int64(Some(v)))) => {
	Ok(vec![v - 1; num_rows])
	}
	Some(ColumnarValue::Scalar(s)) => {
	exec_err!("array_position expected Int64 for start_from, got {s}")
	}

• This matches with postgres which also errors if index is null

postgres=# select array_position(ARRAY['sun', 'mon', 'tue', 'wed', 'thu', 'fri', 'sat'], 'mon', null);
ERROR:  initial position must not be null

However we don't guard nulls when it is an array:

datafusion/datafusion/functions-nested/src/position.rs

Lines 215 to 217 in 766dff1

	Some(ColumnarValue::Array(a)) => {
	Ok(as_int64_array(a)?.values().iter().map(|&x| x - 1).collect())
	}

datafusion/datafusion/functions-nested/src/position.rs

Lines 308 to 314 in 766dff1

	let arr_from = if args.len() == 3 {
	as_int64_array(&args[2])?
	.values()
	.iter()
	.map(|&x| x - 1)
	.collect::<Vec<_>>()
	} else {

• Accessing values() directly, ignoring parent null buffer

Null handling

array_position relies on compare_element_to_list:

datafusion/datafusion/functions-nested/src/utils.rs

Lines 150 to 221 in 766dff1

	pub(crate) fn compare_element_to_list(
	list_array_row: &dyn Array,
	element_array: &dyn Array,
	row_index: usize,
	eq: bool,
	) -> Result<BooleanArray> {
	if list_array_row.data_type() != element_array.data_type() {
	return exec_err!(
	"compare_element_to_list received incompatible types: '{:?}' and '{:?}'.",
	list_array_row.data_type(),
	element_array.data_type()
	);
	}
	let element_array_row = element_array.slice(row_index, 1);
	// Compute all positions in list_row_array (that is itself an
	// array) that are equal to `from_array_row`
	let res = match element_array_row.data_type() {
	// arrow_ord::cmp::eq does not support ListArray, so we need to compare it by loop
	DataType::List(_) => {
	// compare each element of the from array
	let element_array_row_inner = as_list_array(&element_array_row)?.value(0);
	let list_array_row_inner = as_list_array(list_array_row)?;
	list_array_row_inner
	.iter()
	// compare element by element the current row of list_array
	.map(|row| {
	row.map(|row| {
	if eq {
	row.eq(&element_array_row_inner)
	} else {
	row.ne(&element_array_row_inner)
	}
	})
	})
	.collect::<BooleanArray>()
	}
	DataType::LargeList(_) => {
	// compare each element of the from array
	let element_array_row_inner =
	as_large_list_array(&element_array_row)?.value(0);
	let list_array_row_inner = as_large_list_array(list_array_row)?;
	list_array_row_inner
	.iter()
	// compare element by element the current row of list_array
	.map(|row| {
	row.map(|row| {
	if eq {
	row.eq(&element_array_row_inner)
	} else {
	row.ne(&element_array_row_inner)
	}
	})
	})
	.collect::<BooleanArray>()
	}
	_ => {
	let element_arr = Scalar::new(element_array_row);
	// use not_distinct so we can compare NULL
	if eq {
	arrow_ord::cmp::not_distinct(&list_array_row, &element_arr)?
	} else {
	arrow_ord::cmp::distinct(&list_array_row, &element_arr)?
	}
	}
	};
	Ok(res)
	}

When the type to be checked is not a list, we pass through to arrow not_distinct kernel which handles null accordingly. However, we roll our own verification for list types, which critically does not account for null in the needle:

datafusion/datafusion/functions-nested/src/utils.rs

Lines 164 to 187 in 766dff1

	let element_array_row = element_array.slice(row_index, 1);
	// Compute all positions in list_row_array (that is itself an
	// array) that are equal to `from_array_row`
	let res = match element_array_row.data_type() {
	// arrow_ord::cmp::eq does not support ListArray, so we need to compare it by loop
	DataType::List(_) => {
	// compare each element of the from array
	let element_array_row_inner = as_list_array(&element_array_row)?.value(0);
	let list_array_row_inner = as_list_array(list_array_row)?;
	list_array_row_inner
	.iter()
	// compare element by element the current row of list_array
	.map(|row| {
	row.map(|row| {
	if eq {
	row.eq(&element_array_row_inner)
	} else {
	row.ne(&element_array_row_inner)
	}
	})
	})
	.collect::<BooleanArray>()

• element_array_row_inner can be null, but we never check this and assume it is valid

What changes are included in this PR?

Fix handling of index when it is an array to error fast if we detect any nulls.

Revamp compare_element_to_list into a new version which handles nulls properly by using make_comparator from arrow.

Are these changes tested?

Yes.

Are there any user-facing changes?

Corrected behaviour for null handling of array_position[s]

[Jefffrey]

Technically this was checked by the below arm, but I added this as a more specific error message

[Jefffrey]

I added a new version as I didn't want to affect array_remove/array_replace yet; I'll tackle them in followups

(this is why I also omitted the eq parameter, as for now this is only used by position anyway)

[Jefffrey]

I figured it would be a better idea to not do this datatype check inside, as this function is in the hotloop (called for every row), so pulled it into a const generic

[Jefffrey]

Main fix here, using comparator now which handles nulls on both sides properly (null = null is true)

[Jefffrey]

Some of these might overlap, but I wanted to get a comprehensive overview of the behaviours in a single test case; also this ensures we run the array path instead of just the scalar path

[codecov-commenter]

Codecov Report

❌ Patch coverage is 88.09524% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.66%. Comparing base (e8a65f2) to head (adf1eb2).

Files with missing lines	Patch %	Lines
datafusion/functions-nested/src/position.rs	88.23%	2 Missing and 16 partials ⚠️
datafusion/functions-nested/src/utils.rs	86.66%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #21791      +/-   ##
==========================================
- Coverage   80.66%   80.66%   -0.01%     
==========================================
  Files        1095     1095              
  Lines      372294   372447     +153     
  Branches   372294   372447     +153     
==========================================
+ Hits       300324   300446     +122     
- Misses      54055    54074      +19     
- Partials    17915    17927      +12     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.