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

Reply via email to