Skip to content
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

Merged
merged 3 commits into from
Jan 27, 2024
Merged

Conversation

RinChanNOWWW
Copy link
Contributor

@RinChanNOWWW RinChanNOWWW commented Jan 26, 2024

Which issue does this PR close?

Closes #4958.

Rationale for this change

Use take kernel to take RecordBatch.

What changes are included in this PR?

Add a new kernel function take_record_batch.

Are there any user-facing changes?

@github-actions github-actions bot added the arrow Changes to the arrow crate label Jan 26, 2024
Copy link
Contributor

@alamb alamb left a 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

@RinChanNOWWW
Copy link
Contributor Author

How about make the function like:

pub fn take_record_batch<T: ArrowPrimitiveType>(
    record_batch: &RecordBatch,
    indices: &PrimitiveArray<T>,
) -> Result<RecordBatch, ArrowError> {}

@RinChanNOWWW
Copy link
Contributor Author

Is there a trait in arrow-rs that indicates a integer type?

@alamb
Copy link
Contributor

alamb commented Jan 27, 2024

Is there a trait in arrow-rs that indicates a integer type?

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

Copy link
Contributor

@alamb alamb left a 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 🙏

Copy link
Contributor

@tustvold tustvold left a 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.

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @RinChanNOWWW

@alamb alamb merged commit 5117b38 into apache:master Jan 27, 2024
25 checks passed
@RinChanNOWWW RinChanNOWWW deleted the take-record-batch branch January 28, 2024 00:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add take_record_batch kernel
3 participants