-
Notifications
You must be signed in to change notification settings - Fork 892
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
feat: add take_record_batch
.
#5333
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.
Thank you @RinChanNOWWW -- this actually just came up internally in InfluxData today when someone was aking about how to do a record batch take
How about make the function like: pub fn take_record_batch<T: ArrowPrimitiveType>(
record_batch: &RecordBatch,
indices: &PrimitiveArray<T>,
) -> Result<RecordBatch, ArrowError> {} |
Is there a trait in |
I think https://docs.rs/arrow/latest/arrow/datatypes/trait.ArrowDictionaryKeyType.html does, but that might be confusing if it shows up in this function |
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 @RinChanNOWWW -- I have one more small suggestion, but I think this could be merged as is as well.
Thank you very much 🙏
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.
I personally would prefer for this function to take &dyn Array
to match the underlying take
kernel, but I don't feel especially strongly.
b594e30
to
2e5f3d0
Compare
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 @RinChanNOWWW
Which issue does this PR close?
Closes #4958.
Rationale for this change
Use
take
kernel to takeRecordBatch
.What changes are included in this PR?
Add a new kernel function
take_record_batch
.Are there any user-facing changes?