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

[EPIC] Support bitwise op in arrow-rs #2702

Closed
5 of 6 tasks
liukun4515 opened this issue Sep 12, 2022 · 7 comments
Closed
5 of 6 tasks

[EPIC] Support bitwise op in arrow-rs #2702

liukun4515 opened this issue Sep 12, 2022 · 7 comments
Assignees
Labels
arrow Changes to the arrow crate enhancement Any new improvement worthy of a entry in the changelog

Comments

@liukun4515
Copy link
Contributor

liukun4515 commented Sep 12, 2022

Is your feature request related to a problem or challenge? Please describe what you are trying to do.

from the issue about bitwise op in the datafusion, we should migration the bitwise op to the arrow-rs

Describe the solution you'd like

Describe alternatives you've considered

Additional context

@liukun4515 liukun4515 added the enhancement Any new improvement worthy of a entry in the changelog label Sep 12, 2022
@liukun4515 liukun4515 self-assigned this Sep 12, 2022
@alamb
Copy link
Contributor

alamb commented Sep 12, 2022

I think moving these kernels to arrow is a great idea -- thank you @liukun4515 -- and likely the experts in arrow-rs will be able to offer some suggestions on how to improve the performance

@liukun4515
Copy link
Contributor Author

liukun4515 commented Sep 16, 2022

@alamb @tustvold

When I implement the left/right shift operation for the bitwise, I meet an issue.

In the rust, the operation << or shl will be overflow which don't like the bitand, bitor.
The warpping_shl just support wrapping_shl(self, u32) -> Self, the wrapping_shl for generic type is feature in the rust rust-lang/rust#90080
Do you have any thoughts about that?

Can we add a new trait like ArrowNativeTypeOp to support the shl_wrapping(self, u32) -> Self ? From that, the right type must be u32 until the feature becomes stable rust-lang/rust#90080

@tustvold
Copy link
Contributor

The issue you've linked relates to const versions of the functions, looking at the docs wrapping_shl seems to be defined for all the primitive types - e.g. https://doc.rust-lang.org/stable/std/primitive.i64.html#method.wrapping_shl

@liukun4515
Copy link
Contributor Author

The issue you've linked relates to const versions of the functions, looking at the docs wrapping_shl seems to be defined for all the primitive types - e.g. https://doc.rust-lang.org/stable/std/primitive.i64.html#method.wrapping_shl

But the rhs type only be u32 for any primitive typs.

My point is the api of https://doc.rust-lang.org/stable/src/core/num/wrapping.rs.html#87.

Do you means that we can implement all data type with converting the rhs value to the type of u32?

@tustvold
Copy link
Contributor

Tbh I'm not sure I fully understand the use-case for left or right shifting based on anything but a scalar, what does C++ do?

@alamb
Copy link
Contributor

alamb commented Sep 17, 2022

Tbh I'm not sure I fully understand the use-case for left or right shifting based on anything but a scalar, what does C++ do?

I think the biggest argument in my mind is for completeness -- I can cook up some examples when one might use it but they would definitely be contrived.

@alamb
Copy link
Contributor

alamb commented Sep 17, 2022

I don't have any thoughts or opinions on the shift implementation -- I defer to @tustvold

@alamb alamb added the arrow Changes to the arrow crate label Apr 24, 2023
@alamb alamb changed the title Support bitwise op in arrow-rs [EPIC] Support bitwise op in arrow-rs Apr 24, 2023
@tustvold tustvold closed this as completed Jun 1, 2023
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 enhancement Any new improvement worthy of a entry in the changelog
Projects
None yet
Development

No branches or pull requests

3 participants