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 2021/04/20 14:56:58 UTC
[GitHub] [arrow] edponce opened a new pull request #10113: ARROW-11950: [C++][Compute] Add unary negative kernel
edponce opened a new pull request #10113:
URL: https://github.com/apache/arrow/pull/10113
This pull request adds the Negate arithmetic compute kernel for integral and floating-point types. The NegateChecked version is not implemented for unsigned integral types and overflow behavior is consistent with equivalent Add/Subtract operations. The Negate kernels are registered as "negate" and "negate_checked".
This PR also extends support for unary arithmetic compute kernels and tests.
@bkietz please review
--
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:
users@infra.apache.org
[GitHub] [arrow] edponce commented on a change in pull request #10113: ARROW-11950: [C++][Compute] Add unary negative kernel
Posted by GitBox <gi...@apache.org>.
edponce commented on a change in pull request #10113:
URL: https://github.com/apache/arrow/pull/10113#discussion_r617806201
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -369,12 +420,37 @@ std::shared_ptr<ScalarFunction> MakeArithmeticFunctionNotNull(std::string name,
const FunctionDoc* doc) {
auto func = std::make_shared<ArithmeticFunction>(name, Arity::Binary(), doc);
for (const auto& ty : NumericTypes()) {
- auto exec = NumericEqualTypesBinary<ScalarBinaryNotNullEqualTypes, Op>(ty);
+ auto exec = ArithmeticExecFromOp<ScalarBinaryNotNullEqualTypes, Op>(ty);
DCHECK_OK(func->AddKernel({ty, ty}, ty, exec));
}
return func;
}
+template <typename Op>
+std::shared_ptr<ScalarFunction> MakeUnaryArithmeticFunction(std::string name,
+ const FunctionDoc* doc) {
+ auto func = std::make_shared<ArithmeticFunction>(name, Arity::Unary(), doc);
+ for (const auto& ty : NumericTypes()) {
+ auto exec = ArithmeticExecFromOp<ScalarUnary, Op>(ty);
+ DCHECK_OK(func->AddKernel({ty}, ty, exec));
+ }
+ return func;
+}
+
+// Like MakeUnaryArithmeticFunction, but for signed arithmetic ops that need to run
+// only on non-null output.
+template <typename Op>
+std::shared_ptr<ScalarFunction> MakeUnaryCheckedSignedArithmeticFunctionNotNull(
+ std::string name, const FunctionDoc* doc) {
+ auto func = std::make_shared<ArithmeticFunction>(name, Arity::Unary(), doc);
+ for (const auto& ty : arrow::internal::FlattenVectors<std::shared_ptr<arrow::DataType>>(
+ {SignedIntTypes(), FloatingPointTypes()})) {
Review comment:
Also, I noticed that there are two variants of "is_unsigned_integer" one in *type_traits.h* which is the one you reference and a templated alias in *scalar_arithmetic.cc*, so I included "arrow/type_traits.h" and changed invocation to *arrow::is_unsigned_integer(ty->id())*
--
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:
users@infra.apache.org
[GitHub] [arrow] edponce commented on a change in pull request #10113: ARROW-11950: [C++][Compute] Add unary negative kernel
Posted by GitBox <gi...@apache.org>.
edponce commented on a change in pull request #10113:
URL: https://github.com/apache/arrow/pull/10113#discussion_r621541936
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -235,6 +239,49 @@ struct DivideChecked {
}
};
+struct Negate {
+ template <typename T, typename Arg>
+ static constexpr enable_if_floating_point<T> Call(KernelContext*, Arg arg) {
+ return -arg;
+ }
+
+ template <typename T, typename Arg>
+ static constexpr enable_if_unsigned_integer<T> Call(KernelContext*, Arg arg) {
+ return ~arg + 1;
+ }
+
+ template <typename T, typename Arg>
+ static constexpr enable_if_signed_integer<T> Call(KernelContext*, Arg arg) {
+ return arrow::internal::SafeSignedNegate(arg);
+ }
+};
+
+struct NegateChecked {
+ template <typename T, typename Arg>
+ static enable_if_signed_integer<T> Call(KernelContext* ctx, Arg arg) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ T result = 0;
+ if (ARROW_PREDICT_FALSE(NegateWithOverflow(arg, &result))) {
+ ctx->SetStatus(Status::Invalid("overflow"));
Review comment:
Thanks for this notice.
--
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:
users@infra.apache.org
[GitHub] [arrow] edponce commented on a change in pull request #10113: ARROW-11950: [C++][Compute] Add unary negative kernel
Posted by GitBox <gi...@apache.org>.
edponce commented on a change in pull request #10113:
URL: https://github.com/apache/arrow/pull/10113#discussion_r617798895
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -369,12 +420,37 @@ std::shared_ptr<ScalarFunction> MakeArithmeticFunctionNotNull(std::string name,
const FunctionDoc* doc) {
auto func = std::make_shared<ArithmeticFunction>(name, Arity::Binary(), doc);
for (const auto& ty : NumericTypes()) {
- auto exec = NumericEqualTypesBinary<ScalarBinaryNotNullEqualTypes, Op>(ty);
+ auto exec = ArithmeticExecFromOp<ScalarBinaryNotNullEqualTypes, Op>(ty);
DCHECK_OK(func->AddKernel({ty, ty}, ty, exec));
}
return func;
}
+template <typename Op>
+std::shared_ptr<ScalarFunction> MakeUnaryArithmeticFunction(std::string name,
+ const FunctionDoc* doc) {
+ auto func = std::make_shared<ArithmeticFunction>(name, Arity::Unary(), doc);
+ for (const auto& ty : NumericTypes()) {
+ auto exec = ArithmeticExecFromOp<ScalarUnary, Op>(ty);
+ DCHECK_OK(func->AddKernel({ty}, ty, exec));
+ }
+ return func;
+}
+
+// Like MakeUnaryArithmeticFunction, but for signed arithmetic ops that need to run
+// only on non-null output.
+template <typename Op>
+std::shared_ptr<ScalarFunction> MakeUnaryCheckedSignedArithmeticFunctionNotNull(
+ std::string name, const FunctionDoc* doc) {
+ auto func = std::make_shared<ArithmeticFunction>(name, Arity::Unary(), doc);
+ for (const auto& ty : arrow::internal::FlattenVectors<std::shared_ptr<arrow::DataType>>(
+ {SignedIntTypes(), FloatingPointTypes()})) {
Review comment:
To make this generator more general, I suggest to remove "Checked" from its name and to not include the "negate_checked" comment:
```
std::shared_ptr<ScalarFunction> MakeUnarySignedArithmeticFunctionNotNull(...) {
auto func = ...
for (const auto& ty : NumericTypes()) {
if (!is_unsigned_integer(ty->id())) {
auto exec = ...
...
}
}
return func;
}
```
--
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:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #10113: ARROW-11950: [C++][Compute] Add unary negative kernel
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #10113:
URL: https://github.com/apache/arrow/pull/10113#discussion_r617671811
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -817,5 +933,126 @@ TEST(TestBinaryArithmetic, AddWithImplicitCastsUint64EdgeCase) {
ArrayFromJSON(uint64(), "[18446744073709551615]")}));
}
+TEST(TestUnaryArithmetic, DispatchBest) {
+ for (std::string name : {"negate"}) {
+ for (const auto& ty : {int8(), int16(), int32(), int64(), uint8(), uint16(), uint32(),
+ uint64(), float32(), float64()}) {
+ CheckDispatchBest(name, {ty}, {ty});
+ CheckDispatchBest(name, {dictionary(int8(), ty)}, {ty});
+ }
+ }
+
+ for (std::string name : {"negate_checked"}) {
+ for (const auto& ty : {int8(), int16(), int32(), int64(), float32(), float64()}) {
+ CheckDispatchBest(name, {ty}, {ty});
+ CheckDispatchBest(name, {dictionary(int8(), ty)}, {ty});
+ }
+ }
+}
+
+TYPED_TEST(TestUnaryArithmeticSigned, Negate) {
+ using CType = typename TestFixture::CType;
+
+ auto min = std::numeric_limits<CType>::min();
+ auto max = std::numeric_limits<CType>::max();
+
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ // Empty arrays
+ this->AssertUnaryOp(Negate, "[]", "[]");
+ // Array with nulls
+ this->AssertUnaryOp(Negate, "[null]", "[null]");
+ this->AssertUnaryOp(Negate, this->MakeNullScalar(), "[null]");
Review comment:
```suggestion
this->AssertUnaryOp(Negate, this->MakeNullScalar(), this->MakeNullScalar());
```
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -54,6 +54,117 @@ std::shared_ptr<Array> TweakValidityBit(const std::shared_ptr<Array>& array,
return MakeArray(data);
}
+template <typename T>
+class TestUnaryArithmetic : public TestBase {
+ protected:
+ using ArrowType = T;
+ using CType = typename ArrowType::c_type;
+
+ static std::shared_ptr<DataType> type_singleton() {
+ return TypeTraits<ArrowType>::type_singleton();
+ }
+
+ using UnaryFunction =
+ std::function<Result<Datum>(const Datum&, ArithmeticOptions, ExecContext*)>;
+
+ void SetUp() override { options_.check_overflow = false; }
+
+ std::shared_ptr<Scalar> MakeNullScalar() {
+ return arrow::MakeNullScalar(type_singleton());
+ }
+
+ std::shared_ptr<Scalar> MakeScalar(CType value) {
+ return *arrow::MakeScalar(type_singleton(), value);
+ }
+
+ // (Scalar)
+ void AssertUnaryOp(UnaryFunction func, CType argument, CType expected) {
+ auto arg = MakeScalar(argument);
+ auto exp = MakeScalar(expected);
+ ASSERT_OK_AND_ASSIGN(auto actual, func(arg, options_, nullptr));
+ AssertScalarsApproxEqual(*exp, *actual.scalar(), /*verbose=*/true);
+ }
+
+ // (Scalar)
+ void AssertUnaryOp(UnaryFunction func, const std::shared_ptr<Scalar>& arg,
+ const std::string& expected) {
+ auto exp = ArrayFromJSON(type_singleton(), expected);
+ ASSERT_OK_AND_ASSIGN(auto actual, func(arg, options_, nullptr));
+ AssertScalarsApproxEqual(*(*exp->GetScalar(0)), *actual.scalar(), /*verbose=*/true);
Review comment:
Instead of going through JSON, please just provide scalars
```suggestion
const std::shared_ptr<Scalar>& expected) {
ASSERT_OK_AND_ASSIGN(auto actual, func(arg, options_, nullptr));
AssertScalarsApproxEqual(*expected, *actual.scalar(), /*verbose=*/true);
```
##########
File path: docs/source/cpp/compute.rst
##########
@@ -268,14 +268,18 @@ an ``Invalid`` :class:`Status` when overflow is detected.
+--------------------------+------------+--------------------+---------------------+
| divide_checked | Binary | Numeric | Numeric |
+--------------------------+------------+--------------------+---------------------+
-| power | Binary | Numeric | Numeric |
-+--------------------------+------------+--------------------+---------------------+
-| power_checked | Binary | Numeric | Numeric |
-+--------------------------+------------+--------------------+---------------------+
| multiply | Binary | Numeric | Numeric |
+--------------------------+------------+--------------------+---------------------+
| multiply_checked | Binary | Numeric | Numeric |
+--------------------------+------------+--------------------+---------------------+
+| negate | Unary | Numeric | Numeric |
++--------------------------+------------+--------------------+---------------------+
+| negate_checked | Unary | Numeric | Numeric |
Review comment:
```suggestion
| negate_checked | Unary | Signed Numeric | Signed Numeric |
```
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -369,12 +420,37 @@ std::shared_ptr<ScalarFunction> MakeArithmeticFunctionNotNull(std::string name,
const FunctionDoc* doc) {
auto func = std::make_shared<ArithmeticFunction>(name, Arity::Binary(), doc);
for (const auto& ty : NumericTypes()) {
- auto exec = NumericEqualTypesBinary<ScalarBinaryNotNullEqualTypes, Op>(ty);
+ auto exec = ArithmeticExecFromOp<ScalarBinaryNotNullEqualTypes, Op>(ty);
DCHECK_OK(func->AddKernel({ty, ty}, ty, exec));
}
return func;
}
+template <typename Op>
+std::shared_ptr<ScalarFunction> MakeUnaryArithmeticFunction(std::string name,
+ const FunctionDoc* doc) {
+ auto func = std::make_shared<ArithmeticFunction>(name, Arity::Unary(), doc);
+ for (const auto& ty : NumericTypes()) {
+ auto exec = ArithmeticExecFromOp<ScalarUnary, Op>(ty);
+ DCHECK_OK(func->AddKernel({ty}, ty, exec));
+ }
+ return func;
+}
+
+// Like MakeUnaryArithmeticFunction, but for signed arithmetic ops that need to run
+// only on non-null output.
+template <typename Op>
+std::shared_ptr<ScalarFunction> MakeUnaryCheckedSignedArithmeticFunctionNotNull(
+ std::string name, const FunctionDoc* doc) {
+ auto func = std::make_shared<ArithmeticFunction>(name, Arity::Unary(), doc);
+ for (const auto& ty : arrow::internal::FlattenVectors<std::shared_ptr<arrow::DataType>>(
+ {SignedIntTypes(), FloatingPointTypes()})) {
Review comment:
Slightly simpler:
```suggestion
for (const auto& ty : NumericTypes()) {
if (is_unsigned_integer(ty->id())) {
// don't support negate_checked for unsigned integers
continue;
}
```
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -817,5 +933,126 @@ TEST(TestBinaryArithmetic, AddWithImplicitCastsUint64EdgeCase) {
ArrayFromJSON(uint64(), "[18446744073709551615]")}));
}
+TEST(TestUnaryArithmetic, DispatchBest) {
+ for (std::string name : {"negate"}) {
+ for (const auto& ty : {int8(), int16(), int32(), int64(), uint8(), uint16(), uint32(),
+ uint64(), float32(), float64()}) {
+ CheckDispatchBest(name, {ty}, {ty});
+ CheckDispatchBest(name, {dictionary(int8(), ty)}, {ty});
+ }
+ }
+
+ for (std::string name : {"negate_checked"}) {
+ for (const auto& ty : {int8(), int16(), int32(), int64(), float32(), float64()}) {
+ CheckDispatchBest(name, {ty}, {ty});
+ CheckDispatchBest(name, {dictionary(int8(), ty)}, {ty});
+ }
+ }
+}
+
+TYPED_TEST(TestUnaryArithmeticSigned, Negate) {
+ using CType = typename TestFixture::CType;
+
+ auto min = std::numeric_limits<CType>::min();
+ auto max = std::numeric_limits<CType>::max();
+
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ // Empty arrays
+ this->AssertUnaryOp(Negate, "[]", "[]");
+ // Array with nulls
+ this->AssertUnaryOp(Negate, "[null]", "[null]");
+ this->AssertUnaryOp(Negate, this->MakeNullScalar(), "[null]");
+ this->AssertUnaryOp(Negate, "[1, null, -10]", "[-1, null, 10]");
+ // Arrays with zeros
+ this->AssertUnaryOp(Negate, "[0, 0, -0]", "[0, -0, 0]");
+ this->AssertUnaryOp(Negate, 0, -0);
+ this->AssertUnaryOp(Negate, -0, 0);
+ this->AssertUnaryOp(Negate, 0, 0);
+ // Ordinary arrays (positive inputs)
+ this->AssertUnaryOp(Negate, "[1, 10, 127]", "[-1, -10, -127]");
+ this->AssertUnaryOp(Negate, 1, -1);
+ this->AssertUnaryOp(Negate, this->MakeScalar(1), "[-1]");
Review comment:
```suggestion
this->AssertUnaryOp(Negate, this->MakeScalar(1), this->MakeScalar(-1));
```
--
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:
users@infra.apache.org
[GitHub] [arrow] edponce commented on a change in pull request #10113: ARROW-11950: [C++][Compute] Add unary negative kernel
Posted by GitBox <gi...@apache.org>.
edponce commented on a change in pull request #10113:
URL: https://github.com/apache/arrow/pull/10113#discussion_r621567410
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -817,5 +932,126 @@ TEST(TestBinaryArithmetic, AddWithImplicitCastsUint64EdgeCase) {
ArrayFromJSON(uint64(), "[18446744073709551615]")}));
}
+TEST(TestUnaryArithmetic, DispatchBest) {
+ for (std::string name : {"negate"}) {
+ for (const auto& ty : {int8(), int16(), int32(), int64(), uint8(), uint16(), uint32(),
+ uint64(), float32(), float64()}) {
+ CheckDispatchBest(name, {ty}, {ty});
+ CheckDispatchBest(name, {dictionary(int8(), ty)}, {ty});
+ }
+ }
+
Review comment:
I did not add `CheckScalarUnary("negate", std::make_shared<NullArray>(10), std::make_shared<NullArray>(10))` test because it fails with `NotImplemented` kernel via a different path of functions. Specifically, `CheckScalar()` --> `CallFunction()` which is where the error is captured. I think adding a family of `CheckScalar*Fails()` function would be beneficial for testing but consider it outside the scope of this PR. Should I open an issue for extending these 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #10113: ARROW-11950: [C++][Compute] Add unary negative kernel
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #10113:
URL: https://github.com/apache/arrow/pull/10113#discussion_r621259131
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -817,5 +932,126 @@ TEST(TestBinaryArithmetic, AddWithImplicitCastsUint64EdgeCase) {
ArrayFromJSON(uint64(), "[18446744073709551615]")}));
}
+TEST(TestUnaryArithmetic, DispatchBest) {
+ for (std::string name : {"negate"}) {
+ for (const auto& ty : {int8(), int16(), int32(), int64(), uint8(), uint16(), uint32(),
+ uint64(), float32(), float64()}) {
+ CheckDispatchBest(name, {ty}, {ty});
+ CheckDispatchBest(name, {dictionary(int8(), ty)}, {ty});
+ }
+ }
+
Review comment:
You could add a `CheckDispatchFails(function_name, argument_types)` assert function to go with `CheckDispatchBest()`
--
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:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #10113: ARROW-11950: [C++][Compute] Add unary negative kernel
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #10113:
URL: https://github.com/apache/arrow/pull/10113#discussion_r620393593
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -817,5 +932,126 @@ TEST(TestBinaryArithmetic, AddWithImplicitCastsUint64EdgeCase) {
ArrayFromJSON(uint64(), "[18446744073709551615]")}));
}
+TEST(TestUnaryArithmetic, DispatchBest) {
+ for (std::string name : {"negate"}) {
+ for (const auto& ty : {int8(), int16(), int32(), int64(), uint8(), uint16(), uint32(),
+ uint64(), float32(), float64()}) {
+ CheckDispatchBest(name, {ty}, {ty});
+ CheckDispatchBest(name, {dictionary(int8(), ty)}, {ty});
+ }
+ }
+
Review comment:
Please add a test asserting the behavior when `NullType` is used. I expect this will emit a "no kernel found" error (acceptable), or leave the type unpromoted:
```suggestion
CheckDispatchBest("negate", {null()}, {null()});
```
This would also be fine, but I'd like to see a trivial test case asserting we don't try to access absent buffers:
```c++
CheckScalarUnary("negate", NullArray(10), NullArray(10));
```
--
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:
users@infra.apache.org
[GitHub] [arrow] edponce commented on a change in pull request #10113: ARROW-11950: [C++][Compute] Add unary negative kernel
Posted by GitBox <gi...@apache.org>.
edponce commented on a change in pull request #10113:
URL: https://github.com/apache/arrow/pull/10113#discussion_r620610187
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -817,5 +932,126 @@ TEST(TestBinaryArithmetic, AddWithImplicitCastsUint64EdgeCase) {
ArrayFromJSON(uint64(), "[18446744073709551615]")}));
}
+TEST(TestUnaryArithmetic, DispatchBest) {
+ for (std::string name : {"negate"}) {
+ for (const auto& ty : {int8(), int16(), int32(), int64(), uint8(), uint16(), uint32(),
+ uint64(), float32(), float64()}) {
+ CheckDispatchBest(name, {ty}, {ty});
+ CheckDispatchBest(name, {dictionary(int8(), ty)}, {ty});
+ }
+ }
+
Review comment:
Using a `NullType` does trigger a `NotImplemented` error as expected, but I can't catch/assert the error from `CheckDispatchBest()` to pass the tests. Similar scenario for `CheckScalarUnary`. Any ideas?
--
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:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #10113: ARROW-11950: [C++][Compute] Add unary negative kernel
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10113:
URL: https://github.com/apache/arrow/pull/10113#issuecomment-823343200
https://issues.apache.org/jira/browse/ARROW-11950
--
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:
users@infra.apache.org
[GitHub] [arrow] edponce commented on pull request #10113: ARROW-11950: [C++][Compute] Add unary negative kernel
Posted by GitBox <gi...@apache.org>.
edponce commented on pull request #10113:
URL: https://github.com/apache/arrow/pull/10113#issuecomment-823383209
Windows build is failing due to [C4146 warning](https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-2-c4146?view=msvc-160) which is triggered when an unsigned integer is negated. This negation is done purposefully with the expected behavior of modulo arithmetic. An alternative is to negate unsigned integers using two's complement explicitly (see https://github.com/apache/arrow/blob/master/cpp/src/arrow/util/basic_decimal.cc#L375):
```shell
unsigned int x = 10;
unsigned int x_neg = ~x + 1;
```
@bkietz @pitrou please advise
--
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:
users@infra.apache.org
[GitHub] [arrow] cyb70289 commented on pull request #10113: ARROW-11950: [C++][Compute] Add unary negative kernel
Posted by GitBox <gi...@apache.org>.
cyb70289 commented on pull request #10113:
URL: https://github.com/apache/arrow/pull/10113#issuecomment-823738343
Same PR as https://github.com/apache/arrow/pull/10016??
--
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:
users@infra.apache.org
[GitHub] [arrow] edponce commented on pull request #10113: ARROW-11950: [C++][Compute] Add unary negative kernel
Posted by GitBox <gi...@apache.org>.
edponce commented on pull request #10113:
URL: https://github.com/apache/arrow/pull/10113#issuecomment-823548040
After changing the negation operation for unsigned integers, Windows build passes successfully (i.e., no warning).
--
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:
users@infra.apache.org
[GitHub] [arrow] bkietz closed pull request #10113: ARROW-11950: [C++][Compute] Add unary negative kernel
Posted by GitBox <gi...@apache.org>.
bkietz closed pull request #10113:
URL: https://github.com/apache/arrow/pull/10113
--
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:
users@infra.apache.org
[GitHub] [arrow] edponce commented on a change in pull request #10113: ARROW-11950: [C++][Compute] Add unary negative kernel
Posted by GitBox <gi...@apache.org>.
edponce commented on a change in pull request #10113:
URL: https://github.com/apache/arrow/pull/10113#discussion_r621551592
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -817,5 +932,126 @@ TEST(TestBinaryArithmetic, AddWithImplicitCastsUint64EdgeCase) {
ArrayFromJSON(uint64(), "[18446744073709551615]")}));
}
+TEST(TestUnaryArithmetic, DispatchBest) {
+ for (std::string name : {"negate"}) {
+ for (const auto& ty : {int8(), int16(), int32(), int64(), uint8(), uint16(), uint32(),
+ uint64(), float32(), float64()}) {
+ CheckDispatchBest(name, {ty}, {ty});
+ CheckDispatchBest(name, {dictionary(int8(), ty)}, {ty});
+ }
+ }
+
Review comment:
I added `CheckDispatchFails()` which checks if status is an error after invoking either `DispatchBest()` or `DispatchExact`. It seems the only error produced by `Dispatch*()` is `NotImplemented`. Nevertheless, I made the function description general enough and check for not OK status instead of only `NotImplemented` error, https://github.com/edponce/arrow/blob/ARROW-11950-Compute-Add-unary-negative-kernel/cpp/src/arrow/compute/kernels/test_util.h#L151-L152
--
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:
users@infra.apache.org
[GitHub] [arrow] cyb70289 commented on a change in pull request #10113: ARROW-11950: [C++][Compute] Add unary negative kernel
Posted by GitBox <gi...@apache.org>.
cyb70289 commented on a change in pull request #10113:
URL: https://github.com/apache/arrow/pull/10113#discussion_r621045269
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -235,6 +239,49 @@ struct DivideChecked {
}
};
+struct Negate {
+ template <typename T, typename Arg>
+ static constexpr enable_if_floating_point<T> Call(KernelContext*, Arg arg) {
+ return -arg;
+ }
+
+ template <typename T, typename Arg>
+ static constexpr enable_if_unsigned_integer<T> Call(KernelContext*, Arg arg) {
+ return ~arg + 1;
+ }
+
+ template <typename T, typename Arg>
+ static constexpr enable_if_signed_integer<T> Call(KernelContext*, Arg arg) {
+ return arrow::internal::SafeSignedNegate(arg);
+ }
+};
+
+struct NegateChecked {
+ template <typename T, typename Arg>
+ static enable_if_signed_integer<T> Call(KernelContext* ctx, Arg arg) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ T result = 0;
+ if (ARROW_PREDICT_FALSE(NegateWithOverflow(arg, &result))) {
+ ctx->SetStatus(Status::Invalid("overflow"));
Review comment:
PR #10098 changed a bit the behaviour of kernel error handling. Errors are not populated through KernelContext any more.
You can reference latest code in this source file. E.g., https://github.com/apache/arrow/blob/master/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc#L91
--
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:
users@infra.apache.org
[GitHub] [arrow] edponce commented on pull request #10113: ARROW-11950: [C++][Compute] Add unary negative kernel
Posted by GitBox <gi...@apache.org>.
edponce commented on pull request #10113:
URL: https://github.com/apache/arrow/pull/10113#issuecomment-823747793
Resolving same issue as in PR #10016 but #10113 is the one to officially review. I initially had submitted draft #10016 but mistakenly submitted a new PR (i.e., #10113).
--
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:
users@infra.apache.org