feat: Support bitwise operations for unsigned integer types #5476
Conversation
github-actions
bot
added
logical-expr
alamb
changed the title
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
This looks great @izveigor -- thank you. This PR looks really nice.
cc @liukun4515
I had a question about using unwrap
Also could you please add some "end to end" sql tests for this feature using sqllogictest ?
https://github.com/apache/arrow-datafusion/tree/main/datafusion/core/tests/sqllogictests
Specifically, added to https://github.com/apache/arrow-datafusion/blob/main/datafusion/core/tests/sqllogictests/test_files/scalar.slt?
| (Int64, _) | (_, Int64) => Some(Int64), | ||
| (Int32, _) | (_, Int32) => Some(Int32), | ||
| (Int16, _) | (_, Int16) => Some(Int16), | ||
| (UInt64, _) | (_, UInt64) => Some(UInt64), |
There was a problem hiding this comment.
👍 these rules look reasonable to me.
| left, | ||
| right, | ||
| |a: i64, b: i64| a.wrapping_shr(b as u32), | ||
| |a: i64, b: i64| a.wrapping_shr((b as u64).try_into().unwrap()), |
There was a problem hiding this comment.
Since this function returns a result, I think it would be better if this function returned an error on overflow, rather than panicing (aka can we avoid calling the unwrap?)
| binary_bitwise_array_op!( | ||
| left, | ||
| right, | ||
| |a: u8, b: u8| a.wrapping_shr(b as u32), |
There was a problem hiding this comment.
TIL that wrapping_shr for i8 does in fact take a `u32 input 🤷 It looked strange to me but I double checked https://doc.rust-lang.org/std/primitive.i8.html#method.wrapping_shr ✅
github-actions
bot
added
core
| binary_bitwise_array_op!( | ||
| left, | ||
| right, | ||
| |a: u64, b: u64| a.wrapping_shr(b.try_into().unwrap()), |
There was a problem hiding this comment.
Maybe, I don't know some aspects of programming in Rust, but I think it is impossible to return normal error through macros. Anyway, by the behavior of this function: https://github.com/apache/arrow-datafusion/blob/main/datafusion/physical-expr/src/expressions/binary/kernels.rs#L60, I consider that unwrap() in this situation is not a big problem.
There was a problem hiding this comment.
but I think it is impossible to return normal error through macros.
I think it is possible (the function the macro is used in has to return a result)
Anyway, by the behavior of this function: https://github.com/apache/arrow-datafusion/blob/main/datafusion/physical-expr/src/expressions/binary/kernels.rs#L60, I consider that unwrap() in this situation is not a big problem.
I agree the pre-existing pattern is a reasonable justification for using unwrap.
Thank you
|
Benchmark runs are scheduled for baseline = 99ef989 and contender = a6f4a77. a6f4a77 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
4 participants
Which issue does this PR close?
Closes #5475