You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "Jefffrey (via GitHub)" <gi...@apache.org> on 2025/11/07 06:52:30 UTC
[PR] refactor: simplify `calculate_binary_math` in datafusion-functions [datafusion]
Jefffrey opened a new pull request, #18525:
URL: https://github.com/apache/datafusion/pull/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.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: simplify `calculate_binary_math` in datafusion-functions [datafusion]
Posted by "theirix (via GitHub)" <gi...@apache.org>.
theirix commented on code in PR #18525:
URL: https://github.com/apache/datafusion/pull/18525#discussion_r2507712070
##########
datafusion/functions/src/utils.rs:
##########
@@ -140,38 +140,25 @@ where
F: Fn(L::Native, R::Native) -> Result<O::Native, ArrowError>,
R::Native: TryFrom<ScalarValue>,
{
- Ok(match right {
+ let left = left.as_primitive::<L>();
+ let right = right.cast_to(&R::DATA_TYPE, None)?;
+ let result = match right {
ColumnarValue::Scalar(scalar) => {
- let right_value: R::Native =
- R::Native::try_from(scalar.clone()).map_err(|_| {
- DataFusionError::NotImplemented(format!(
- "Cannot convert scalar value {} to {}",
- &scalar,
- R::DATA_TYPE
- ))
- })?;
- let left_array = left.as_primitive::<L>();
- // Bind right value
- let result =
- left_array.try_unary::<_, O, _>(|lvalue| fun(lvalue, right_value))?;
- Arc::new(result) as _
+ let right = R::Native::try_from(scalar.clone()).map_err(|_| {
+ DataFusionError::NotImplemented(format!(
+ "Cannot convert scalar value {} to {}",
+ &scalar,
+ R::DATA_TYPE
+ ))
+ })?;
+ left.try_unary::<_, O, _>(|lvalue| fun(lvalue, right))?
}
ColumnarValue::Array(right) => {
- let right_casted = arrow::compute::cast(&right, &R::DATA_TYPE)?;
- 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) {
Review 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.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: simplify `calculate_binary_math` in datafusion-functions [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #18525:
URL: https://github.com/apache/datafusion/pull/18525#discussion_r2505633207
##########
datafusion/functions/src/utils.rs:
##########
@@ -140,38 +140,25 @@ where
F: Fn(L::Native, R::Native) -> Result<O::Native, ArrowError>,
R::Native: TryFrom<ScalarValue>,
{
- Ok(match right {
+ let left = left.as_primitive::<L>();
+ let right = right.cast_to(&R::DATA_TYPE, None)?;
Review Comment:
I wonder why we aren't also casting left as well up here (it was only done in the branch below 🤔 )
##########
datafusion/functions/src/utils.rs:
##########
@@ -140,38 +140,25 @@ where
F: Fn(L::Native, R::Native) -> Result<O::Native, ArrowError>,
R::Native: TryFrom<ScalarValue>,
{
- Ok(match right {
+ let left = left.as_primitive::<L>();
+ let right = right.cast_to(&R::DATA_TYPE, None)?;
+ let result = match right {
ColumnarValue::Scalar(scalar) => {
- let right_value: R::Native =
- R::Native::try_from(scalar.clone()).map_err(|_| {
- DataFusionError::NotImplemented(format!(
- "Cannot convert scalar value {} to {}",
- &scalar,
- R::DATA_TYPE
- ))
- })?;
- let left_array = left.as_primitive::<L>();
- // Bind right value
- let result =
- left_array.try_unary::<_, O, _>(|lvalue| fun(lvalue, right_value))?;
- Arc::new(result) as _
+ let right = R::Native::try_from(scalar.clone()).map_err(|_| {
+ DataFusionError::NotImplemented(format!(
+ "Cannot convert scalar value {} to {}",
+ &scalar,
+ R::DATA_TYPE
+ ))
+ })?;
+ left.try_unary::<_, O, _>(|lvalue| fun(lvalue, right))?
}
ColumnarValue::Array(right) => {
- let right_casted = arrow::compute::cast(&right, &R::DATA_TYPE)?;
- 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) {
Review 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?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: simplify `calculate_binary_math` in datafusion-functions [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #18525:
URL: https://github.com/apache/datafusion/pull/18525#discussion_r2501895566
##########
datafusion/functions/src/utils.rs:
##########
@@ -140,38 +140,25 @@ where
F: Fn(L::Native, R::Native) -> Result<O::Native, ArrowError>,
R::Native: TryFrom<ScalarValue>,
{
- Ok(match right {
+ let left = left.as_primitive::<L>();
+ let right = right.cast_to(&R::DATA_TYPE, None)?;
Review Comment:
This right cast wasn't applied if it was a scalar, so cast the columnarvalue itself to align both arms
##########
datafusion/functions/src/utils.rs:
##########
@@ -140,38 +140,25 @@ where
F: Fn(L::Native, R::Native) -> Result<O::Native, ArrowError>,
R::Native: TryFrom<ScalarValue>,
{
- Ok(match right {
+ let left = left.as_primitive::<L>();
+ let right = right.cast_to(&R::DATA_TYPE, None)?;
+ let result = match right {
ColumnarValue::Scalar(scalar) => {
- let right_value: R::Native =
- R::Native::try_from(scalar.clone()).map_err(|_| {
- DataFusionError::NotImplemented(format!(
- "Cannot convert scalar value {} to {}",
- &scalar,
- R::DATA_TYPE
- ))
- })?;
- let left_array = left.as_primitive::<L>();
- // Bind right value
- let result =
- left_array.try_unary::<_, O, _>(|lvalue| fun(lvalue, right_value))?;
- Arc::new(result) as _
+ let right = R::Native::try_from(scalar.clone()).map_err(|_| {
+ DataFusionError::NotImplemented(format!(
+ "Cannot convert scalar value {} to {}",
+ &scalar,
+ R::DATA_TYPE
+ ))
+ })?;
+ left.try_unary::<_, O, _>(|lvalue| fun(lvalue, right))?
}
ColumnarValue::Array(right) => {
- let right_casted = arrow::compute::cast(&right, &R::DATA_TYPE)?;
- 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) {
Review 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
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: simplify `calculate_binary_math` in datafusion-functions [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #18525:
URL: https://github.com/apache/datafusion/pull/18525#discussion_r2505752475
##########
datafusion/functions/src/utils.rs:
##########
@@ -140,38 +140,25 @@ where
F: Fn(L::Native, R::Native) -> Result<O::Native, ArrowError>,
R::Native: TryFrom<ScalarValue>,
{
- Ok(match right {
+ let left = left.as_primitive::<L>();
+ let right = right.cast_to(&R::DATA_TYPE, None)?;
+ let result = match right {
ColumnarValue::Scalar(scalar) => {
- let right_value: R::Native =
- R::Native::try_from(scalar.clone()).map_err(|_| {
- DataFusionError::NotImplemented(format!(
- "Cannot convert scalar value {} to {}",
- &scalar,
- R::DATA_TYPE
- ))
- })?;
- let left_array = left.as_primitive::<L>();
- // Bind right value
- let result =
- left_array.try_unary::<_, O, _>(|lvalue| fun(lvalue, right_value))?;
- Arc::new(result) as _
+ let right = R::Native::try_from(scalar.clone()).map_err(|_| {
+ DataFusionError::NotImplemented(format!(
+ "Cannot convert scalar value {} to {}",
+ &scalar,
+ R::DATA_TYPE
+ ))
+ })?;
+ left.try_unary::<_, O, _>(|lvalue| fun(lvalue, right))?
}
ColumnarValue::Array(right) => {
- let right_casted = arrow::compute::cast(&right, &R::DATA_TYPE)?;
- 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) {
Review 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 🤔
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: simplify `calculate_binary_math` in datafusion-functions [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey merged PR #18525:
URL: https://github.com/apache/datafusion/pull/18525
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org