findepi commented on code in PR #13741:
URL: https://github.com/apache/datafusion/pull/13741#discussion_r1881583324
##########
datafusion/expr-common/src/type_coercion/binary.rs:
##########
@@ -1449,6 +1455,22 @@ fn null_coercion(lhs_type: &DataType, rhs_type:
&DataType) -> Option<DataType> {
}
}
+/// Resolves integer types to interval types for temporal arithmetic
+fn resolve_ints_to_intervals(
+ lhs: &DataType,
+ rhs: &DataType,
+) -> Option<(DataType, DataType, DataType)> {
+ use arrow::datatypes::DataType::*;
+ use arrow::datatypes::IntervalUnit::*;
+
+ match (lhs, rhs) {
+ // Handle integer + interval cases
+ (Int32 | Int64, _) => Some((Interval(DayTime), rhs.clone(),
rhs.clone())),
Review Comment:
should this check rhs type to be something specific?
##########
datafusion/expr-common/src/type_coercion/binary.rs:
##########
@@ -1449,6 +1455,22 @@ fn null_coercion(lhs_type: &DataType, rhs_type:
&DataType) -> Option<DataType> {
}
}
+/// Resolves integer types to interval types for temporal arithmetic
+fn resolve_ints_to_intervals(
+ lhs: &DataType,
+ rhs: &DataType,
+) -> Option<(DataType, DataType, DataType)> {
+ use arrow::datatypes::DataType::*;
+ use arrow::datatypes::IntervalUnit::*;
+
+ match (lhs, rhs) {
+ // Handle integer + interval cases
Review Comment:
"+ interval" or "+ date" ?
##########
datafusion/expr-common/src/type_coercion/binary.rs:
##########
@@ -1607,6 +1629,18 @@ mod tests {
}};
}
+ /// Test coercion rules for assymetric binary operators
+ ///
+ /// Applies coercion rules for `$LHS_TYPE $OP $RHS_TYPE` and asserts that
the
+ /// the result types are `$RESULT_TYPE1` and `$RESULT_TYPE2` respectively
+ macro_rules! test_coercion_assymetric_binary_rule {
+ ($LHS_TYPE:expr, $RHS_TYPE:expr, $OP:expr, $RESULT_TYPE1:expr,
$RESULT_TYPE2:expr) => {{
+ let (lhs, rhs) = get_input_types(&$LHS_TYPE, &$OP, &$RHS_TYPE)?;
+ assert_eq!(lhs, $RESULT_TYPE1);
+ assert_eq!(rhs, $RESULT_TYPE2);
Review Comment:
for `+` operator symmetry is expected, so the assert function could do this
for us
##########
datafusion/expr-common/src/type_coercion/binary.rs:
##########
@@ -1607,6 +1629,18 @@ mod tests {
}};
}
+ /// Test coercion rules for assymetric binary operators
+ ///
+ /// Applies coercion rules for `$LHS_TYPE $OP $RHS_TYPE` and asserts that
the
+ /// the result types are `$RESULT_TYPE1` and `$RESULT_TYPE2` respectively
+ macro_rules! test_coercion_assymetric_binary_rule {
Review Comment:
What's the benefit of this being a macro? Could this be ordinary function?
##########
datafusion/expr-common/src/type_coercion/binary.rs:
##########
@@ -1869,6 +1905,51 @@ mod tests {
Operator::Multiply,
DataType::Float64
);
+
+ // Test integer to interval coercion for temporal arithmetic
+ test_coercion_assymetric_binary_rule!(
+ DataType::Int32,
+ DataType::Timestamp(TimeUnit::Nanosecond, None),
+ Operator::Plus,
+ DataType::Interval(IntervalUnit::DayTime),
+ DataType::Timestamp(TimeUnit::Nanosecond, None)
+ );
+ test_coercion_assymetric_binary_rule!(
+ DataType::Timestamp(TimeUnit::Nanosecond, None),
+ DataType::Int64,
+ Operator::Plus,
+ DataType::Timestamp(TimeUnit::Nanosecond, None),
+ DataType::Interval(IntervalUnit::DayTime)
+ );
+ test_coercion_assymetric_binary_rule!(
+ DataType::Int32,
+ DataType::Date32,
Review Comment:
do we also have Date32 + Int32 test (in that order)?
##########
datafusion/expr/src/expr_schema.rs:
##########
@@ -453,6 +455,26 @@ impl ExprSchemable for Expr {
}
_ => Ok(Expr::Cast(Cast::new(Box::new(self),
cast_to_type.clone()))),
}
+ } else if matches!(
+ (&this_type, cast_to_type),
+ (DataType::Int32 | DataType::Int64, DataType::Interval(_))
+ ) {
+ // Convert integer (days) to the corresponding DayTime interval
+ match self {
+ Expr::Literal(ScalarValue::Int32(Some(days))) => {
+ Ok(Expr::Literal(ScalarValue::IntervalDayTime(Some(
+ arrow_buffer::IntervalDayTime::new(days, 0),
+ ))))
+ }
+ Expr::Literal(ScalarValue::Int64(Some(days))) => {
+ Ok(Expr::Literal(ScalarValue::IntervalDayTime(Some(
+ arrow_buffer::IntervalDayTime::new(days as i32, 0),
+ ))))
+ }
+ _ => plan_err!(
+ "Cannot automatically convert {this_type:?} to
{cast_to_type:?}"
+ ),
+ }
Review Comment:
This works for literals only and is immediate.
For other types all the function does is create cast expression. Why would
we want to special-case here?
--
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-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
