lidavidm commented on a change in pull request #10898:
URL: https://github.com/apache/arrow/pull/10898#discussion_r686927494
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -2001,6 +2020,45 @@ TYPED_TEST(TestUnaryArithmeticIntegral, Log) {
}
}
+TYPED_TEST(TestBinaryArithmeticIntegral, Log) {
+ // Integer arguments promoted to double, sanity check here
+ this->AssertBinop(Logb, "[1, 10, null]", "[10, 10, null]",
+ ArrayFromJSON(float64(), "[0, 1, null]"));
+ this->AssertBinop(Logb, "[1, 2, null]", "[2, 2, null]",
+ ArrayFromJSON(float64(), "[0, 1, null]"));
+ this->AssertBinop(Logb, "[10, 100, null]", this->MakeScalar(10),
+ ArrayFromJSON(float64(), "[1, 2, null]"));
+}
+
+TYPED_TEST(TestBinaryArithmeticFloating, Log) {
+ // Integer arguments promoted to double, sanity check here
Review comment:
nit: I don't think this comment applies here
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -1589,6 +1636,19 @@ const FunctionDoc log1p_checked_doc{
"-inf or NaN."),
{"x"}};
+const FunctionDoc logb_doc{
+ "Compute log of x to base b of arguments element-wise",
+ ("Values <= -1 return -inf or NaN. Null values return null.\n"
Review comment:
Values <= 0?
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -2001,6 +2020,45 @@ TYPED_TEST(TestUnaryArithmeticIntegral, Log) {
}
}
+TYPED_TEST(TestBinaryArithmeticIntegral, Log) {
+ // Integer arguments promoted to double, sanity check here
+ this->AssertBinop(Logb, "[1, 10, null]", "[10, 10, null]",
+ ArrayFromJSON(float64(), "[0, 1, null]"));
+ this->AssertBinop(Logb, "[1, 2, null]", "[2, 2, null]",
+ ArrayFromJSON(float64(), "[0, 1, null]"));
+ this->AssertBinop(Logb, "[10, 100, null]", this->MakeScalar(10),
+ ArrayFromJSON(float64(), "[1, 2, null]"));
+}
+
+TYPED_TEST(TestBinaryArithmeticFloating, Log) {
+ // Integer arguments promoted to double, sanity check here
+ this->AssertBinop(Logb, "[1.0, 10.0, null]", "[10.0, 10.0, null]", "[0.0,
1.0, null]");
+ this->AssertBinop(Logb, "[1.0, 2.0, null]", "[2.0, 2.0, null]", "[0.0, 1.0,
null]");
+ this->AssertBinop(Logb, "[10.0, 100.0, 1000.0, null]", this->MakeScalar(10),
+ "[1.0, 2.0, 3.0, null]");
Review comment:
We should also test what happens on Log of NaN, Inf, and -Inf. We should
also check the error cases below here, with both check_overflow = true and
check_overflow = false. Or really, it should look similar to the tests for the
unary log functions here:
https://github.com/apache/arrow/blob/8804fa6c21b9569526e551b92d345820cf63814c/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc#L1953
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -1317,6 +1351,19 @@ std::shared_ptr<ScalarFunction>
MakeArithmeticFunctionFloatingPoint(
return func;
}
+template <typename Op>
+std::shared_ptr<ScalarFunction> MakeArithmeticFunctionFloatingPointNotNull(
+ std::string name, const FunctionDoc* doc) {
+ auto func =
+ std::make_shared<ArithmeticFloatingPointFunction>(name, Arity::Binary(),
doc);
+ for (const auto& ty : FloatingPointTypes()) {
+ auto output = is_integer(ty->id()) ? float64() : ty;
Review comment:
nit: this line is redundant now (ty is never integer). I had filed
ARROW-13361 to clean this up.
--
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]
