fix: make array null argument handling follow SQL semantics#22508
Open
kumarUjjawal wants to merge 3 commits into
Open
fix: make array null argument handling follow SQL semantics#22508kumarUjjawal wants to merge 3 commits into
kumarUjjawal wants to merge 3 commits into
Conversation
github-actions
Bot
added
sqllogictest
kumarUjjawal
force-pushed
the
fix/null-handling-array
branch
from
b859522 to
a07ec99
Compare
Contributor
Author
|
@neilconway Could you take a look. |
neilconway
approved these changes
May 25, 2026
Contributor
neilconway
left a comment
neilconway
left a comment
There was a problem hiding this comment.
We need an SLT test for the array_element change, I think?
Otherwise, lgtm! Nice work.
Comment on lines
+213
to
+215
| let array_field = args.arg_fields[0].as_ref().clone(); | ||
| let nullable = args.arg_fields.iter().any(|f| f.is_nullable()); | ||
| Ok(Arc::new(array_field.with_nullable(nullable))) |
Contributor
There was a problem hiding this comment.
Seems like we want the same change to return_field_from_args for array_remove and array_remove_all? Looking around, it seems like array_any_match has a similar bug. Can you fix these, either as part of this PR or in a separate PR?
Comment on lines
+524
to
+525
| for element_nullability in [true, false] { | ||
| for count_nullability in [true, false] { |
Contributor
There was a problem hiding this comment.
You could potentially clean up the deep nesting with iproduct!:
for (array_nullable, item_nullable, element_nullable, count_nullable) in
iproduct!(bools, bools, bools, bools)
But feel free to ignore if you think that's too clever.
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
2 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
Some array functions were not handling null index or count arguments correctly.
A null
sizeormaxvalue was sometimes treated like0, which gave wrong results like[]or the original array.array_elementalso 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?
array_resizereturnNULLwhensizeisNULLarray_replace_nreturnNULLwhenmaxisNULLarray_remove_nreturnNULLwhenmaxisNULLarray_elementcheck for null indexes explicitlyarray_repeatso null counts stay explicit in offset buildingarray_remove_nfield nullability so planner metadata matches runtime behaviorAre 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
NULLfor null index or count arguments instead of returning[], an unchanged array, or relying on accidental behavior.