-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix: graceful NULL and type error handling in array functions #14737
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: graceful NULL and type error handling in array functions #14737
Conversation
a108022 to
3f07b44
Compare
|
the tests failed because the error message didn’t match. It looks like the pipeline outputs more than we expected. I temporarily removed the error message check in |
|
Marking as draft as I think this PR is no longer waiting on feedback. Please mark it as ready for review when it is ready for another look |
| for i in 0..row_count { | ||
| if list_array.is_null(i) { | ||
| array_lengths.push(0); | ||
| arrays.push(new_null_array(list_array.value(i).data_type(), 1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this line fix the issue below.
for null case, you just need to make sure the offset append a single 0 and the null buffer append a null, that is why we array_lengths.push(0) and append null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, thank you so much! I think I did it wrong when dealing with array_sort(NULL). I've pushed a new version, please take a look to check if that's the correct implementation.
jayzhan211
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏻
|
@jayzhan211 Thank you so much for all the help and guidance! |
|
Thanks @alan910127 |
…#14737) * feat: arbitrary typed argument in array function * fix: array_sort null handling * fix: array_resize signature * test: add array_sort sqllogictest for null and invalid types * fix: don't match error message * chore: use string instead of data type * refactor: use new_null_array * fix: pass null to array argument should return null * fix: handle null argument for array in replace and resize * fix: mismatched error message * fix: incorrect number of rows returned * test: update null tests * fix: treat NULLs as lists directly to prevent extra handling * fix: incorrect null pushing in array_sort
…#14737) * feat: arbitrary typed argument in array function * fix: array_sort null handling * fix: array_resize signature * test: add array_sort sqllogictest for null and invalid types * fix: don't match error message * chore: use string instead of data type * refactor: use new_null_array * fix: pass null to array argument should return null * fix: handle null argument for array in replace and resize * fix: mismatched error message * fix: incorrect number of rows returned * test: update null tests * fix: treat NULLs as lists directly to prevent extra handling * fix: incorrect null pushing in array_sort
Which issue does this PR close?
Rationale for this change
This change ensures that array functions handle
NULLand incorrect argument types gracefully, preventing unexpected failures or internal errors showing up to the user.What changes are included in this PR?
Added
ArrayFunctionArgument::DataType(data_type)variant for array functions to define an argument of a specific type.array_sortdescornulls_firstis NULL.Utf8NULL inputs:
Incorrect type of inputs:
N/A
array_replace,array_replace_n,array_replace_allNULL inputs:
incorrect type of inputs:
array_resizesizeis NULL, returns an empty array. Ifvalueis NULL, fill the new slots to be NULL.NULL inputs:
incorrect type of inputs:
Are these changes tested?
Tests for
array_sortwith NULL and incorrect inputs are added inarray.slt. Other functions are already tested viaarrays_valuesinsqllogictest. If additional tests are needed, please let me know.Are there any user-facing changes?
No.