westonpace commented on code in PR #36289:
URL: https://github.com/apache/arrow/pull/36289#discussion_r1259970381
##########
cpp/src/arrow/compute/kernels/scalar_round.cc:
##########
@@ -96,8 +117,29 @@ struct RoundUtil {
}
return (power >= 0) ? pow10 : (1 / pow10);
}
+
+ // Calculate powers of ten with arbitrary integer exponent
+ template <typename T>
+ static enable_if_integer_value<T> Pow10(int64_t power) {
+ DCHECK_GE(power, 0);
+ DCHECK_LE(power, std::numeric_limits<T>::digits10);
+ static constexpr uint64_t lut[] = {
+ Pow10Struct<0>::value, Pow10Struct<1>::value, Pow10Struct<2>::value,
+ Pow10Struct<3>::value, Pow10Struct<4>::value, Pow10Struct<5>::value,
+ Pow10Struct<6>::value, Pow10Struct<7>::value, Pow10Struct<8>::value,
+ Pow10Struct<9>::value, Pow10Struct<10>::value, Pow10Struct<11>::value,
+ Pow10Struct<12>::value, Pow10Struct<13>::value, Pow10Struct<14>::value,
+ Pow10Struct<15>::value, Pow10Struct<16>::value, Pow10Struct<17>::value,
+ Pow10Struct<18>::value, Pow10Struct<19>::value};
+
+ return static_cast<T>(lut[power]);
+ }
};
Review Comment:
This is clever, though I don't know if it is more readable than something
like
https://github.com/apache/arrow/blob/main/cpp/src/arrow/util/decimal_internal.h#L36-L58
##########
docs/source/cpp/compute.rst:
##########
@@ -563,30 +563,32 @@ representation based on the rounding criterion.
+-------------------+------------+-------------+-------------------------+----------------------------------+--------+
| floor | Unary | Numeric | Float32/Float64/Decimal |
| |
+-------------------+------------+-------------+-------------------------+----------------------------------+--------+
-| round | Unary | Numeric | Float32/Float64/Decimal |
:struct:`RoundOptions` | (1)(2) |
+| round | Unary | Numeric | Input Type |
:struct:`RoundOptions` | (1)(2) |
Review Comment:
So I think this is technically a breaking change right?
Before, if we had something like:
```
x = pa.array([1, 2], pa.int32())
y = pc.round(x)
```
Then `y` would be a double array. Now, `y` will be an `int32` array. I
think this is correct and the old behavior was unintentional so I think it is
an ok breaking change. Still, we should make sure to mark the PR as a breaking
change if my understanding is correct so that we document it as such in the
release notes.
CC @jorisvandenbossche for second opinion.
##########
docs/source/cpp/compute.rst:
##########
@@ -563,30 +563,32 @@ representation based on the rounding criterion.
+-------------------+------------+-------------+-------------------------+----------------------------------+--------+
| floor | Unary | Numeric | Float32/Float64/Decimal |
| |
+-------------------+------------+-------------+-------------------------+----------------------------------+--------+
-| round | Unary | Numeric | Float32/Float64/Decimal |
:struct:`RoundOptions` | (1)(2) |
+| round | Unary | Numeric | Input Type |
:struct:`RoundOptions` | (1)(2) |
+-------------------+------------+-------------+-------------------------+----------------------------------+--------+
-| round_to_multiple | Unary | Numeric | Float32/Float64/Decimal |
:struct:`RoundToMultipleOptions` | (1)(3) |
+| round_to_multiple | Unary | Numeric | Input Type |
:struct:`RoundToMultipleOptions` | (1)(3) |
+-------------------+------------+-------------+-------------------------+----------------------------------+--------+
| trunc | Unary | Numeric | Float32/Float64/Decimal |
| |
+-------------------+------------+-------------+-------------------------+----------------------------------+--------+
-* \(1) Output value is a 64-bit floating-point for integral inputs and the
- retains the same type for floating-point and decimal inputs. By default
- rounding functions displace a value to the nearest integer using
- HALF_TO_EVEN to resolve ties. Options are available to control the rounding
- criterion. Both ``round`` and ``round_to_multiple`` have the ``round_mode``
- option to set the rounding mode.
+* \(1) By default rounding functions displace a value to the nearest
Review Comment:
```suggestion
* \(1) By default rounding functions change a value to the nearest
```
I realize this wasn't your PR but I think we can use simpler English here.
--
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]
