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:
|
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
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.