-
Notifications
You must be signed in to change notification settings - Fork 4k
ARROW-10836: [Rust] Extend take kernel to FixedSizeListArray #8862
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
Conversation
Signed-off-by: wayne <i@waynest.com>
|
Hi @alamb @jorgecarleitao, could you please take a look at this:wink:. Thanks! The purpose of this pr is to support |
jorgecarleitao
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.
LGTM. I left a small comment to improve coverage, but the implementation looks really good!
| Some(vec![Some(3), Some(4), Some(5)]), | ||
| Some(vec![Some(6), Some(7), Some(8)]), | ||
| ], | ||
| vec![2, 1, 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.
would it be possible / useful to add a test that uses some null indices?
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.
Really appreciate your review!
The third case used null indices (2 in [3, 2, 1, 2, 0]) if you mean to null value inside a list.
BTW I also changed the first case to include some nulls in sub-array in the following commit.
Signed-off-by: wayne <i@waynest.com>
|
It looks like CI jammed and the coverage report doesn't show up so I ran one on my machine. The report is posted below. coverage report
|
alamb
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.
Looks good to me -- I reviewed the test cases carefully and they looked good to me. Thanks @waynexia
This PR add supports to
take()onFixedSizeListArrayby implementing following two methods:take_value_indices_from_fixed_size_list(), which calculates indeces of child array fortake(). Andtake_fixed_size_list(), which constructs result list array.And removed a dynamic cast inside
take_value_indices_from_list()by the way.