jonahgao commented on code in PR #7904:
URL: https://github.com/apache/arrow-datafusion/pull/7904#discussion_r1368200017
##########
datafusion/physical-expr/src/math_expressions.rs:
##########
@@ -701,6 +701,29 @@ macro_rules! make_try_abs_function {
}};
}
+// Decimal types should keep the same precision and scale by using
`with_data_type()`.
+// https://github.com/apache/arrow-rs/issues/4644
+macro_rules! make_decimal_abs_function {
+ ($ARRAY_TYPE:ident) => {{
+ |args: &[ArrayRef]| {
+ let array = downcast_arg!(&args[0], "abs arg", $ARRAY_TYPE);
+ let res: $ARRAY_TYPE = array
+ .try_unary(|x| {
+ // Valid Decimal values should never overflow
+ x.checked_abs().ok_or_else(|| {
Review Comment:
@viirya
Not all operators/expressions require this. Overflow in `abs` here is a bug,
while others like `sum` may overflow even if inputs are valid.
I have considered using `wrapping_abs`, but I’m worried that if invalid
values are introduced elsewhere, they might be hidden and not easily detected.
It seems to be a trade-off between performance and safety, but for me, both
are ok.
I will follow the suggestion and change it to `wrapping_abs`.
##########
datafusion/physical-expr/src/math_expressions.rs:
##########
@@ -701,6 +701,29 @@ macro_rules! make_try_abs_function {
}};
}
+// Decimal types should keep the same precision and scale by using
`with_data_type()`.
+// https://github.com/apache/arrow-rs/issues/4644
+macro_rules! make_decimal_abs_function {
+ ($ARRAY_TYPE:ident) => {{
+ |args: &[ArrayRef]| {
+ let array = downcast_arg!(&args[0], "abs arg", $ARRAY_TYPE);
+ let res: $ARRAY_TYPE = array
+ .try_unary(|x| {
+ // Valid Decimal values should never overflow
+ x.checked_abs().ok_or_else(|| {
Review Comment:
@viirya
Not all operators/expressions require this. Overflow in `abs` here is a bug,
while others like `sum` may overflow even if inputs are valid.
I have considered using `wrapping_abs`, but I’m worried that if invalid
values are introduced elsewhere, they might be hidden and not easily detected.
It seems to be a trade-off between performance and safety, but for me, both
are ok.
I will follow the suggestion and change it to `wrapping_abs`.
--
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]
