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
