jorgecarleitao commented on a change in pull request #8076:
URL: https://github.com/apache/arrow/pull/8076#discussion_r480293094
##########
File path: rust/datafusion/src/execution/physical_plan/expressions.rs
##########
@@ -991,48 +991,36 @@ impl fmt::Display for BinaryExpr {
}
}
-// Returns a formatted error about being impossible to coerce types for the
binary operator.
-fn coercion_error<T>(
- lhs_type: &DataType,
- op: &Operator,
- rhs_type: &DataType,
-) -> Result<T> {
+// Returns a formatted error about being impossible to coerce types to a
common type
+fn coercion_error<T>(lhs_type: &DataType, rhs_type: &DataType) -> Result<T> {
Err(ExecutionError::General(
format!(
- "The binary operator '{}' can't evaluate with lhs = '{:?}' and rhs
= '{:?}'",
Review comment:
The message was re-formulated because it made it a bit clearer that the
coercion did not fail due to the use of the types on a specific operator: any
operator would have yielded the same error, and thus I dropped the operator
from the message altogether.
This was a side effect of removing the operator from the coercions'
functuons. Essentially, code such as `numerical_coercion(lhs_type: &DataType,
op: Operator, rhs_type: &DataType)` suggests that coercion depends on the
operator, but this is not true: only the error message depends on the operator.
However, I do agree with you that that is a generic message. We could wrap
the error and re-write it with the operator's information. E.g.
```
Operator::Plus | Operator::Minus | Operator::Divide | Operator::Multiply => {
numerical_coercion(lhs_type, rhs_type)
}
```
could be a place to catch the error and re-write the error message with the
operator's information. What do you think?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
