pitrou commented on code in PR #14434:
URL: https://github.com/apache/arrow/pull/14434#discussion_r1023140381
##########
cpp/src/arrow/compute/kernels/base_arithmetic_internal.h:
##########
@@ -485,6 +485,22 @@ struct NegateChecked {
}
};
+struct Exp {
+ template <typename T, typename Arg>
Review Comment:
Nothing mandatory, but now that we are C++17-compatible, can use `if
constexpr` to avoid SFINAE hacks:
```c++
template <typename T, typename Arg>
T Call(KernelContext*, Arg exp, Status*) {
static_assert(std::is_same<T, Arg>::value);
if constexpr(is_decimal_type<T>::value) {
return exp.Exp();
} else {
return std::exp(exp);
}
}
```
##########
docs/source/cpp/compute.rst:
##########
@@ -450,6 +450,18 @@ Mixed time resolution temporal inputs will be cast to
finest input resolution.
+------------------+--------+------------------+----------------------+-------+
| divide_checked | Binary | Numeric/Temporal | Numeric/Temporal | \(1) |
+------------------+--------+------------------+----------------------+-------+
+| exp | Unary | Numeric | Numeric | |
++------------------+--------+------------------+----------------------+-------+
+| ln | Unary | Numeric | Numeric | |
Review Comment:
`ln` and friends are already mentioned below under "Logarithmic functions".
##########
docs/source/cpp/compute.rst:
##########
@@ -450,6 +450,18 @@ Mixed time resolution temporal inputs will be cast to
finest input resolution.
+------------------+--------+------------------+----------------------+-------+
| divide_checked | Binary | Numeric/Temporal | Numeric/Temporal | \(1) |
+------------------+--------+------------------+----------------------+-------+
+| exp | Unary | Numeric | Numeric | |
Review Comment:
We should be more precise when listing input and output types (see examples
under "Logaritmic Functions" below).
Also you'll need to correct the table formatting after this :-)
```suggestion
| exp | Unary | Float32/Float64/Decimal | Float32/Float64
| |
```
##########
cpp/src/arrow/compute/api_scalar.h:
##########
@@ -610,6 +610,15 @@ Result<Datum> Power(const Datum& left, const Datum& right,
ArithmeticOptions options = ArithmeticOptions(),
ExecContext* ctx = NULLPTR);
+/// \brief Raise Euler's number to the specified power, element-wise.
+/// If the exponent value is null the result will be null.
+///
+/// \param[in] arg the base
Review Comment:
We use the term "power" in other sentences of this docstring.
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -2224,6 +2229,10 @@ void RegisterScalarArithmetic(FunctionRegistry*
registry) {
"power_checked", pow_checked_doc);
DCHECK_OK(registry->AddFunction(std::move(power_checked)));
+ // ----------------------------------------------------------------------
+ auto exp = MakeUnaryArithmeticFunctionFloatingPoint<Exp>("exp", exp_doc);
Review Comment:
This will implicitly convert the decimal _input_ to floating-point. Instead
it seems you want `ArithmeticDecimalToFloatingPointFunction`.
(otherwise, the `enable_if_decimal_value` snippet shouldn't even be needed)
--
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]
