alamb commented on code in PR #6433:
URL: https://github.com/apache/arrow-datafusion/pull/6433#discussion_r1205975000
##########
datafusion/physical-expr/src/expressions/binary/kernels_arrow.rs:
##########
@@ -333,34 +337,88 @@ pub(crate) fn add_dyn_temporal(left: &ArrayRef, right:
&ArrayRef) -> Result<Arra
}
}
-pub(crate) fn add_dyn_temporal_scalar(
+pub(crate) fn add_dyn_temporal_right_scalar(
Review Comment:
Since adding `interval + duration` is the same as adding `duration +
interval` I wonder if we need both of these kernels? Instead, perhaps we can
use the same pattern as used elsewhere in binary.rs and do something like:
Given: `a + b`:
* if `b` is a scalar, call `add_dyn_scalar(a, b)`
* if `a` is a scalar, call `add_dyn_scalar(b, a)`
Given: `a - b`:
* if `b` is a scalar, call `sub_dyn_scalar(a, b)`
* if `a` is a scalar, call `add_dyn_scalar(-b, a)`
🤔
##########
datafusion/physical-expr/src/expressions/binary.rs:
##########
@@ -1087,84 +1092,69 @@ impl BinaryExpr {
scalar: ScalarValue,
result_type: &DataType,
) -> Result<Option<Result<ArrayRef>>> {
+ use Operator::*;
Review Comment:
❤️
##########
datafusion/physical-expr/src/expressions/binary.rs:
##########
@@ -1519,18 +1599,85 @@ pub fn ts_scalar_interval_op(
array.data_type()
)))?,
};
- Ok(ColumnarValue::Array(ret))
+ Ok(ret)
+}
+
+/// This function handles timestamp +/- interval operations where the former is
Review Comment:
Minor suggestion is to move these functions into
datafusion/physical-expr/src/expressions/binary/kernels_arrow.rs so they are
more discoverable
##########
datafusion/physical-expr/src/expressions/binary.rs:
##########
@@ -1689,7 +1812,500 @@ pub fn interval_scalar_interval_op(
scalar.get_datatype(),
)))?,
};
- Ok(ColumnarValue::Array(ret))
+ Ok(ret)
+}
+
+/// This function handles interval +/- interval operations where the former is
+/// a scalar and the latter is an array, resulting in an interval array.
+pub fn scalar_interval_op_interval(
Review Comment:
cc @tustvold I am not sure if these exist (yet) in arrow-rs but I think we
should be working to port them upstream
--
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]
