realno commented on a change in pull request #1525:
URL: https://github.com/apache/arrow-datafusion/pull/1525#discussion_r780628753
##########
File path: datafusion/src/scalar.rs
##########
@@ -526,6 +526,282 @@ macro_rules! eq_array_primitive {
}
impl ScalarValue {
+ /// Return true if the value is numeric
+ pub fn is_numeric(&self) -> bool {
+ matches!(self,
+ ScalarValue::Float32(_)
+ | ScalarValue::Float64(_)
+ | ScalarValue::Decimal128(_, _, _)
+ | ScalarValue::Int8(_)
+ | ScalarValue::Int16(_)
+ | ScalarValue::Int32(_)
+ | ScalarValue::Int64(_)
+ | ScalarValue::UInt8(_)
+ | ScalarValue::UInt16(_)
+ | ScalarValue::UInt32(_)
+ | ScalarValue::UInt64(_)
+ )
+ }
+
+ /// Add two numeric ScalarValues
+ pub fn add(lhs: &ScalarValue, rhs: &ScalarValue) -> Result<ScalarValue> {
+ if !lhs.is_numeric() || !rhs.is_numeric() {
+ return Err(DataFusionError::Internal(format!(
+ "Addition only supports numeric types, \
+ here has {:?} and {:?}",
+ lhs.get_datatype(),
+ rhs.get_datatype()
+ )));
+ }
+
+ // TODO: Finding a good way to support operation between different
types without
Review comment:
First of all, thanks for suggesting this approach, I was able to get it
working after making `evaluate_to_scalar` to `pub(crate)`, it will reduce the
amount of code significantly.
Though after some thinking I am leaning towards having a separate discussion
and make the change in a different PR. Here are some of my thoughts:
1. It requires changing the scope of an internal function
2. As you mentioned, there are quite a bit of performance overhead
3. I think ScalarValue arithmetic is important to many future operators, we
can take the chance to provide official support. It feels right to formally
discuss the topic and review if this is the best way to achieve it.
4. This method doesn't support uptype when overflow happens. This is useful
because for aggregation functions it is quite common the results requires more
accuracy than the original type.
5. Some other functions such as `sum` and `avg` also have similar
implementations. Though this is not a good reason to keep as is but it shows
the value of official Scalar arithmetic support.
Please let me know your thoughts. If makes sense I am happy to create a new
issue for the work and join the discussion. Thanks!
--
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]
