You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "bjchambers (via GitHub)" <gi...@apache.org> on 2026/07/21 20:13:26 UTC
[PR] fix: array_any_value returns NULL for empty list elements [datafusion]
bjchambers opened a new pull request, #23775:
URL: https://github.com/apache/datafusion/pull/23775
## Which issue does this PR close?
- Closes #23773.
## Rationale for this change
`array_any_value` reads the wrong value (or panics) when its input list column
contains a non-null **empty** (length-0) element.
`general_array_any_value` guards null and all-null elements, but a non-null
empty element falls through to the no-nulls branch, which unconditionally reads
`values[start]`:
- **interior empty list** → reads `values[start]`, i.e. the *next* element's
value (silently wrong data)
- **trailing empty list** (`start == values.len()`) → out-of-bounds slice →
panic `range end index N out of range for slice of length N-1`
The panic is easy to trigger in practice when the `array_any_value` output flows
into a hash `RepartitionExec` (e.g. the value is used as an equi-join key):
repartitioning slices batches so an empty element can land at the end of a
values buffer, tripping the out-of-bounds read on a spawned task.
## What changes are included in this PR?
Guard the empty case explicitly in `general_array_any_value` — an empty list has
no value to take, so the result is `NULL`.
Sibling functions in `extract.rs` were audited and are already safe:
`array_element` bounds-checks the index against `len`; `array_slice` /
`array_pop_front` / `array_pop_back` guard `len == 0`.
## Are these changes tested?
Yes:
- Kernel-level regression tests for `general_array_any_value`: an interior empty
element (previously wrong value) and a trailing empty element (previously
panic).
- `array_any_value.slt` cases covering `List` and `LargeList` with interior and
trailing empty elements.
## Are there any user-facing changes?
`array_any_value` now returns `NULL` for an empty list element instead of
returning the next element's value or panicking the query. No API changes.
--
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: array_any_value returns NULL for empty list elements [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway merged PR #23775:
URL: https://github.com/apache/datafusion/pull/23775
--
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: array_any_value returns NULL for empty list elements [datafusion]
Posted by "bjchambers (via GitHub)" <gi...@apache.org>.
bjchambers commented on PR #23775:
URL: https://github.com/apache/datafusion/pull/23775#issuecomment-5052350728
> Overall looks good! Nicely targeted fix, good test coverage.
Thanks for the quick review!
> Can you update the comment that describes `array_any_value` to document that it returns NULL for empty arrays? (~`extract.rs:84`).
This and other comments have been addressed.
--
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: array_any_value returns NULL for empty list elements [datafusion]
Posted by "bjchambers (via GitHub)" <gi...@apache.org>.
bjchambers commented on code in PR #23775:
URL: https://github.com/apache/datafusion/pull/23775#discussion_r3641021705
##########
datafusion/functions-nested/src/extract.rs:
##########
@@ -970,7 +970,7 @@ where
#[user_doc(
doc_section(label = "Array Functions"),
- description = "Returns the first non-null element in the array.",
+ description = "Returns the first non-null element in the array. Returns NULL if the array is empty or all elements are NULL.",
Review Comment:
Done. I was thinking it was somewhat worthwhile since it's not necessarily obvious what "Returns the first non-null element in the array." does if there are no non-null elements in the array. But, agreed that `NULL` is the most likely candidate.
--
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: array_any_value returns NULL for empty list elements [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on code in PR #23775:
URL: https://github.com/apache/datafusion/pull/23775#discussion_r3638988089
##########
datafusion/functions-nested/src/extract.rs:
##########
@@ -970,7 +970,7 @@ where
#[user_doc(
doc_section(label = "Array Functions"),
- description = "Returns the first non-null element in the array.",
+ description = "Returns the first non-null element in the array. Returns NULL if the array is empty or all elements are NULL.",
Review Comment:
One more minor nit:
```suggestion
description = "Returns the first non-null element in the array. Returns NULL if the array is empty or NULL.",
```
i.e., if first or more elements of the array is NULL, we'd expect a NULL return value; we only need to document the corner-case 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: array_any_value returns NULL for empty list elements [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #23775:
URL: https://github.com/apache/datafusion/pull/23775#discussion_r3635010472
##########
datafusion/functions-nested/src/extract.rs:
##########
@@ -1236,6 +1244,47 @@ mod tests {
Ok(())
}
+ // An empty (length-0) list element that is not null must yield NULL, not
+ // the next element's value.
+ #[test]
+ fn test_array_any_value_empty_list_element() -> Result<()> {
Review Comment:
we can omit these unit tests if the SLTs cover the same 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: array_any_value returns NULL for empty list elements [datafusion]
Posted by "bjchambers (via GitHub)" <gi...@apache.org>.
bjchambers commented on code in PR #23775:
URL: https://github.com/apache/datafusion/pull/23775#discussion_r3635185540
##########
datafusion/functions-nested/src/extract.rs:
##########
@@ -1236,6 +1244,47 @@ mod tests {
Ok(())
}
+ // An empty (length-0) list element that is not null must yield NULL, not
+ // the next element's value.
+ #[test]
+ fn test_array_any_value_empty_list_element() -> Result<()> {
Review Comment:
I think they cover similar cases. The only difference (which may be worth keeping) is that these control the null layout more explicitly, while the SLT relies on `cast(make_array() as bigint[])` to make the empty array. Thoughts on if that is a good reason to keep them?
--
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: array_any_value returns NULL for empty list elements [datafusion]
Posted by "bjchambers (via GitHub)" <gi...@apache.org>.
bjchambers commented on code in PR #23775:
URL: https://github.com/apache/datafusion/pull/23775#discussion_r3635721830
##########
datafusion/functions-nested/src/extract.rs:
##########
@@ -1236,6 +1244,47 @@ mod tests {
Ok(())
}
+ // An empty (length-0) list element that is not null must yield NULL, not
+ // the next element's value.
+ #[test]
+ fn test_array_any_value_empty_list_element() -> Result<()> {
Review Comment:
removed
--
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: array_any_value returns NULL for empty list elements [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on code in PR #23775:
URL: https://github.com/apache/datafusion/pull/23775#discussion_r3632859389
##########
datafusion/functions-nested/src/extract.rs:
##########
@@ -1236,6 +1246,49 @@ mod tests {
Ok(())
}
+ // An empty (length-0) list element that is not null must yield NULL, not
+ // the next element's value. Previously the no-nulls branch unconditionally
+ // read values[start], returning the wrong element for interior empty lists.
+ #[test]
+ fn test_array_any_value_empty_list_element() -> Result<()> {
+ let values: ArrayRef = Arc::new(Int32Array::from(vec![1, 2, 3]));
+ // row 0 = [1], row 1 = [] (empty, non-null), row 2 = [2, 3]
+ let offsets = OffsetBuffer::new(ScalarBuffer::from(vec![0, 1, 1, 3]));
+ let field = Arc::new(Field::new("item", DataType::Int32, true));
+
+ let list_array = ListArray::new(field, offsets, values, None);
+
+ let result = general_array_any_value(&list_array)?;
+ let result = result.as_any().downcast_ref::<Int32Array>().unwrap();
+
+ assert_eq!(result.value(0), 1);
+ assert!(result.is_null(1)); // empty list -> NULL (previously read `2`)
Review Comment:
Remove "(previously read `2`)"
##########
datafusion/functions-nested/src/extract.rs:
##########
@@ -1236,6 +1246,49 @@ mod tests {
Ok(())
}
+ // An empty (length-0) list element that is not null must yield NULL, not
+ // the next element's value. Previously the no-nulls branch unconditionally
+ // read values[start], returning the wrong element for interior empty lists.
Review Comment:
Nit: I think comments should describe the current state of the code (which is what future readers will care about), rather than narrate how the code has evolved over time (that belongs in `git` commit comments).
##########
datafusion/functions-nested/src/extract.rs:
##########
@@ -1236,6 +1246,49 @@ mod tests {
Ok(())
}
+ // An empty (length-0) list element that is not null must yield NULL, not
+ // the next element's value. Previously the no-nulls branch unconditionally
+ // read values[start], returning the wrong element for interior empty lists.
+ #[test]
+ fn test_array_any_value_empty_list_element() -> Result<()> {
+ let values: ArrayRef = Arc::new(Int32Array::from(vec![1, 2, 3]));
+ // row 0 = [1], row 1 = [] (empty, non-null), row 2 = [2, 3]
+ let offsets = OffsetBuffer::new(ScalarBuffer::from(vec![0, 1, 1, 3]));
+ let field = Arc::new(Field::new("item", DataType::Int32, true));
+
+ let list_array = ListArray::new(field, offsets, values, None);
+
+ let result = general_array_any_value(&list_array)?;
+ let result = result.as_any().downcast_ref::<Int32Array>().unwrap();
+
+ assert_eq!(result.value(0), 1);
+ assert!(result.is_null(1)); // empty list -> NULL (previously read `2`)
+ assert_eq!(result.value(2), 2);
+
+ Ok(())
+ }
+
+ // A trailing empty list element has start == values.len(); the old code did
+ // `extend(0, start, start + 1)` and panicked with an out-of-bounds slice.
Review Comment:
Remove the description of previous behavior.
##########
datafusion/functions-nested/src/extract.rs:
##########
@@ -1062,13 +1062,23 @@ where
for (row_index, offset_window) in array.offsets().windows(2).enumerate() {
let start = offset_window[0];
+ let end = offset_window[1];
- // array is null
+ // the list element is null
if array.is_null(row_index) {
mutable.try_extend_nulls(1)?;
continue;
}
+ // the list element is empty; there is no value to take, so the result
+ // is NULL. Without this guard the no-nulls branch below would read
+ // `values[start]`, which is either the next element (wrong value) or
+ // out of bounds when `start == values.len()` (panic).
Review Comment:
Remove "Without this..."
##########
datafusion/sqllogictest/test_files/array/array_any_value.slt:
##########
@@ -145,6 +145,36 @@ select array_any_value(make_array(NULL, 1, 2, 3, 4, 5)), array_any_value(column1
1 41
1 51
+# array_any_value with empty (length-0) list elements
+# A non-null but empty list must yield NULL, including a trailing empty element
+# whose start offset equals the values length (previously read out of bounds
+# and panicked the worker task).
Review Comment:
Remove "(previously ...)"
--
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: array_any_value returns NULL for empty list elements [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #23775:
URL: https://github.com/apache/datafusion/pull/23775#issuecomment-5038830694
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/23775?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 `89.65517%` with `3 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 80.71%. Comparing base ([`eef1017`](https://app.codecov.io/gh/apache/datafusion/commit/eef101769d650b2b06ac2bcf298f71113a047c9d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`ed69900`](https://app.codecov.io/gh/apache/datafusion/commit/ed69900728bb0af8f8b6f3dc07904f1052817f16?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/23775?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-nested/src/extract.rs](https://app.codecov.io/gh/apache/datafusion/pull/23775?src=pr&el=tree&filepath=datafusion%2Ffunctions-nested%2Fsrc%2Fextract.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9mdW5jdGlvbnMtbmVzdGVkL3NyYy9leHRyYWN0LnJz) | 89.65% | [0 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/23775?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 #23775 +/- ##
==========================================
- Coverage 80.71% 80.71% -0.01%
==========================================
Files 1089 1089
Lines 368748 368777 +29
Branches 368748 368777 +29
==========================================
+ Hits 297633 297651 +18
- Misses 53372 53375 +3
- Partials 17743 17751 +8
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/23775?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: array_any_value returns NULL for empty list elements [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #23775:
URL: https://github.com/apache/datafusion/pull/23775#discussion_r3635606959
##########
datafusion/functions-nested/src/extract.rs:
##########
@@ -1236,6 +1244,47 @@ mod tests {
Ok(())
}
+ // An empty (length-0) list element that is not null must yield NULL, not
+ // the next element's value.
+ #[test]
+ fn test_array_any_value_empty_list_element() -> Result<()> {
Review Comment:
an empty list element will have the same structure regardless; i could see why this would matter if we were testing some edge case with a non-empty null slot, but since this is just for regular empty lists its probably not necessary
--
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