alamb commented on code in PR #5476:
URL: https://github.com/apache/arrow-datafusion/pull/5476#discussion_r1125523908
##########
datafusion/expr/src/type_coercion/binary.rs:
##########
@@ -173,12 +173,27 @@ fn bitwise_coercion(left_type: &DataType, right_type:
&DataType) -> Option<DataT
return Some(left_type.clone());
}
- // TODO support other data type
match (left_type, right_type) {
- (Int64, _) | (_, Int64) => Some(Int64),
- (Int32, _) | (_, Int32) => Some(Int32),
- (Int16, _) | (_, Int16) => Some(Int16),
+ (UInt64, _) | (_, UInt64) => Some(UInt64),
Review Comment:
👍 these rules look reasonable to me.
##########
datafusion/physical-expr/src/expressions/binary/kernels.rs:
##########
@@ -125,10 +140,42 @@ pub(crate) fn bitwise_shift_right(left: ArrayRef, right:
ArrayRef) -> Result<Arr
binary_bitwise_array_op!(
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()),
Review Comment:
Since this function returns a result, I think it would be better if this
function returned an error on overflow, rather than `panic`ing (aka can we
avoid calling the `unwrap`?)
##########
datafusion/physical-expr/src/expressions/binary/kernels.rs:
##########
@@ -125,10 +140,42 @@ pub(crate) fn bitwise_shift_right(left: ArrayRef, right:
ArrayRef) -> Result<Arr
binary_bitwise_array_op!(
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()),
Int64Array
)
}
+ DataType::UInt8 => {
+ binary_bitwise_array_op!(
+ left,
+ right,
+ |a: u8, b: u8| a.wrapping_shr(b as u32),
Review 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 ✅
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
