refactor: simplify calculate_binary_math in datafusion-functions
#18525
Conversation
| { | ||
| Ok(match right { | ||
| let left = left.as_primitive::<L>(); | ||
| let right = right.cast_to(&R::DATA_TYPE, None)?; |
There was a problem hiding this comment.
This right cast wasn't applied if it was a scalar, so cast the columnarvalue itself to align both arms
There was a problem hiding this comment.
I wonder why we aren't also casting left as well up here (it was only done in the branch below 🤔 )
| let right_array = right_casted.as_primitive::<R>(); | ||
|
|
||
| // Types are compatible even they are decimals with different scale or precision | ||
| let result = if PrimitiveArray::<L>::is_compatible(&L::DATA_TYPE) { |
There was a problem hiding this comment.
This is_compatible check would always be true since we're essentially comparing L generic with itself; this seemed like a mistake so I'm removing it
There was a problem hiding this comment.
The documentation of https://docs.rs/arrow/latest/arrow/array/struct.PrimitiveArray.html#method.is_compatible says
This is equivalent to data_type == T::DATA_TYPE, however ignores timestamp timezones and decimal precision and scale
So maybe it is something related to timezones?
There was a problem hiding this comment.
It's possible it was meant to check compatibility of the left array with L generic type itself; but given no tests failed I'm not sure if the branch would be reached anyway 🤔
There was a problem hiding this comment.
The change looks ok. Seem like I intended to check extra casting to the right array's type, but placed an always-true check. Given we tend to use coercing (like in #18032) instead of specifying exact types, it wouldn't be required.
alamb
left a comment
There was a problem hiding this comment.
The code looks better to me and all the tests are passing so I think it is good to go from my perspective 👍
| let right_array = right_casted.as_primitive::<R>(); | ||
|
|
||
| // Types are compatible even they are decimals with different scale or precision | ||
| let result = if PrimitiveArray::<L>::is_compatible(&L::DATA_TYPE) { |
There was a problem hiding this comment.
The documentation of https://docs.rs/arrow/latest/arrow/array/struct.PrimitiveArray.html#method.is_compatible says
This is equivalent to data_type == T::DATA_TYPE, however ignores timestamp timezones and decimal precision and scale
So maybe it is something related to timezones?
| { | ||
| Ok(match right { | ||
| let left = left.as_primitive::<L>(); | ||
| let right = right.cast_to(&R::DATA_TYPE, None)?; |
There was a problem hiding this comment.
I wonder why we aren't also casting left as well up here (it was only done in the branch below 🤔 )
…pache#18525) Ensure the `right` value gets casted if it is a scalar (currently we only cast if it is an array). Remove unused arm for casting left value.
…pache#18525) Ensure the `right` value gets casted if it is a scalar (currently we only cast if it is an array). Remove unused arm for casting left value.
Ensure the
rightvalue gets casted if it is a scalar (currently we only cast if it is an array).Remove unused arm for casting left value.