You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/10/17 05:47:02 UTC
[GitHub] [arrow] drabastomek opened a new pull request, #14434: [DRAFT] Expanding coverage of math functions from Substrait to Acero
drabastomek opened a new pull request, #14434:
URL: https://github.com/apache/arrow/pull/14434
This PR adds the following functionality to Acero/Substrait consumer
1. Support for logarithmic functions.
2. Support for power, sqrt, exp, abs, sign.
3. Implements exp kernel.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
westonpace commented on PR #14434:
URL: https://github.com/apache/arrow/pull/14434#issuecomment-1287328017
Pinged @lidavidm / @pitrou for a brief review of the Exp kernel. You can probably ignore the Substrait part if you want.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a diff in pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #14434:
URL: https://github.com/apache/arrow/pull/14434#discussion_r1008326541
##########
cpp/src/arrow/compute/kernels/base_arithmetic_internal.h:
##########
@@ -485,6 +485,22 @@ struct NegateChecked {
}
};
+struct Exp {
+ template <typename T, typename Arg>
+ static constexpr enable_if_floating_value<Arg, T> Call(KernelContext*, Arg exp,
+ Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ return std::exp(exp);
+ }
+
+ template <typename T, typename Arg>
+ static constexpr enable_if_decimal_value<Arg, T> Call(KernelContext*, Arg exp,
+ Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ return exp.Exp();
Review Comment:
The code here is dead code. We should remove it.
It doesn't matter what Substrait does; we currently implement most of these kernels by casting to float first. If that's not OK for Substrait, then we'll have to come back to implement actual decimal versions of these kernels.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #14434:
URL: https://github.com/apache/arrow/pull/14434#discussion_r1025293714
##########
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:
I'm not asking whether C++ supports it, but whether Arrow wants to support it :-)
As a comparison, our logarithm functions only support reals (either floating-point or decimals), not integers.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rok commented on a diff in pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
rok commented on code in PR #14434:
URL: https://github.com/apache/arrow/pull/14434#discussion_r1023885830
##########
cpp/src/arrow/compute/kernels/base_arithmetic_internal.h:
##########
@@ -485,6 +485,22 @@ struct NegateChecked {
}
};
+struct Exp {
+ template <typename T, typename Arg>
Review Comment:
Removed the decimal branch since `exp.Exp()` doesn't exists.
##########
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:
Removed.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #14434:
URL: https://github.com/apache/arrow/pull/14434#discussion_r1024093560
##########
cpp/src/arrow/engine/substrait/function_test.cc:
##########
@@ -313,7 +330,18 @@ TEST(FunctionMapping, ValidCases) {
{"abc", "def"},
{utf8(), utf8()},
"abcdef",
- utf8()}};
+ utf8()},
+ {{kSubstraitLogarithmicFunctionsUri, "ln"}, {"1"}, {int8()}, "0", float64()},
+ {{kSubstraitLogarithmicFunctionsUri, "log10"}, {"1"}, {int8()}, "0", float64()},
+ {{kSubstraitLogarithmicFunctionsUri, "log2"}, {"2"}, {int8()}, "1", float64()},
+ {{kSubstraitLogarithmicFunctionsUri, "log1p"}, {"0"}, {int8()}, "0", float64()},
+ {{kSubstraitLogarithmicFunctionsUri, "logb"},
Review Comment:
I'll note these tests don't have a lot of discriminating power.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a diff in pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #14434:
URL: https://github.com/apache/arrow/pull/14434#discussion_r1002137969
##########
cpp/src/arrow/compute/kernels/base_arithmetic_internal.h:
##########
@@ -485,6 +485,22 @@ struct NegateChecked {
}
};
+struct Exp {
+ template <typename T, typename Arg>
+ static constexpr enable_if_floating_value<Arg, T> Call(KernelContext*, Arg exp,
+ Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ return std::exp(exp);
+ }
+
+ template <typename T, typename Arg>
+ static constexpr enable_if_decimal_value<Arg, T> Call(KernelContext*, Arg exp,
+ Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ return exp.Exp();
Review Comment:
I guess it's tested below so it should clearly exist…
##########
cpp/src/arrow/compute/kernels/base_arithmetic_internal.h:
##########
@@ -485,6 +485,22 @@ struct NegateChecked {
}
};
+struct Exp {
+ template <typename T, typename Arg>
+ static constexpr enable_if_floating_value<Arg, T> Call(KernelContext*, Arg exp,
+ Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ return std::exp(exp);
+ }
+
+ template <typename T, typename Arg>
+ static constexpr enable_if_decimal_value<Arg, T> Call(KernelContext*, Arg exp,
+ Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ return exp.Exp();
Review Comment:
Ah, no, this isn't being used at all - decimals are being converted to floats first. Can we remove this?
##########
cpp/src/arrow/compute/kernels/base_arithmetic_internal.h:
##########
@@ -485,6 +485,22 @@ struct NegateChecked {
}
};
+struct Exp {
+ template <typename T, typename Arg>
+ static constexpr enable_if_floating_value<Arg, T> Call(KernelContext*, Arg exp,
+ Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ return std::exp(exp);
+ }
+
+ template <typename T, typename Arg>
+ static constexpr enable_if_decimal_value<Arg, T> Call(KernelContext*, Arg exp,
+ Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ return exp.Exp();
Review Comment:
Does this actually exist? I don't see a definition for it in (Basic)Decimal128
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] drabastomek commented on a diff in pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
drabastomek commented on code in PR #14434:
URL: https://github.com/apache/arrow/pull/14434#discussion_r1003986308
##########
cpp/src/arrow/compute/kernels/base_arithmetic_internal.h:
##########
@@ -485,6 +485,22 @@ struct NegateChecked {
}
};
+struct Exp {
+ template <typename T, typename Arg>
+ static constexpr enable_if_floating_value<Arg, T> Call(KernelContext*, Arg exp,
+ Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ return std::exp(exp);
+ }
+
+ template <typename T, typename Arg>
+ static constexpr enable_if_decimal_value<Arg, T> Call(KernelContext*, Arg exp,
+ Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ return exp.Exp();
Review Comment:
QQ: David, are you asking if the `enable_if_decimal_value` exists? I'm not sure I understand your comment, sorry...
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] drabastomek commented on a diff in pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
drabastomek commented on code in PR #14434:
URL: https://github.com/apache/arrow/pull/14434#discussion_r999826503
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -1717,6 +1717,12 @@ const FunctionDoc pow_doc{
"wraps around. If either base or exponent is null the result will be null."),
{"base", "exponent"}};
+const FunctionDoc exp_doc{
+ "Computes Euler's number raised to the specified power, element-wise",
Review Comment:
Corrected the `exp_doc`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a diff in pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
westonpace commented on code in PR #14434:
URL: https://github.com/apache/arrow/pull/14434#discussion_r1008586544
##########
cpp/src/arrow/compute/kernels/base_arithmetic_internal.h:
##########
@@ -485,6 +485,22 @@ struct NegateChecked {
}
};
+struct Exp {
+ template <typename T, typename Arg>
+ static constexpr enable_if_floating_value<Arg, T> Call(KernelContext*, Arg exp,
+ Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ return std::exp(exp);
+ }
+
+ template <typename T, typename Arg>
+ static constexpr enable_if_decimal_value<Arg, T> Call(KernelContext*, Arg exp,
+ Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ return exp.Exp();
Review Comment:
FWIW, we should not be allowing this kind of implicit cast and should instead reject a plan containing a decimal exp call. If the producer wants to allow the cast to float they can allow it.
However, this issue is beyond this PR. I've created https://issues.apache.org/jira/browse/ARROW-18193
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] drabastomek commented on a diff in pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
drabastomek commented on code in PR #14434:
URL: https://github.com/apache/arrow/pull/14434#discussion_r1003984637
##########
cpp/src/arrow/engine/substrait/extension_set.cc:
##########
@@ -858,11 +867,34 @@ struct DefaultExtensionIdRegistry : ExtensionIdRegistryImpl {
// -------------- Substrait -> Arrow Functions -----------------
// Mappings with a _checked variant
- for (const auto& function_name : {"add", "subtract", "multiply", "divide"}) {
+ for (const auto& function_name :
+ {"add", "subtract", "multiply", "divide", "power", "sqrt", "abs"}) {
Review Comment:
Ha -- good catch. I changed the `DecodeOptionlessUncheckedArithmetic` and updated the tests.
##########
cpp/src/arrow/engine/substrait/extension_set.cc:
##########
@@ -858,11 +867,34 @@ struct DefaultExtensionIdRegistry : ExtensionIdRegistryImpl {
// -------------- Substrait -> Arrow Functions -----------------
// Mappings with a _checked variant
- for (const auto& function_name : {"add", "subtract", "multiply", "divide"}) {
+ for (const auto& function_name :
+ {"add", "subtract", "multiply", "divide", "power", "sqrt", "abs"}) {
DCHECK_OK(
AddSubstraitCallToArrow({kSubstraitArithmeticFunctionsUri, function_name},
DecodeOptionlessOverflowableArithmetic(function_name)));
}
+
+ // Mappings without a _checked variant
+ for (const auto& function_name : {"exp", "sign"}) {
+ DCHECK_OK(
+ AddSubstraitCallToArrow({kSubstraitArithmeticFunctionsUri, function_name},
+ DecodeOptionlessUncheckedArithmetic(function_name)));
+ }
+
+ // Mappings for log functions
+ for (const auto& function_name : {"ln", "log10", "log2", "logb", "log1p"}) {
+ DCHECK_OK(
+ AddSubstraitCallToArrow({kSubstraitLogarithmicFunctionsUri, function_name},
+ DecodeOptionlessOverflowableArithmetic(function_name)));
Review Comment:
I changed the DecodeOptionlessUncheckedArithmetic and updated the tests.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #14434:
URL: https://github.com/apache/arrow/pull/14434#issuecomment-1281197337
https://issues.apache.org/jira/browse/ARROW-15538
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #14434:
URL: https://github.com/apache/arrow/pull/14434#issuecomment-1281197367
:warning: Ticket **has not been started in JIRA**, please click 'Start Progress'.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
westonpace commented on PR #14434:
URL: https://github.com/apache/arrow/pull/14434#issuecomment-1315644132
@rok Yes, I think that would be helpful. I had agreed with @drabastomek offline to finish this up for him but hadn't gotten around to it yet. If you have a few cycles to address the review comments that would be appreciated!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] drabastomek commented on pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
drabastomek commented on PR #14434:
URL: https://github.com/apache/arrow/pull/14434#issuecomment-1315651328
@westonpace @rok Thanks guys! I had this on my radar for this week. I addressed the comments but would be great if @rok could have another looks -- much appreciated!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rok commented on a diff in pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
rok commented on code in PR #14434:
URL: https://github.com/apache/arrow/pull/14434#discussion_r1023885320
##########
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:
I've reworded this a little.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #14434:
URL: https://github.com/apache/arrow/pull/14434#discussion_r1004236510
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc:
##########
@@ -1519,6 +1519,65 @@ TEST_F(TestUnaryArithmeticDecimal, AbsoluteValue) {
}
}
+TYPED_TEST(TestUnaryArithmeticFloating, Exp) {
+ using CType = typename TestFixture::CType;
+
+ auto min = std::numeric_limits<CType>::lowest();
+ auto max = std::numeric_limits<CType>::max();
+
+ auto exp = [](const Datum& arg, ArithmeticOptions, ExecContext* ctx) {
+ return Exp(arg, ctx);
+ };
+ // Empty arrays
+ this->AssertUnaryOp(exp, "[]", "[]");
+ // Array with nulls
+ this->AssertUnaryOp(exp, "[null]", "[null]");
+ this->AssertUnaryOp(exp, this->MakeNullScalar(), this->MakeNullScalar());
+ this->AssertUnaryOp(exp, "[-1.0, null, 10.0]",
+ "[0.36787944117144233, null, 22026.465794806718]");
+ // Ordinary arrays (positive, negative, fractional, and zero inputs)
+ this->AssertUnaryOp(
+ exp, "[-10.0, 0, 0.5, 1.0]",
+ "[0.000045399929762484854,1.0,1.6487212707001282,2.718281828459045]");
+ this->AssertUnaryOp(exp, 1.3F, 3.6692964926535487F);
+ this->AssertUnaryOp(exp, this->MakeScalar(1.3F), this->MakeScalar(3.6692964926535487F));
+ // Arrays with infinites
+ this->AssertUnaryOp(exp, "[-Inf, Inf]", "[0, Inf]");
+ // Arrays with NaNs
+ this->SetNansEqual(true);
+ this->AssertUnaryOp(exp, "[NaN]", "[NaN]");
+ this->AssertUnaryOp(exp, "[NaN]", "[NaN]");
+ this->AssertUnaryOp(exp, "[NaN]", "[NaN]");
Review Comment:
Why three times?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #14434: [DRAFT] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #14434:
URL: https://github.com/apache/arrow/pull/14434#issuecomment-1280321351
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
Thanks for opening a pull request!
If this is not a [minor PR](https://github.com/apache/arrow/blob/master/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on JIRA? https://issues.apache.org/jira/browse/ARROW
Opening JIRAs ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename pull request title in the following format?
ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
See also:
* [Other pull requests](https://github.com/apache/arrow/pulls/)
* [Contribution Guidelines - How to contribute patches](https://arrow.apache.org/docs/developers/contributing.html#how-to-contribute-patches)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou merged pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
pitrou merged PR #14434:
URL: https://github.com/apache/arrow/pull/14434
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #14434:
URL: https://github.com/apache/arrow/pull/14434#discussion_r1024092008
##########
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:
Yes, implicit cast is fine then.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] drabastomek commented on a diff in pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
drabastomek commented on code in PR #14434:
URL: https://github.com/apache/arrow/pull/14434#discussion_r1023063717
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc:
##########
@@ -1519,6 +1519,65 @@ TEST_F(TestUnaryArithmeticDecimal, AbsoluteValue) {
}
}
+TYPED_TEST(TestUnaryArithmeticFloating, Exp) {
+ using CType = typename TestFixture::CType;
+
+ auto min = std::numeric_limits<CType>::lowest();
+ auto max = std::numeric_limits<CType>::max();
+
+ auto exp = [](const Datum& arg, ArithmeticOptions, ExecContext* ctx) {
+ return Exp(arg, ctx);
+ };
+ // Empty arrays
+ this->AssertUnaryOp(exp, "[]", "[]");
+ // Array with nulls
+ this->AssertUnaryOp(exp, "[null]", "[null]");
+ this->AssertUnaryOp(exp, this->MakeNullScalar(), this->MakeNullScalar());
+ this->AssertUnaryOp(exp, "[-1.0, null, 10.0]",
+ "[0.36787944117144233, null, 22026.465794806718]");
+ // Ordinary arrays (positive, negative, fractional, and zero inputs)
+ this->AssertUnaryOp(
+ exp, "[-10.0, 0, 0.5, 1.0]",
+ "[0.000045399929762484854,1.0,1.6487212707001282,2.718281828459045]");
+ this->AssertUnaryOp(exp, 1.3F, 3.6692964926535487F);
+ this->AssertUnaryOp(exp, this->MakeScalar(1.3F), this->MakeScalar(3.6692964926535487F));
+ // Arrays with infinites
+ this->AssertUnaryOp(exp, "[-Inf, Inf]", "[0, Inf]");
+ // Arrays with NaNs
+ this->SetNansEqual(true);
+ this->AssertUnaryOp(exp, "[NaN]", "[NaN]");
+ this->AssertUnaryOp(exp, "[NaN]", "[NaN]");
+ this->AssertUnaryOp(exp, "[NaN]", "[NaN]");
Review Comment:
Good catch! Thanks @pitrou
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a diff in pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
westonpace commented on code in PR #14434:
URL: https://github.com/apache/arrow/pull/14434#discussion_r997559329
##########
cpp/src/arrow/compute/kernels/base_arithmetic_internal.h:
##########
@@ -485,6 +485,34 @@ struct NegateChecked {
}
};
+struct Exp {
+ template <typename T, typename Arg>
+ static enable_if_floating_value<Arg, T> Call(KernelContext*, Arg exp, Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ return std::exp(exp);
+ }
+
+ template <typename T, typename Arg>
+ static enable_if_decimal_value<Arg, T> Call(KernelContext*, Arg exp, Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ return std::exp(exp);
+ }
+};
+
+struct ExpChecked {
Review Comment:
I'm not sure there is any value in creating a checked variant that does the same thing as the unchecked variant.
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -2224,6 +2230,15 @@ 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:
Any new kernel function will need unit tests.
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -1717,6 +1717,12 @@ const FunctionDoc pow_doc{
"wraps around. If either base or exponent is null the result will be null."),
{"base", "exponent"}};
+const FunctionDoc exp_doc{
+ "Computes Euler's number raised to the specified power, element-wise",
Review Comment:
```suggestion
"Compute Euler's number raised to the specified power, element-wise",
```
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -1717,6 +1717,12 @@ const FunctionDoc pow_doc{
"wraps around. If either base or exponent is null the result will be null."),
{"base", "exponent"}};
+const FunctionDoc exp_doc{
+ "Computes Euler's number raised to the specified power, element-wise",
+ ("Negative integer power returns an error. However, integer overflow\n"
Review Comment:
```suggestion
("Negative integer power returns an error. However, integer overflow\n"
```
What does negative integer power mean? This function only has non-integral kernels.
I'm not sure what you mean by integer overflow. Why "integer"? The return value is a floating point. If there is an overflow then "[+HUGE_VAL](https://en.cppreference.com/w/cpp/numeric/math/HUGE_VAL), +HUGE_VALF, or +HUGE_VALL is returned."
##########
cpp/src/arrow/engine/substrait/extension_set.cc:
##########
@@ -858,11 +858,27 @@ struct DefaultExtensionIdRegistry : ExtensionIdRegistryImpl {
// -------------- Substrait -> Arrow Functions -----------------
// Mappings with a _checked variant
- for (const auto& function_name : {"add", "subtract", "multiply", "divide"}) {
+ for (const auto& function_name : {"add", "subtract", "multiply", "divide", "sign",
+ "power", "sqrt", "exp", "abs"}) {
DCHECK_OK(
AddSubstraitCallToArrow({kSubstraitArithmeticFunctionsUri, function_name},
DecodeOptionlessOverflowableArithmetic(function_name)));
}
+
+ // Mappings for log functions
+ for (const auto& function_name : {"ln", "log10", "log2", "logb", "log1p"}) {
+ DCHECK_OK(
+ AddSubstraitCallToArrow({kSubstraitLogarithmicFunctionsUri, function_name},
+ DecodeOptionlessOverflowableArithmetic(function_name)));
+ }
+
+ // Mappings for rounding functions
+ for (const auto& function_name : {"ceil", "floor"}) {
+ DCHECK_OK(
+ AddSubstraitCallToArrow({kSubstraitRoundingFunctionsUri, function_name},
+ DecodeOptionlessOverflowableArithmetic(function_name)));
Review Comment:
Why use "Overflowable" here? I don't think there is a `ceil_checked` or `floor_checked`?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] drabastomek commented on a diff in pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
drabastomek commented on code in PR #14434:
URL: https://github.com/apache/arrow/pull/14434#discussion_r998550549
##########
cpp/src/arrow/engine/substrait/extension_set.cc:
##########
@@ -858,11 +858,27 @@ struct DefaultExtensionIdRegistry : ExtensionIdRegistryImpl {
// -------------- Substrait -> Arrow Functions -----------------
// Mappings with a _checked variant
- for (const auto& function_name : {"add", "subtract", "multiply", "divide"}) {
+ for (const auto& function_name : {"add", "subtract", "multiply", "divide", "sign",
+ "power", "sqrt", "exp", "abs"}) {
DCHECK_OK(
AddSubstraitCallToArrow({kSubstraitArithmeticFunctionsUri, function_name},
DecodeOptionlessOverflowableArithmetic(function_name)));
}
+
+ // Mappings for log functions
+ for (const auto& function_name : {"ln", "log10", "log2", "logb", "log1p"}) {
+ DCHECK_OK(
+ AddSubstraitCallToArrow({kSubstraitLogarithmicFunctionsUri, function_name},
+ DecodeOptionlessOverflowableArithmetic(function_name)));
+ }
+
+ // Mappings for rounding functions
+ for (const auto& function_name : {"ceil", "floor"}) {
+ DCHECK_OK(
+ AddSubstraitCallToArrow({kSubstraitRoundingFunctionsUri, function_name},
+ DecodeOptionlessOverflowableArithmetic(function_name)));
Review Comment:
I noticed that after. Updated to `DecodeOptionlessUncheckedArithmetic`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] drabastomek commented on a diff in pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
drabastomek commented on code in PR #14434:
URL: https://github.com/apache/arrow/pull/14434#discussion_r997719032
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -1717,6 +1717,12 @@ const FunctionDoc pow_doc{
"wraps around. If either base or exponent is null the result will be null."),
{"base", "exponent"}};
+const FunctionDoc exp_doc{
+ "Computes Euler's number raised to the specified power, element-wise",
+ ("Negative integer power returns an error. However, integer overflow\n"
Review Comment:
That was a placeholder from power. I updated it.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
westonpace commented on PR #14434:
URL: https://github.com/apache/arrow/pull/14434#issuecomment-1283026424
> It seems like a safe bet to me to accept both unspecified and TIE_TO_EVEN.
Worst case we find out when we get around to implementing Substrait function testing for this specific option :laughing: Nightmare fuel for @richtia
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #14434:
URL: https://github.com/apache/arrow/pull/14434#discussion_r1004234377
##########
cpp/src/arrow/compute/kernels/base_arithmetic_internal.h:
##########
@@ -485,6 +485,22 @@ struct NegateChecked {
}
};
+struct Exp {
+ template <typename T, typename Arg>
+ static constexpr enable_if_floating_value<Arg, T> Call(KernelContext*, Arg exp,
+ Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ return std::exp(exp);
+ }
+
+ template <typename T, typename Arg>
+ static constexpr enable_if_decimal_value<Arg, T> Call(KernelContext*, Arg exp,
+ Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ return exp.Exp();
Review Comment:
@drabastomek {{Decimal128::Exp}} doesn't exist, so this definition wouldn't compile.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rok commented on pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
rok commented on PR #14434:
URL: https://github.com/apache/arrow/pull/14434#issuecomment-1316872575
@pitrou I've agreed with @drabastomek to implement the review feedback. Could you please take another look?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] drabastomek commented on a diff in pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
drabastomek commented on code in PR #14434:
URL: https://github.com/apache/arrow/pull/14434#discussion_r997717129
##########
cpp/src/arrow/compute/kernels/base_arithmetic_internal.h:
##########
@@ -485,6 +485,34 @@ struct NegateChecked {
}
};
+struct Exp {
+ template <typename T, typename Arg>
+ static enable_if_floating_value<Arg, T> Call(KernelContext*, Arg exp, Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ return std::exp(exp);
+ }
+
+ template <typename T, typename Arg>
+ static enable_if_decimal_value<Arg, T> Call(KernelContext*, Arg exp, Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ return std::exp(exp);
+ }
+};
+
+struct ExpChecked {
Review Comment:
Good point. Removed the checks for `exp` and added `DecodeOptionlessUncheckedMapping` method to handle these cases. I tried to use the `DecodeOptionlessBasicMapping` but it was throwing that Acero needed the optional parameter at index 0.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jvanstraten commented on pull request #14434: [DRAFT] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
jvanstraten commented on PR #14434:
URL: https://github.com/apache/arrow/pull/14434#issuecomment-1280844214
General note that applies more broadly than just this PR: I don't think `DecodeOptionlessOverflowableArithmetic` is suitable for these functions, nor for division or for any of the floating-point versions of the functions it's already used for. The problem is that these functions all have different sets of options (integer overflow, floating point rounding, and/or domain error handling), while `DecodeOptionlessOverflowableArithmetic` only seems to handle the integer overflow option. In addition, it expects `<name>_checked` kernels to exist in addition to the normal kernels to handle the "throw an error on integer overflow" option. Also, Substrait doesn't even define functions that are generally only applied to floating point numbers (like `exp`) for integers at all. That's not necessarily a problem for consumption, though.
That being said, the way options are passed in Substrait is [currently being revised](https://github.com/substrait-io/substrait/pull/342), so I don't know how much value there is in fixing this short-term/maybe it's fine for now as a placeholder. I'm not at all up-to-date on planning for Acero-Substrait.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a diff in pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
westonpace commented on code in PR #14434:
URL: https://github.com/apache/arrow/pull/14434#discussion_r1002079597
##########
cpp/src/arrow/engine/substrait/extension_set.cc:
##########
@@ -858,11 +867,34 @@ struct DefaultExtensionIdRegistry : ExtensionIdRegistryImpl {
// -------------- Substrait -> Arrow Functions -----------------
// Mappings with a _checked variant
- for (const auto& function_name : {"add", "subtract", "multiply", "divide"}) {
+ for (const auto& function_name :
+ {"add", "subtract", "multiply", "divide", "power", "sqrt", "abs"}) {
Review Comment:
`sqrt` isn't currently defined as overflowable in Substrait.
##########
cpp/src/arrow/engine/substrait/extension_set.cc:
##########
@@ -858,11 +867,34 @@ struct DefaultExtensionIdRegistry : ExtensionIdRegistryImpl {
// -------------- Substrait -> Arrow Functions -----------------
// Mappings with a _checked variant
- for (const auto& function_name : {"add", "subtract", "multiply", "divide"}) {
+ for (const auto& function_name :
+ {"add", "subtract", "multiply", "divide", "power", "sqrt", "abs"}) {
DCHECK_OK(
AddSubstraitCallToArrow({kSubstraitArithmeticFunctionsUri, function_name},
DecodeOptionlessOverflowableArithmetic(function_name)));
}
+
+ // Mappings without a _checked variant
+ for (const auto& function_name : {"exp", "sign"}) {
+ DCHECK_OK(
+ AddSubstraitCallToArrow({kSubstraitArithmeticFunctionsUri, function_name},
+ DecodeOptionlessUncheckedArithmetic(function_name)));
+ }
+
+ // Mappings for log functions
+ for (const auto& function_name : {"ln", "log10", "log2", "logb", "log1p"}) {
+ DCHECK_OK(
+ AddSubstraitCallToArrow({kSubstraitLogarithmicFunctionsUri, function_name},
+ DecodeOptionlessOverflowableArithmetic(function_name)));
Review Comment:
The logrithmic functions are not technically overflowable either.
##########
cpp/src/arrow/engine/substrait/function_test.cc:
##########
@@ -177,6 +177,31 @@ TEST(FunctionMapping, ValidCases) {
{nullptr, int8(), int8()},
"0",
int8()},
+ {{kSubstraitArithmeticFunctionsUri, "sign"},
+ {"SILENT", "-1"},
Review Comment:
```suggestion
{"-1"},
```
`sign` does not have an overflow argument.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rok commented on a diff in pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
rok commented on code in PR #14434:
URL: https://github.com/apache/arrow/pull/14434#discussion_r1024417287
##########
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:
I'll admit it's not a terribly interesting case but still.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rok commented on pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
rok commented on PR #14434:
URL: https://github.com/apache/arrow/pull/14434#issuecomment-1315306708
@drabastomek can I help move this forward?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rok commented on pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
rok commented on PR #14434:
URL: https://github.com/apache/arrow/pull/14434#issuecomment-1318708282
@pitrou do you think this needs another pass?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #14434:
URL: https://github.com/apache/arrow/pull/14434#discussion_r1024094633
##########
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:
I don't know, does `exp` actually accept integers?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jvanstraten commented on pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
jvanstraten commented on PR #14434:
URL: https://github.com/apache/arrow/pull/14434#issuecomment-1282621073
> I'm not sure what rounding behavior we have implemented in Acero and therefore the safest thing to do would be to reject all plans that are asking for a specific rounding behavior.
Assuming that the arithmetic operations are all ultimately implemented using standard library operations, you can query the current mode with [`fegetround()`](https://en.cppreference.com/w/cpp/numeric/fenv/feround) (it's a global processor state thing). But unless an Arrow user goes out of their way to change the mode and this somehow propagates to all threads correctly, it's just going to be `FE_TONEAREST`, which corresponds with Substrait `TIE_TO_EVEN` (`TIE_AWAY_FROM_ZERO` does not seem to be implemented by the standard library, the rest of the mapping should be fairly straightforward).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rok commented on a diff in pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
rok commented on code in PR #14434:
URL: https://github.com/apache/arrow/pull/14434#discussion_r1023328835
##########
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:
Wouldn't integer be a valid input type and hence input type remains `Numeric`? `Float32/Float64` as output makes sense.
This is out of scope for this PR, but how about `complex` values ([example](https://numpy.org/devdocs/reference/generated/numpy.exp.html))? Should Acero kernels support complex? What about Substrait?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rok commented on a diff in pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
rok commented on code in PR #14434:
URL: https://github.com/apache/arrow/pull/14434#discussion_r1023886977
##########
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:
I went with your suggestion, but I think we should probably still specify integers?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] drabastomek commented on pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
drabastomek commented on PR #14434:
URL: https://github.com/apache/arrow/pull/14434#issuecomment-1320417526
Thank you all for the support with this!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rok commented on a diff in pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
rok commented on code in PR #14434:
URL: https://github.com/apache/arrow/pull/14434#discussion_r1023888727
##########
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:
Perhaps `AddDecimalUnaryKernels<Exp>(exp.get());` could be added here? But since `Decimal128::Exp` doesn't exist we want implicit cast and keep `MakeUnaryArithmeticFunctionFloatingPoint`?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rok commented on a diff in pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
rok commented on code in PR #14434:
URL: https://github.com/apache/arrow/pull/14434#discussion_r1024291581
##########
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:
As per [cppreference](https://en.cppreference.com/w/cpp/numeric/math/exp) it does since c++11.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] drabastomek commented on a diff in pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
drabastomek commented on code in PR #14434:
URL: https://github.com/apache/arrow/pull/14434#discussion_r1023078026
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc:
##########
@@ -1519,6 +1519,65 @@ TEST_F(TestUnaryArithmeticDecimal, AbsoluteValue) {
}
}
+TYPED_TEST(TestUnaryArithmeticFloating, Exp) {
+ using CType = typename TestFixture::CType;
+
+ auto min = std::numeric_limits<CType>::lowest();
+ auto max = std::numeric_limits<CType>::max();
+
+ auto exp = [](const Datum& arg, ArithmeticOptions, ExecContext* ctx) {
+ return Exp(arg, ctx);
+ };
+ // Empty arrays
+ this->AssertUnaryOp(exp, "[]", "[]");
+ // Array with nulls
+ this->AssertUnaryOp(exp, "[null]", "[null]");
+ this->AssertUnaryOp(exp, this->MakeNullScalar(), this->MakeNullScalar());
+ this->AssertUnaryOp(exp, "[-1.0, null, 10.0]",
+ "[0.36787944117144233, null, 22026.465794806718]");
+ // Ordinary arrays (positive, negative, fractional, and zero inputs)
+ this->AssertUnaryOp(
+ exp, "[-10.0, 0, 0.5, 1.0]",
+ "[0.000045399929762484854,1.0,1.6487212707001282,2.718281828459045]");
+ this->AssertUnaryOp(exp, 1.3F, 3.6692964926535487F);
+ this->AssertUnaryOp(exp, this->MakeScalar(1.3F), this->MakeScalar(3.6692964926535487F));
+ // Arrays with infinites
+ this->AssertUnaryOp(exp, "[-Inf, Inf]", "[0, Inf]");
+ // Arrays with NaNs
+ this->SetNansEqual(true);
+ this->AssertUnaryOp(exp, "[NaN]", "[NaN]");
+ this->AssertUnaryOp(exp, "[NaN]", "[NaN]");
+ this->AssertUnaryOp(exp, "[NaN]", "[NaN]");
Review Comment:
Updated the docs.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] drabastomek commented on a diff in pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
drabastomek commented on code in PR #14434:
URL: https://github.com/apache/arrow/pull/14434#discussion_r999826159
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -2224,6 +2230,15 @@ 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:
Tests added.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
westonpace commented on PR #14434:
URL: https://github.com/apache/arrow/pull/14434#issuecomment-1281604803
> I don't think DecodeOptionlessOverflowableArithmetic is suitable for these functions, nor for division or for any of the floating-point versions of the functions it's already used for.
At the moment we should focus on exposing the capability that we have, and describing it accurately. For example, in Arrow we have "_checked" but "what exactly is checked?" For example, in Arrow we have `divide` and `divide_checked`. They seem to behave as follows (through trial and error):
| Method | Data Type | Problem | Behavior |
| --- | --- | --- | --- |
| divide | integral | overflow | silent |
| divide | integral | divide by zero | error |
| divide | integral | domain error | error |
| divide | floating | divide by zero | limit |
| divide | floating | domain error | nan |
| divide_checked | integral | overflow | error |
| divide_checked | integral | divide by zero | error |
| divide_checked | integral | domain error | error |
| divide_checked | floating | divide by zero | error |
| divide_checked | floating | domain error | error |
Thus we should have the following behavior for substrait calls
| Call | Data Type | Options | Arrow Result |
| --- | --- | --- | --- |
| divide | integral | overflow=SILENT | divide |
| divide | integral | overflow=SATURATE | reject plan |
| divide | integral | overflow=ERROR | divide_checked |
| divide | floating | rounding=* | reject plan[1] |
| divide | floating | on_domain_error=NAN | divide |
| divide | floating | on_domain_error=ERROR | divide_checked |
| divide | floating | on_division_by_zero=LIMIT | divide |
| divide | floating | on_division_by_zero=NAN | reject plan |
| divide | floating | on_division_by_zero=ERROR | divide_checked |
| divide | floating | on_domain_error=NAN on_division_by_zero=ERROR | reject plan[2] |
| divide | floating | on_domain_error=ERROR on_division_by_zero=LIMIT | reject plan[2] |
1. I'm not sure what rounding behavior we have implemented in Acero and therefore the safest thing to do would be to reject all plans that are asking for a specific rounding behavior.
2. We cannot satisfy these specific combinations between divide and divide_checked.
Caveat / Needs clarification:
IMHO, It is not clear, in Substrait, if division by zero for integral arguments counts as overflow or not. Also, it is possible to have 0/0 in integer arithmetic, which I have been interpreting as "domain error" (technically division by zero requires non-zero/0) but have no idea what the integral behavior should be.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] drabastomek commented on a diff in pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
drabastomek commented on code in PR #14434:
URL: https://github.com/apache/arrow/pull/14434#discussion_r1003987511
##########
cpp/src/arrow/engine/substrait/function_test.cc:
##########
@@ -177,6 +177,31 @@ TEST(FunctionMapping, ValidCases) {
{nullptr, int8(), int8()},
"0",
int8()},
+ {{kSubstraitArithmeticFunctionsUri, "sign"},
+ {"SILENT", "-1"},
Review Comment:
Changed. Thanks for catching this.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] drabastomek commented on a diff in pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
drabastomek commented on code in PR #14434:
URL: https://github.com/apache/arrow/pull/14434#discussion_r1008259756
##########
cpp/src/arrow/compute/kernels/base_arithmetic_internal.h:
##########
@@ -485,6 +485,22 @@ struct NegateChecked {
}
};
+struct Exp {
+ template <typename T, typename Arg>
+ static constexpr enable_if_floating_value<Arg, T> Call(KernelContext*, Arg exp,
+ Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ return std::exp(exp);
+ }
+
+ template <typename T, typename Arg>
+ static constexpr enable_if_decimal_value<Arg, T> Call(KernelContext*, Arg exp,
+ Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ return exp.Exp();
Review Comment:
Ah... I see. Ok, but the Substrait still can call Acero with a decimal value. I understand it'd be converted to float first to do the `Exp` but would we rather error on this or keep it? I'm happy to remove this and the relevant test just thinking out loud.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] drabastomek commented on pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
drabastomek commented on PR #14434:
URL: https://github.com/apache/arrow/pull/14434#issuecomment-1281368219
Thanks @jvanstraten. You're right about the `exp` not being defined for ints - I changed the calls and added `_checked` method too.
I agree that currently all the `_checked` methods only check for int overflows and it might be something we might want to address at some point. Not sure if this should be a part of this PR tho.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] drabastomek commented on a diff in pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
drabastomek commented on code in PR #14434:
URL: https://github.com/apache/arrow/pull/14434#discussion_r997717129
##########
cpp/src/arrow/compute/kernels/base_arithmetic_internal.h:
##########
@@ -485,6 +485,34 @@ struct NegateChecked {
}
};
+struct Exp {
+ template <typename T, typename Arg>
+ static enable_if_floating_value<Arg, T> Call(KernelContext*, Arg exp, Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ return std::exp(exp);
+ }
+
+ template <typename T, typename Arg>
+ static enable_if_decimal_value<Arg, T> Call(KernelContext*, Arg exp, Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ return std::exp(exp);
+ }
+};
+
+struct ExpChecked {
Review Comment:
Good point. Removed the checks for `exp` and added `DecodeOptionlessUncheckedMapping` method to handle these cases.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rok commented on a diff in pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
rok commented on code in PR #14434:
URL: https://github.com/apache/arrow/pull/14434#discussion_r1024323342
##########
cpp/src/arrow/engine/substrait/function_test.cc:
##########
@@ -313,7 +330,18 @@ TEST(FunctionMapping, ValidCases) {
{"abc", "def"},
{utf8(), utf8()},
"abcdef",
- utf8()}};
+ utf8()},
+ {{kSubstraitLogarithmicFunctionsUri, "ln"}, {"1"}, {int8()}, "0", float64()},
+ {{kSubstraitLogarithmicFunctionsUri, "log10"}, {"1"}, {int8()}, "0", float64()},
+ {{kSubstraitLogarithmicFunctionsUri, "log2"}, {"2"}, {int8()}, "1", float64()},
+ {{kSubstraitLogarithmicFunctionsUri, "log1p"}, {"0"}, {int8()}, "0", float64()},
+ {{kSubstraitLogarithmicFunctionsUri, "logb"},
Review Comment:
Agreed. I've changed the `exp(0)`-like cases to be more discriminating.
Also note the comment above:
```
// These are not meant to be an exhaustive test of Substrait
// conformance. Instead, we should test just enough to ensure
// we are mapping to the correct function
TEST(FunctionMapping, ValidCases) {
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rok commented on a diff in pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
rok commented on code in PR #14434:
URL: https://github.com/apache/arrow/pull/14434#discussion_r1024421590
##########
docs/source/cpp/compute.rst:
##########
@@ -435,43 +435,45 @@ floating-point arguments will cast all arguments to floating-point, while mixed
decimal and integer arguments will cast all arguments to decimals.
Mixed time resolution temporal inputs will be cast to finest input resolution.
-+------------------+--------+------------------+----------------------+-------+
-| Function name | Arity | Input types | Output type | Notes |
-+==================+========+==================+======================+=======+
-| abs | Unary | Numeric | Numeric | |
-+------------------+--------+------------------+----------------------+-------+
-| abs_checked | Unary | Numeric | Numeric | |
-+------------------+--------+------------------+----------------------+-------+
-| add | Binary | Numeric/Temporal | Numeric/Temporal | \(1) |
-+------------------+--------+------------------+----------------------+-------+
-| add_checked | Binary | Numeric/Temporal | Numeric/Temporal | \(1) |
-+------------------+--------+------------------+----------------------+-------+
-| divide | Binary | Numeric/Temporal | Numeric/Temporal | \(1) |
-+------------------+--------+------------------+----------------------+-------+
-| divide_checked | Binary | Numeric/Temporal | Numeric/Temporal | \(1) |
-+------------------+--------+------------------+----------------------+-------+
-| multiply | Binary | Numeric/Temporal | Numeric/Temporal | \(1) |
-+------------------+--------+------------------+----------------------+-------+
-| multiply_checked | Binary | Numeric/Temporal | Numeric/Temporal | \(1) |
-+------------------+--------+------------------+----------------------+-------+
-| negate | Unary | Numeric | Numeric | |
-+------------------+--------+------------------+----------------------+-------+
-| negate_checked | Unary | Signed Numeric | Signed Numeric | |
-+------------------+--------+------------------+----------------------+-------+
-| power | Binary | Numeric | Numeric | |
-+------------------+--------+------------------+----------------------+-------+
-| power_checked | Binary | Numeric | Numeric | |
-+------------------+--------+------------------+----------------------+-------+
-| sign | Unary | Numeric | Int8/Float32/Float64 | \(2) |
-+------------------+--------+------------------+----------------------+-------+
-| sqrt | Unary | Numeric | Numeric | |
-+------------------+--------+------------------+----------------------+-------+
-| sqrt_checked | Unary | Numeric | Numeric | |
-+------------------+--------+------------------+----------------------+-------+
-| subtract | Binary | Numeric/Temporal | Numeric/Temporal | \(1) |
-+------------------+--------+------------------+----------------------+-------+
-| subtract_checked | Binary | Numeric/Temporal | Numeric/Temporal | \(1) |
-+------------------+--------+------------------+----------------------+-------+
++------------------+--------+-------------------------+----------------------+-------+
+| Function name | Arity | Input types | Output type | Notes |
++==================+========+=========================+======================+=======+
+| abs | Unary | Numeric | Numeric | |
++------------------+--------+-------------------------+----------------------+-------+
+| abs_checked | Unary | Numeric | Numeric | |
++------------------+--------+-------------------------+----------------------+-------+
+| add | Binary | Numeric/Temporal | Numeric/Temporal | \(1) |
++------------------+--------+-------------------------+----------------------+-------+
+| add_checked | Binary | Numeric/Temporal | Numeric/Temporal | \(1) |
++------------------+--------+-------------------------+----------------------+-------+
+| divide | Binary | Numeric/Temporal | Numeric/Temporal | \(1) |
++------------------+--------+-------------------------+----------------------+-------+
+| divide_checked | Binary | Numeric/Temporal | Numeric/Temporal | \(1) |
++------------------+--------+-------------------------+----------------------+-------+
+| exp | Unary | Float32/Float64/Decimal | Float32/Float64 | |
Review Comment:
```suggestion
| exp | Unary | Numeric | Float32/Float64 | |
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #14434:
URL: https://github.com/apache/arrow/pull/14434#issuecomment-1319244423
Benchmark runs are scheduled for baseline = 0f87e6bf89211a989e4b77990b35d157655ea1a8 and contender = b5b0282516e04fd27cf59f3c05e849967d997cb4. b5b0282516e04fd27cf59f3c05e849967d997cb4 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Failed :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/e9e72d4aa21c4f539e1a4591311e5f85...6d3a95b3d1624a5ba0d92176f07548da/)
[Failed :arrow_down:0.74% :arrow_up:0.0%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/ea98491bfd8b444b8792697f8b101d38...650ebded1e2a4d67a9cae852d6cca632/)
[Failed :arrow_down:0.0% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/16ef84fde7e54330943dd5ca50df6cb1...d1c8711a83924e1794b410aa75f3e983/)
[Failed :arrow_down:0.07% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/bca46eddfb9b431188d63d83038ecd39...20b3fd4006d144b4afe55b7e54e925e5/)
Buildkite builds:
[Finished] [`b5b02825` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/1888)
[Failed] [`b5b02825` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/1910)
[Failed] [`b5b02825` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/1879)
[Failed] [`b5b02825` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/1902)
[Failed] [`0f87e6bf` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/1887)
[Finished] [`0f87e6bf` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/1909)
[Finished] [`0f87e6bf` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/1878)
[Finished] [`0f87e6bf` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/1901)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rok commented on pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
rok commented on PR #14434:
URL: https://github.com/apache/arrow/pull/14434#issuecomment-1318792580
Nice! Thanks @drabastomek & all!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rok commented on a diff in pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
rok commented on code in PR #14434:
URL: https://github.com/apache/arrow/pull/14434#discussion_r1025299474
##########
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:
Integer support for logarithms feels more important than here. But I suppose implicit casting will cover the gap if needed? Shall I remove the integer support then?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rok commented on a diff in pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero
Posted by GitBox <gi...@apache.org>.
rok commented on code in PR #14434:
URL: https://github.com/apache/arrow/pull/14434#discussion_r1025301610
##########
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:
Oh we're using `MakeUnaryArithmeticFunctionFloatingPoint` we can't remove it anyway.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org