fix: array_any_value returns NULL for empty list elements#23775
Open
bjchambers wants to merge 1 commit into
Open
fix: array_any_value returns NULL for empty list elements#23775bjchambers wants to merge 1 commit into
bjchambers wants to merge 1 commit into
Conversation
…ements general_array_any_value handled null and all-null list elements, but a non-null *empty* (length-0) element fell through to the no-nulls branch, which unconditionally read values[start]. That returned the next element's value for an interior empty list (silently wrong data), and read out of bounds when start == values.len() (a trailing empty element), panicking with "range end index N out of range for slice of length N-1". The panic surfaced when the array_any_value output flowed into a hash RepartitionExec (e.g. used as an equi-join key): batches got sliced so an empty element landed at the end of a values buffer, tripping the out-of-bounds read on a spawned task. Guard the empty case explicitly: an empty list has no value to take, so the result is NULL. Sibling functions in this file are already safe (array_element bounds-checks the index against len; array_slice / pop_front / pop_back guard len == 0). Regression tests added at the kernel level (interior + trailing empty) and as sqllogictest cases. Signed-off-by: Ben Chambers <bchambers@apache.org>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ 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 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
neilconway
reviewed
Jul 22, 2026
neilconway
left a comment
Contributor
There was a problem hiding this comment.
Overall looks good! Nicely targeted fix, good test coverage.
Can you update the comment that describes array_any_value to document that it returns NULL for empty arrays? (~extract.rs:84).
Comment on lines
+1250
to
+1251
| // the next element's value. Previously the no-nulls branch unconditionally | ||
| // read values[start], returning the wrong element for interior empty lists. |
Contributor
There was a problem hiding this 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).
| 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`) |
Contributor
There was a problem hiding this comment.
Remove "(previously read 2)"
Comment on lines
+1271
to
+1272
| // 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. |
Contributor
There was a problem hiding this comment.
Remove the description of previous behavior.
Comment on lines
+150
to
+151
| # whose start offset equals the values length (previously read out of bounds | ||
| # and panicked the worker task). |
Contributor
There was a problem hiding this comment.
Remove "(previously ...)"
Comment on lines
+1074
to
+1076
| // 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). |
Contributor
There was a problem hiding this comment.
Remove "Without this..."
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
array_any_valuereads the wrong value (or panics) when its input list columncontains a non-null empty (length-0) element.
general_array_any_valueguards null and all-null elements, but a non-nullempty element falls through to the no-nulls branch, which unconditionally reads
values[start]:values[start], i.e. the next element'svalue (silently wrong data)
start == values.len()) → out-of-bounds slice →panic
range end index N out of range for slice of length N-1The panic is easy to trigger in practice when the
array_any_valueoutput flowsinto 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 hasno value to take, so the result is
NULL.Sibling functions in
extract.rswere audited and are already safe:array_elementbounds-checks the index againstlen;array_slice/array_pop_front/array_pop_backguardlen == 0.Are these changes tested?
Yes:
general_array_any_value: an interior emptyelement (previously wrong value) and a trailing empty element (previously
panic).
array_any_value.sltcases coveringListandLargeListwith interior andtrailing empty elements.
Are there any user-facing changes?
array_any_valuenow returnsNULLfor an empty list element instead ofreturning the next element's value or panicking the query. No API changes.