-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 length error with array_has
#12459
Conversation
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.
@alamb I would love to get this merged, it's causing many queries that use ANY()
to fail right now.
cc @jayzhan211 who I think wrote this.
@@ -200,7 +201,10 @@ fn array_has_dispatch_for_scalar<O: OffsetSizeTrait>( | |||
// If first argument is empty list (second argument is non-null), return false | |||
// i.e. array_has([], non-null element) -> false | |||
if values.len() == 0 { | |||
return Ok(Arc::new(BooleanArray::from(vec![Some(false)]))); | |||
return Ok(Arc::new(BooleanArray::new( | |||
BooleanBuffer::new_unset(haystack.len()), |
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.
the point is that in this case the array returned should be the same length as the haystack, not 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.
Thank you for this fix @samuelcolvin -- I have one small nit pick about where the test should live, but we can also fix that as a follow on PR.
7123862
to
b59daed
Compare
b59daed
to
9c4b6c4
Compare
80906e8
to
16a6006
Compare
slt test updated and dedicated test removed. It would be great to get this merged. We'll run datafusion main until there's a another release since this is is affecting a lot of users as we migrate querying from timescale to datafusion. |
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.
Thanks @jayzhan211 and @samuelcolvin
# test empty arrays return length | ||
# issue: https://github.com/apache/datafusion/pull/12459 | ||
statement ok | ||
create table values_all_empty (a int[]) as values ([]), ([]); |
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.
beautiful 🎉
Thanks @alamb for merging so quickly. 🙏 |
I try hard to prioritize bug fixes. Thank you for fixing the bug! |
Rationale for this change
Fixes an error we've been experiencing in prod where some queries that use
array_has
(including viaANY()
) result int:What changes are included in this PR?
Fix a bug in
array_has
where it returned 1 row, instead of the size of the group.Are these changes tested?
Yes.
Are there any user-facing changes?
No.