Replace macro with function for array_position and array_positions#8170
Replace macro with function for array_position and array_positions#8170alamb merged 7 commits intoapache:mainfrom
array_position and array_positions#8170Conversation
github-actions
Bot
added
physical-expr
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
jayzhan211
force-pushed
the
array-position-revisit
branch
from
385be3c to
e46a627
Compare
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
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
| }}; | ||
| } | ||
|
|
||
| /// Computes a BooleanArray indicating equality or inequality between elements in a list array and a specified element array. |
| 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.
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.
nit: if we only use i32 as OffsetSize, do we really need the Generics?
There was a problem hiding this comment.
no, but we will need it if we want to extend it for large list, I'm just lazy to remove it for now :)
4 participants
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?