alamb commented on code in PR #6221:
URL: https://github.com/apache/arrow-datafusion/pull/6221#discussion_r1185101115
##########
datafusion/expr/src/type_coercion/binary.rs:
##########
@@ -106,6 +106,55 @@ pub fn binary_operator_data_type(
}
}
+pub fn get_result_type(
Review Comment:
I think we need some docstrings on this function. Something like:
```suggestion
/// returns the resulting type of a binary expression evaluating the `op`
with the left and right hand types
pub fn get_result_type(
```
Also calling this method `result_type` rather than `get_result_type` might
be more consistent with the conventions in the codebase (this is a minor point)
##########
datafusion/expr/src/type_coercion/binary.rs:
##########
@@ -106,6 +106,55 @@ pub fn binary_operator_data_type(
}
}
+pub fn get_result_type(
+ lhs_type: &DataType,
+ op: &Operator,
+ rhs_type: &DataType,
+) -> Result<DataType> {
+ let result = match op {
+ Operator::And
+ | Operator::Or
+ | Operator::Eq
+ | Operator::NotEq
+ | Operator::Lt
+ | Operator::Gt
+ | Operator::GtEq
+ | Operator::LtEq
+ | Operator::IsDistinctFrom
+ | Operator::IsNotDistinctFrom => Some(DataType::Boolean),
+ Operator::Plus | Operator::Minus
+ if is_datetime(lhs_type)
+ || is_datetime(rhs_type)
+ || is_interval(lhs_type)
+ || is_interval(rhs_type) =>
+ {
+ temporal_add_sub_coercion(lhs_type, rhs_type, op)
+ }
+ Operator::BitwiseAnd
+ | Operator::BitwiseOr
+ | Operator::BitwiseXor
+ | Operator::BitwiseShiftRight
+ | Operator::BitwiseShiftLeft
+ | Operator::Plus
+ | Operator::Minus
+ | Operator::Modulo
+ | Operator::Divide
+ | Operator::Multiply
+ | Operator::RegexMatch
+ | Operator::RegexIMatch
+ | Operator::RegexNotMatch
+ | Operator::RegexNotIMatch
+ | Operator::StringConcat => coerce_types(lhs_type, op, rhs_type).ok(),
+ };
+
+ match result {
+ None => Err(DataFusionError::Plan(format!(
+ "there isn't result type for {lhs_type:?} {op} {rhs_type:?}"
Review Comment:
```suggestion
"Unsupported argument types. Can not evaluate {lhs_type:?} {op}
{rhs_type:?}"
```
I think would make a nicer error message
##########
datafusion/expr/src/type_coercion/binary.rs:
##########
@@ -106,6 +106,55 @@ pub fn binary_operator_data_type(
}
}
+pub fn get_result_type(
+ lhs_type: &DataType,
+ op: &Operator,
+ rhs_type: &DataType,
+) -> Result<DataType> {
+ let result = match op {
+ Operator::And
+ | Operator::Or
+ | Operator::Eq
+ | Operator::NotEq
+ | Operator::Lt
+ | Operator::Gt
+ | Operator::GtEq
+ | Operator::LtEq
+ | Operator::IsDistinctFrom
+ | Operator::IsNotDistinctFrom => Some(DataType::Boolean),
+ Operator::Plus | Operator::Minus
+ if is_datetime(lhs_type)
+ || is_datetime(rhs_type)
+ || is_interval(lhs_type)
+ || is_interval(rhs_type) =>
+ {
+ temporal_add_sub_coercion(lhs_type, rhs_type, op)
Review Comment:
Eventually I think it would make the code easier to understand and maintain
if we can disentangle `coerce_types` from `get_result_types`. To start in this
PR, calling into coerce_types is fine.
Eventually I think it would keep the logic much simpler if
`get_result_type` is only called on expressions that have their inputs properly
coerced.
##########
datafusion/expr/src/type_coercion/binary.rs:
##########
@@ -42,7 +42,7 @@ pub fn binary_operator_data_type(
let result_type = if !any_decimal(lhs_type, rhs_type) {
Review Comment:
I als find `binary_oprator_data_type` to be confusing and redundant with
`get_result_type` but we can fix that in a follow on PR perhaps
##########
datafusion/physical-expr/src/expressions/datetime.rs:
##########
@@ -77,7 +77,7 @@ impl PhysicalExpr for DateTimeIntervalExpr {
}
fn data_type(&self, input_schema: &Schema) -> Result<DataType> {
Review Comment:
I would also love to remove `DateTimeIntervalExpr` out of its own thing and
into binary.rs with the other operators, but for now 👍
--
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]
