findepi commented on code in PR #13741: URL: https://github.com/apache/datafusion/pull/13741#discussion_r1881988070
########## datafusion/expr-common/src/type_coercion/binary.rs: ########## @@ -186,6 +186,12 @@ fn signature(lhs: &DataType, op: &Operator, rhs: &DataType) -> Result<Signature> } else if let Some(numeric) = mathematics_numerical_coercion(lhs, rhs) { // Numeric arithmetic, e.g. Int32 + Int32 Ok(Signature::uniform(numeric)) + } else if let Some((new_lhs, new_rhs, ret)) = resolve_ints_to_intervals(lhs, rhs) { Review Comment: we're in `Plus | Minus | Multiply | Divide | Modulo => {` branch but the new logic should apply - to `+` symmetrically - to `date - int`, but likely not to `int - date` (at least per PostgreSQL) - not at all to other operators so what about passing the operator to the new function and doing the work for `+` and `-` (so that code already identifies asymmetric treatment) ########## 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: if a Cast is supposed to work, it should work regardless how it's weaved into the plan. `Expr.cast_to` should not have any additional logic (other than checks) vs creating `Expr::Cast` directly. However, we should first decide whether integer should be castable to interval at all. It's not castable in PostgreSQL ``` postgres=# SELECT 10::interval; ERROR: cannot cast type integer to interval LINE 1: SELECT 10::interval; ^ ``` so I am inclined it should not be castable in DF either. Which means, the `date + int` cannot be converted (desugared) to `date + interval` using ordinary cast expression (as such shouldn't exist), but it can be converted (desugared) by some other means (eg with a custom conversion function or combination of expressions). Alternatively, we defer `date + int` until the execution, eg somewhere in https://github.com/apache/datafusion/blob/cc43766104cf66b8dbf431cbf6bdf25eda6ba274/datafusion/physical-expr/src/expressions/binary.rs#L301 but i'd prefer to desugar earlier and avoid separate code path in the `BinaryExpr` physical expr ########## datafusion/expr-common/src/type_coercion/binary.rs: ########## @@ -1449,6 +1455,26 @@ 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 + temporal types cases + (Int32 | Int64, rhs) if rhs.is_temporal() => { Review Comment: it should be `date + int`, not `any_temporal + int`: ``` postgres=# select now()::date + 1; ?column? ------------ 2024-12-13 (1 row) postgres=# select now()::time + 1; ERROR: operator does not exist: time without time zone + integer LINE 1: select now()::time + 1; ^ HINT: No operator matches the given name and argument types. You might need to add explicit type casts. postgres=# select now()::timestamp + 1; ERROR: operator does not exist: timestamp without time zone + integer LINE 1: select now()::timestamp + 1; ^ HINT: No operator matches the given name and argument types. You might need to add explicit type casts. postgres=# select now()::timestamptz + 1; ERROR: operator does not exist: timestamp with time zone + integer LINE 1: select now()::timestamptz + 1; ^ HINT: No operator matches the given name and argument types. You might need to add explicit type casts. ``` (i didn't check other `is_temporal` types: interval, duration) ########## 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: i assumed so. But also -- macros aren't "better functions", they come with downsides, so if something is expressible as a function, it should. ########## 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: You could have this ```rust let (a, b) = get_input_types(&$LHS_TYPE, &$OP, &$RHS_TYPE)?; assert_eq!(a, $RESULT_TYPE1); assert_eq!(b, $RESULT_TYPE2); // test symmetry let (b, a) = get_input_types(&$RHS_TYPE, &$OP, &$LHS_TYPE)?; assert_eq!(a, $RESULT_TYPE1); assert_eq!(b, $RESULT_TYPE2); ``` -- 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