-
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
Replace macro with function for array_position
and array_positions
#8170
Conversation
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
385be3c
to
e46a627
Compare
Signed-off-by: jayzhan211 <[email protected]>
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.
This certainly seems like an improvement to me. I think we can probably make it even more efficient but doing that as a follow on PR would make a lot of sense too. Thank you @jayzhan211
cc @Veeupup in case you have some other thoughts
@@ -131,6 +131,78 @@ macro_rules! array { | |||
}}; | |||
} | |||
|
|||
/// Computes a BooleanArray indicating equality or inequality between elements in a list array and a specified element array. |
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.
❤️
eq: bool, | ||
) -> Result<BooleanArray> { | ||
let indices = UInt32Array::from(vec![row_index as u32]); | ||
let element_array_row = arrow::compute::take(element_array, &indices, None)?; |
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.
This will always be a single row Array as indices have a single value 🤔 I wonder if you could call
I think you could instead call list_array_row.values().slice()
and find the relevant row to compare against those values 🤔
.filter(|(_, x)| *x == el) | ||
.flat_map(|(i, _)| Some((i + 1) as u64)) | ||
.collect::<UInt64Array>(); | ||
fn general_position<OffsetSize: OffsetSizeTrait>( |
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.
nit: if we only use i32 as OffsetSize, do we really need the Generics?
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.
no, but we will need it if we want to extend it for large list, I'm just lazy to remove it for now :)
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.
Nice extension 👍
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 : )
Which issue does this PR close?
Part of #7988
Closes #8145
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?