-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 error on array_distinct
when input is empty #13810
#14034
Conversation
Thanks @cht42 ! What does FLUP stand for 🤔 My google fu doesn't seem to be able to find anythihg relevant: https://www.google.com/search?q=flup&oq=flup |
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.
Thank you @cht42
Can you please add some tests, ideally in sqllogicteset format?
Here are the instructions: https://github.com/apache/datafusion/tree/main/datafusion/sqllogictest
Ideally you should be able to extend one of the existing tests in datafusion/sqllogictest/test_files/array.slt
Perhaps you can follow the model of #10786
array_distinct
when input is empty #13810
Means: FoLlow UP. Maybe just used at my company internaly 😅 |
@@ -513,9 +513,6 @@ fn general_array_distinct<OffsetSize: OffsetSizeTrait>( | |||
array: &GenericListArray<OffsetSize>, | |||
field: &FieldRef, | |||
) -> Result<ArrayRef> { | |||
if array.len() == 0 { |
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.
do we really need to remove this check? 🤔 it prevents unnecessary work/allocations if nothing is in the array
added back the early check on empty arrays and added a test case |
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.
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.
lgtm thanks @cht42
Which issue does this PR close?
Closes/FLUP #13809
Rationale for this change
FLUP for #13810, this fix will handle nulls as well
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?