edponce commented on a change in pull request #10567:
URL: https://github.com/apache/arrow/pull/10567#discussion_r655907205
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -1511,5 +1517,55 @@ TEST(TestBinaryArithmeticDecimal, Divide) {
}
}
+TYPED_TEST(TestUnaryArithmeticFloating, Log) {
+ auto ty = this->type_singleton();
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertUnaryOp(Ln, ArrayFromJSON(ty, "[1, 2.7182818284590452354]"),
+ ArrayFromJSON(ty, "[0, 1]"));
+ this->AssertUnaryOp(Log10, ArrayFromJSON(ty, "[1, 10]"), ArrayFromJSON(ty,
"[0, 1]"));
+ this->AssertUnaryOp(Log2, ArrayFromJSON(ty, "[1, 2]"), ArrayFromJSON(ty,
"[0, 1]"));
+ }
+ this->AssertUnaryOpRaises(Ln, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Ln, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log10, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log10, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log2, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log2, "[-1]", "domain error");
+}
+
+TYPED_TEST(TestUnaryArithmeticSigned, Log) {
+ auto ty = this->type_singleton();
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertUnaryOp(Ln, ArrayFromJSON(ty, "[1]"), ArrayFromJSON(float64(),
"[0]"));
+ this->AssertUnaryOp(Log10, ArrayFromJSON(ty, "[1, 10]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ this->AssertUnaryOp(Log2, ArrayFromJSON(ty, "[1, 2]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ }
+ this->AssertUnaryOpRaises(Ln, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Ln, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log10, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log10, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log2, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log2, "[-1]", "domain error");
+}
+
+TYPED_TEST(TestUnaryArithmeticUnsigned, Log) {
+ auto ty = this->type_singleton();
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertUnaryOp(Ln, ArrayFromJSON(ty, "[1]"), ArrayFromJSON(float64(),
"[0]"));
+ this->AssertUnaryOp(Log10, ArrayFromJSON(ty, "[1, 10]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ this->AssertUnaryOp(Log2, ArrayFromJSON(ty, "[1, 2]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ }
+ this->AssertUnaryOpRaises(Ln, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log10, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log2, "[0]", "divide by zero");
+}
+
Review comment:
In valid test cases, no need to use `ty` (aka `this->type_singleton()`)
with `ArrayFromJSON` because that is the default type. Also, no need to use the
`this` keyword for the class methods.
Moreover, PR #10395 extends the unary scalar arithmetic test class to
support combinations of JSON/Array inputs which will allow you to further
simplify the test statements as follows:
* For integer inputs: `AssertUnaryOp(Ln, "[1]", ArrayFromJSON(float64(),
"[0]"));`
* For floating point inputs: `AssertUnaryOp(Log10, "[1, 10]", "[0, 1]");`
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -1511,5 +1517,55 @@ TEST(TestBinaryArithmeticDecimal, Divide) {
}
}
+TYPED_TEST(TestUnaryArithmeticFloating, Log) {
+ auto ty = this->type_singleton();
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertUnaryOp(Ln, ArrayFromJSON(ty, "[1, 2.7182818284590452354]"),
+ ArrayFromJSON(ty, "[0, 1]"));
+ this->AssertUnaryOp(Log10, ArrayFromJSON(ty, "[1, 10]"), ArrayFromJSON(ty,
"[0, 1]"));
+ this->AssertUnaryOp(Log2, ArrayFromJSON(ty, "[1, 2]"), ArrayFromJSON(ty,
"[0, 1]"));
+ }
+ this->AssertUnaryOpRaises(Ln, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Ln, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log10, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log10, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log2, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log2, "[-1]", "domain error");
+}
+
+TYPED_TEST(TestUnaryArithmeticSigned, Log) {
+ auto ty = this->type_singleton();
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertUnaryOp(Ln, ArrayFromJSON(ty, "[1]"), ArrayFromJSON(float64(),
"[0]"));
+ this->AssertUnaryOp(Log10, ArrayFromJSON(ty, "[1, 10]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ this->AssertUnaryOp(Log2, ArrayFromJSON(ty, "[1, 2]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ }
+ this->AssertUnaryOpRaises(Ln, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Ln, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log10, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log10, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log2, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log2, "[-1]", "domain error");
+}
+
+TYPED_TEST(TestUnaryArithmeticUnsigned, Log) {
+ auto ty = this->type_singleton();
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertUnaryOp(Ln, ArrayFromJSON(ty, "[1]"), ArrayFromJSON(float64(),
"[0]"));
+ this->AssertUnaryOp(Log10, ArrayFromJSON(ty, "[1, 10]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ this->AssertUnaryOp(Log2, ArrayFromJSON(ty, "[1, 2]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ }
+ this->AssertUnaryOpRaises(Ln, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log10, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log2, "[0]", "divide by zero");
+}
+
Review comment:
Add test cases with `Inf` and `NaN` inputs.
##########
File path: docs/source/cpp/compute.rst
##########
@@ -274,6 +274,18 @@ an ``Invalid`` :class:`Status` when overflow is detected.
+--------------------------+------------+--------------------+---------------------+
| divide_checked | Binary | Numeric | Numeric (1)
|
+--------------------------+------------+--------------------+---------------------+
+| ln | Unary | Numeric | Numeric
|
++--------------------------+------------+--------------------+---------------------+
+| ln_checked | Unary | Numeric | Numeric
|
++--------------------------+------------+--------------------+---------------------+
+| log10 | Unary | Numeric | Numeric
|
++--------------------------+------------+--------------------+---------------------+
+| log10_checked | Unary | Numeric | Numeric
|
++--------------------------+------------+--------------------+---------------------+
+| log2 | Unary | Numeric | Numeric
|
++--------------------------+------------+--------------------+---------------------+
+| log2_checked | Unary | Numeric | Numeric
|
++--------------------------+------------+--------------------+---------------------+
Review comment:
I would suggest to add a note stating that these functions return
`float64` value for integral inputs and same type as input for floating-point
inputs.
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -1511,5 +1517,55 @@ TEST(TestBinaryArithmeticDecimal, Divide) {
}
}
+TYPED_TEST(TestUnaryArithmeticFloating, Log) {
+ auto ty = this->type_singleton();
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertUnaryOp(Ln, ArrayFromJSON(ty, "[1, 2.7182818284590452354]"),
+ ArrayFromJSON(ty, "[0, 1]"));
+ this->AssertUnaryOp(Log10, ArrayFromJSON(ty, "[1, 10]"), ArrayFromJSON(ty,
"[0, 1]"));
+ this->AssertUnaryOp(Log2, ArrayFromJSON(ty, "[1, 2]"), ArrayFromJSON(ty,
"[0, 1]"));
+ }
+ this->AssertUnaryOpRaises(Ln, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Ln, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log10, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log10, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log2, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log2, "[-1]", "domain error");
+}
+
+TYPED_TEST(TestUnaryArithmeticSigned, Log) {
+ auto ty = this->type_singleton();
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertUnaryOp(Ln, ArrayFromJSON(ty, "[1]"), ArrayFromJSON(float64(),
"[0]"));
+ this->AssertUnaryOp(Log10, ArrayFromJSON(ty, "[1, 10]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ this->AssertUnaryOp(Log2, ArrayFromJSON(ty, "[1, 2]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ }
+ this->AssertUnaryOpRaises(Ln, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Ln, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log10, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log10, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log2, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log2, "[-1]", "domain error");
+}
+
+TYPED_TEST(TestUnaryArithmeticUnsigned, Log) {
+ auto ty = this->type_singleton();
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertUnaryOp(Ln, ArrayFromJSON(ty, "[1]"), ArrayFromJSON(float64(),
"[0]"));
+ this->AssertUnaryOp(Log10, ArrayFromJSON(ty, "[1, 10]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ this->AssertUnaryOp(Log2, ArrayFromJSON(ty, "[1, 2]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ }
+ this->AssertUnaryOpRaises(Ln, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log10, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log2, "[0]", "divide by zero");
+}
+
Review comment:
Add test cases with `Inf`, `NaN`, `null`, `min`, `max` inputs.
For min/max you can refer to the tests of
[`AbsoluteValue`](https://github.com/apache/arrow/blob/master/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc#L1111-L1147).
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -1511,5 +1517,55 @@ TEST(TestBinaryArithmeticDecimal, Divide) {
}
}
+TYPED_TEST(TestUnaryArithmeticFloating, Log) {
+ auto ty = this->type_singleton();
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertUnaryOp(Ln, ArrayFromJSON(ty, "[1, 2.7182818284590452354]"),
+ ArrayFromJSON(ty, "[0, 1]"));
+ this->AssertUnaryOp(Log10, ArrayFromJSON(ty, "[1, 10]"), ArrayFromJSON(ty,
"[0, 1]"));
+ this->AssertUnaryOp(Log2, ArrayFromJSON(ty, "[1, 2]"), ArrayFromJSON(ty,
"[0, 1]"));
+ }
+ this->AssertUnaryOpRaises(Ln, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Ln, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log10, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log10, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log2, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log2, "[-1]", "domain error");
+}
+
+TYPED_TEST(TestUnaryArithmeticSigned, Log) {
+ auto ty = this->type_singleton();
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertUnaryOp(Ln, ArrayFromJSON(ty, "[1]"), ArrayFromJSON(float64(),
"[0]"));
+ this->AssertUnaryOp(Log10, ArrayFromJSON(ty, "[1, 10]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ this->AssertUnaryOp(Log2, ArrayFromJSON(ty, "[1, 2]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ }
+ this->AssertUnaryOpRaises(Ln, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Ln, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log10, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log10, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log2, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log2, "[-1]", "domain error");
+}
+
+TYPED_TEST(TestUnaryArithmeticUnsigned, Log) {
+ auto ty = this->type_singleton();
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertUnaryOp(Ln, ArrayFromJSON(ty, "[1]"), ArrayFromJSON(float64(),
"[0]"));
+ this->AssertUnaryOp(Log10, ArrayFromJSON(ty, "[1, 10]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ this->AssertUnaryOp(Log2, ArrayFromJSON(ty, "[1, 2]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ }
+ this->AssertUnaryOpRaises(Ln, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log10, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log2, "[0]", "divide by zero");
+}
+
Review comment:
In valid test cases, no need to use `ty` (aka `this->type_singleton()`)
with `ArrayFromJSON` because that is the default type.
Moreover, PR #10395 extends the unary scalar arithmetic test class to
support combinations of JSON/Array inputs which will allow you to further
simplify the test statements as follows:
* For integer inputs: `AssertUnaryOp(Ln, "[1]", ArrayFromJSON(float64(),
"[0]"));`
* For floating point inputs: `AssertUnaryOp(Log10, "[1, 10]", "[0, 1]");`
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -734,6 +924,19 @@ std::shared_ptr<ScalarFunction>
MakeUnarySignedArithmeticFunctionNotNull(
return func;
}
+// Integer arguments return double values
+template <typename Op>
+std::shared_ptr<ScalarFunction> MakeUnaryIntToDoubleNotNull(std::string name,
+ const FunctionDoc*
doc) {
+ auto func = std::make_shared<ArithmeticFunction>(name, Arity::Unary(), doc);
+ for (const auto& ty : NumericTypes()) {
+ auto output = is_integer(ty->id()) ? float64() : ty;
+ auto exec = IntToDoubleExecFromOp<ScalarUnaryNotNull, Op>(ty);
+ DCHECK_OK(func->AddKernel({ty}, output, exec));
+ }
+ return func;
+}
+
Review comment:
I suggest to change the function name to
`MakeUnaryArithmeticFunctionFloatOutTypeNotNull`.
Also, the `_checked` variants use the `ScalarUnaryNotNull` kernel exec
generator but the regular variants use `ScalarUnary`. Need to add
`MakeUnaryArithmeticFunctionFloatOutType` with same logic but using
`ScalarUnary`.
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -485,6 +644,37 @@ ArrayKernelExec ArithmeticExecFromOp(detail::GetTypeId
get_id) {
}
}
+// For kernels that always return floating results
+template <template <typename... Args> class KernelGenerator, typename Op>
+ArrayKernelExec IntToDoubleExecFromOp(detail::GetTypeId get_id) {
Review comment:
The variety of generator dispatchers available in the compute kernel are
not consistent with their names. At some point we should rename them
consistently, but that is out-of-scope for this PR. Nevertheless, I suggest to
change the name of this generator dispatcher to
`GenerateUnaryArithmeticFloatOutType`. This is simply a suggestion, we can
rename later.
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -485,6 +644,37 @@ ArrayKernelExec ArithmeticExecFromOp(detail::GetTypeId
get_id) {
}
}
+// For kernels that always return floating results
+template <template <typename... Args> class KernelGenerator, typename Op>
+ArrayKernelExec IntToDoubleExecFromOp(detail::GetTypeId get_id) {
Review comment:
*For context:* There are a variety of generator dispatchers in the
compute layer and their names are inconsistent (this is indirectly related to
ARROW-9161). There has been previous work in [renaming them for
consistency](https://github.com/apache/arrow/pull/7461#issuecomment-645623392)
but looking at the codebase, we will need another pass.
I suggest to change the name `IntToDoubleExecFromOp` to
`GenerateArithmeticWithFloatOutType`.
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -734,6 +924,19 @@ std::shared_ptr<ScalarFunction>
MakeUnarySignedArithmeticFunctionNotNull(
return func;
}
+// Integer arguments return double values
+template <typename Op>
+std::shared_ptr<ScalarFunction> MakeUnaryIntToDoubleNotNull(std::string name,
+ const FunctionDoc*
doc) {
+ auto func = std::make_shared<ArithmeticFunction>(name, Arity::Unary(), doc);
+ for (const auto& ty : NumericTypes()) {
+ auto output = is_integer(ty->id()) ? float64() : ty;
+ auto exec = IntToDoubleExecFromOp<ScalarUnaryNotNull, Op>(ty);
+ DCHECK_OK(func->AddKernel({ty}, output, exec));
+ }
+ return func;
+}
+
Review comment:
I suggest to change the function name to
`MakeUnaryArithmeticFunctionWithFloatOutTypeNotNull`.
Also, the `_checked` variants use the `ScalarUnaryNotNull` kernel exec
generator but the regular variants use `ScalarUnary`. Need to add
`MakeUnaryArithmeticFunctionWithFloatOutType` with same logic but using
`ScalarUnary`.
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
