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 2020/06/03 11:33:34 UTC
[GitHub] [arrow] kszucs opened a new pull request #7341: ARROW-9022: [C++][Compute] Make Add function safe for numeric limits
kszucs opened a new pull request #7341:
URL: https://github.com/apache/arrow/pull/7341
Currently the output type of the Add function is identical with the argument types which makes it unsafe to add numeric limit values, so instead of using `(int8, int8) -> int8` signature we should use `(int8, int8) -> int16`.
----------------------------------------------------------------
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] wesm commented on a change in pull request #7341: ARROW-9022: [C++][Compute] Make Add function safe for numeric limits
Posted by GitBox <gi...@apache.org>.
wesm commented on a change in pull request #7341:
URL: https://github.com/apache/arrow/pull/7341#discussion_r435305624
##########
File path: cpp/src/arrow/compute/api_scalar.h
##########
@@ -48,6 +48,26 @@ namespace compute {
ARROW_EXPORT
Result<Datum> Add(const Datum& left, const Datum& right, ExecContext* ctx = NULLPTR);
+/// \brief Subtract two values. Array values must be the same length. If a
Review comment:
Can you write out the names of these functions, `Subtract` and `Multiply`?
##########
File path: cpp/src/arrow/compute/kernels/codegen_internal.h
##########
@@ -522,53 +503,56 @@ struct ScalarUnaryNotNull {
// // implementation
// }
// };
-template <typename OutType, typename Arg0Type, typename Arg1Type, typename Op,
- typename FlippedOp = Op>
+template <typename OutType, typename Arg0Type, typename Arg1Type, typename Op>
struct ScalarBinary {
using OUT = typename GetOutputType<OutType>::T;
using ARG0 = typename GetViewType<Arg0Type>::T;
using ARG1 = typename GetViewType<Arg1Type>::T;
- template <typename ChosenOp>
static void ArrayArray(KernelContext* ctx, const ExecBatch& batch, Datum* out) {
ArrayIterator<Arg0Type> arg0(*batch[0].array());
ArrayIterator<Arg1Type> arg1(*batch[1].array());
OutputAdapter<OutType>::Write(ctx, out, [&]() -> OUT {
- return ChosenOp::template Call(ctx, arg0(), arg1());
+ return Op::template Call(ctx, arg0(), arg1());
});
}
- template <typename ChosenOp>
static void ArrayScalar(KernelContext* ctx, const ExecBatch& batch, Datum* out) {
ArrayIterator<Arg0Type> arg0(*batch[0].array());
auto arg1 = UnboxScalar<Arg1Type>::Unbox(batch[1]);
OutputAdapter<OutType>::Write(ctx, out, [&]() -> OUT {
- return ChosenOp::template Call(ctx, arg0(), arg1);
+ return Op::template Call(ctx, arg0(), arg1);
+ });
+ }
+
+ static void ScalarArray(KernelContext* ctx, const ExecBatch& batch, Datum* out) {
Review comment:
I'm not thrilled about this because you're generating extra compiled code that wasn't there before
----------------------------------------------------------------
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] kszucs edited a comment on pull request #7341: ARROW-9022: [C++][Compute] Make Add function safe for numeric limits
Posted by GitBox <gi...@apache.org>.
kszucs edited a comment on pull request #7341:
URL: https://github.com/apache/arrow/pull/7341#issuecomment-638254725
> This is probably nuanced enough to merit a mailing list discussion.
Certainly.
What kind of overflow strategies would could we have for other functions with higher probability of overflowing like product?
----------------------------------------------------------------
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] kszucs edited a comment on pull request #7341: ARROW-9042: [C++] Add Subtract and Multiply arithmetic kernels with wrap-around behavior
Posted by GitBox <gi...@apache.org>.
kszucs edited a comment on pull request #7341:
URL: https://github.com/apache/arrow/pull/7341#issuecomment-639542072
@bkietz I think it is ready for a review now, the build failures are present on the master branch.
----------------------------------------------------------------
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] kszucs commented on pull request #7341: ARROW-9042: [C++] Add Subtract and Multiply arithmetic kernels with wrap-around behavior
Posted by GitBox <gi...@apache.org>.
kszucs commented on pull request #7341:
URL: https://github.com/apache/arrow/pull/7341#issuecomment-639542072
@bkietz I think it is ready for a review now.
----------------------------------------------------------------
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] kszucs commented on pull request #7341: ARROW-9022: [C++][Compute] Make Add function safe for numeric limits
Posted by GitBox <gi...@apache.org>.
kszucs commented on pull request #7341:
URL: https://github.com/apache/arrow/pull/7341#issuecomment-638254725
What kind of overflow strategies would could we have for other functions with higher probability of overflowing like product?
----------------------------------------------------------------
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] kszucs commented on a change in pull request #7341: ARROW-9022: [C++][Compute] Make Add function safe for numeric limits
Posted by GitBox <gi...@apache.org>.
kszucs commented on a change in pull request #7341:
URL: https://github.com/apache/arrow/pull/7341#discussion_r434500867
##########
File path: cpp/src/arrow/compute/kernels/codegen_internal.h
##########
@@ -522,53 +503,56 @@ struct ScalarUnaryNotNull {
// // implementation
// }
// };
-template <typename OutType, typename Arg0Type, typename Arg1Type, typename Op,
- typename FlippedOp = Op>
+template <typename OutType, typename Arg0Type, typename Arg1Type, typename Op>
struct ScalarBinary {
using OUT = typename GetOutputType<OutType>::T;
using ARG0 = typename GetViewType<Arg0Type>::T;
using ARG1 = typename GetViewType<Arg1Type>::T;
- template <typename ChosenOp>
static void ArrayArray(KernelContext* ctx, const ExecBatch& batch, Datum* out) {
ArrayIterator<Arg0Type> arg0(*batch[0].array());
ArrayIterator<Arg1Type> arg1(*batch[1].array());
OutputAdapter<OutType>::Write(ctx, out, [&]() -> OUT {
- return ChosenOp::template Call(ctx, arg0(), arg1());
+ return Op::template Call(ctx, arg0(), arg1());
});
}
- template <typename ChosenOp>
static void ArrayScalar(KernelContext* ctx, const ExecBatch& batch, Datum* out) {
ArrayIterator<Arg0Type> arg0(*batch[0].array());
auto arg1 = UnboxScalar<Arg1Type>::Unbox(batch[1]);
OutputAdapter<OutType>::Write(ctx, out, [&]() -> OUT {
- return ChosenOp::template Call(ctx, arg0(), arg1);
+ return Op::template Call(ctx, arg0(), arg1);
+ });
+ }
+
+ static void ScalarArray(KernelContext* ctx, const ExecBatch& batch, Datum* out) {
Review comment:
With a little code duplication I removed the ChosenOp template parameters making the implementation a bit more straightforward and reusable for other functions like Subtract where the order matters and defining a flipped operator would involve calling to another Negation function.
----------------------------------------------------------------
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 #7341: ARROW-9042: [C++] Add Subtract and Multiply arithmetic kernels with wrap-around behavior
Posted by GitBox <gi...@apache.org>.
bkietz closed pull request #7341:
URL: https://github.com/apache/arrow/pull/7341
----------------------------------------------------------------
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] wesm edited a comment on pull request #7341: ARROW-9022: [C++][Compute] Make Add function safe for numeric limits
Posted by GitBox <gi...@apache.org>.
wesm edited a comment on pull request #7341:
URL: https://github.com/apache/arrow/pull/7341#issuecomment-638284817
Per the mailing list discussion, I think we should apply the strategies used by open source analytic DBMS that we have access to and not think too hard about it:
* Do signed arithmetic as unsigned to prevent UB
* Do not promote small integers (except perhaps in multiplication, what do the DBMSes do?)
* Do not check for overflows
----------------------------------------------------------------
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] pitrou commented on pull request #7341: ARROW-9022: [C++][Compute] Make Add function safe for numeric limits
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #7341:
URL: https://github.com/apache/arrow/pull/7341#issuecomment-638415622
As I said on the ML, I'm -1 on implicit promotion. We may discuss whether overflow should be detected or not. But trying to make the output type "big enough" is a can of worms.
----------------------------------------------------------------
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] kszucs commented on pull request #7341: ARROW-9022: [C++][Compute] Make Add function safe for numeric limits
Posted by GitBox <gi...@apache.org>.
kszucs commented on pull request #7341:
URL: https://github.com/apache/arrow/pull/7341#issuecomment-638303877
I'll update the PR as discussed, and defer the implicit promotions to a follow-up PR.
----------------------------------------------------------------
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] wesm edited a comment on pull request #7341: ARROW-9022: [C++][Compute] Make Add function safe for numeric limits
Posted by GitBox <gi...@apache.org>.
wesm edited a comment on pull request #7341:
URL: https://github.com/apache/arrow/pull/7341#issuecomment-639151369
We could also simply not generate multiply kernels for 8- and 16-byte integer types.
----------------------------------------------------------------
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] wesm commented on a change in pull request #7341: ARROW-9022: [C++][Compute] Make Add function safe for numeric limits
Posted by GitBox <gi...@apache.org>.
wesm commented on a change in pull request #7341:
URL: https://github.com/apache/arrow/pull/7341#discussion_r435307017
##########
File path: cpp/src/arrow/compute/kernels/codegen_internal.h
##########
@@ -522,53 +503,56 @@ struct ScalarUnaryNotNull {
// // implementation
// }
// };
-template <typename OutType, typename Arg0Type, typename Arg1Type, typename Op,
- typename FlippedOp = Op>
+template <typename OutType, typename Arg0Type, typename Arg1Type, typename Op>
struct ScalarBinary {
using OUT = typename GetOutputType<OutType>::T;
using ARG0 = typename GetViewType<Arg0Type>::T;
using ARG1 = typename GetViewType<Arg1Type>::T;
- template <typename ChosenOp>
static void ArrayArray(KernelContext* ctx, const ExecBatch& batch, Datum* out) {
ArrayIterator<Arg0Type> arg0(*batch[0].array());
ArrayIterator<Arg1Type> arg1(*batch[1].array());
OutputAdapter<OutType>::Write(ctx, out, [&]() -> OUT {
- return ChosenOp::template Call(ctx, arg0(), arg1());
+ return Op::template Call(ctx, arg0(), arg1());
});
}
- template <typename ChosenOp>
static void ArrayScalar(KernelContext* ctx, const ExecBatch& batch, Datum* out) {
ArrayIterator<Arg0Type> arg0(*batch[0].array());
auto arg1 = UnboxScalar<Arg1Type>::Unbox(batch[1]);
OutputAdapter<OutType>::Write(ctx, out, [&]() -> OUT {
- return ChosenOp::template Call(ctx, arg0(), arg1);
+ return Op::template Call(ctx, arg0(), arg1);
+ });
+ }
+
+ static void ScalarArray(KernelContext* ctx, const ExecBatch& batch, Datum* out) {
Review comment:
--I'm not thrilled about this because you're generating extra compiled code that wasn't there before--
Sorry I think I was mistaken about this. This is fine
----------------------------------------------------------------
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] wesm commented on pull request #7341: ARROW-9022: [C++][Compute] Make Add function safe for numeric limits
Posted by GitBox <gi...@apache.org>.
wesm commented on pull request #7341:
URL: https://github.com/apache/arrow/pull/7341#issuecomment-638418561
Agreed. Implicit casts or type promotions is something that is typically negotiated by the orchestration/planning layer 1 to 2 levels above the 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kszucs commented on a change in pull request #7341: ARROW-9022: [C++][Compute] Make Add function safe for numeric limits
Posted by GitBox <gi...@apache.org>.
kszucs commented on a change in pull request #7341:
URL: https://github.com/apache/arrow/pull/7341#discussion_r434501950
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -21,21 +21,44 @@ namespace arrow {
namespace compute {
struct Add {
- template <typename OUT, typename ARG0, typename ARG1>
- static constexpr OUT Call(KernelContext*, ARG0 left, ARG1 right) {
+ template <typename ARG0, typename ARG1>
+ static constexpr auto Call(KernelContext*, ARG0 left, ARG1 right)
+ -> decltype(left + right) {
Review comment:
Thanks @bkietz for this fix!
----------------------------------------------------------------
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] kszucs commented on a change in pull request #7341: ARROW-9022: [C++][Compute] Make Add function safe for numeric limits
Posted by GitBox <gi...@apache.org>.
kszucs commented on a change in pull request #7341:
URL: https://github.com/apache/arrow/pull/7341#discussion_r435630182
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -37,68 +37,275 @@
namespace arrow {
namespace compute {
-template <typename ArrowType>
-class TestArithmeticKernel : public TestBase {
- private:
- void AssertAddArrays(const std::shared_ptr<Array> lhs, const std::shared_ptr<Array> rhs,
- const std::shared_ptr<Array> expected) {
- ASSERT_OK_AND_ASSIGN(Datum out, arrow::compute::Add(lhs, rhs));
- std::shared_ptr<Array> actual = out.make_array();
- ASSERT_OK(actual->ValidateFull());
- AssertArraysEqual(*expected, *actual);
- }
+template <typename TypePair>
+class TestBinaryArithmetics;
+template <typename I, typename O>
+class TestBinaryArithmetics<std::pair<I, O>> : public TestBase {
protected:
- virtual void AssertAdd(const std::string& lhs, const std::string& rhs,
- const std::string& expected) {
- auto type = TypeTraits<ArrowType>::type_singleton();
- AssertAddArrays(ArrayFromJSON(type, lhs), ArrayFromJSON(type, rhs),
- ArrayFromJSON(type, expected));
+ using InputType = I;
+ using OutputType = O;
+ using InputCType = typename I::c_type;
+ using OutputCType = typename O::c_type;
+
+ using BinaryFunction =
+ std::function<Result<Datum>(const Datum&, const Datum&, ExecContext*)>;
+
+ std::shared_ptr<Array> MakeInputArray(const std::vector<InputCType>& values) {
+ std::shared_ptr<Array> out;
+ ArrayFromVector<InputType>(values, &out);
+ return out;
+ }
+
+ std::shared_ptr<Array> MakeOutputArray(const std::vector<OutputCType>& values) {
+ std::shared_ptr<Array> out;
+ ArrayFromVector<OutputType>(values, &out);
+ return out;
+ }
+
+ // (Scalar, Array)
+ void AssertBinop(BinaryFunction func, InputCType lhs, const std::string& rhs,
+ const std::string& expected) {
+ auto input_type = TypeTraits<InputType>::type_singleton();
+ auto output_type = TypeTraits<OutputType>::type_singleton();
+
+ ASSERT_OK_AND_ASSIGN(auto left, MakeScalar(input_type, lhs));
+ auto right = ArrayFromJSON(input_type, rhs);
+ auto exp = ArrayFromJSON(output_type, expected);
+
+ ASSERT_OK_AND_ASSIGN(auto result, func(left, right, nullptr));
+ std::shared_ptr<Array> out = result.make_array();
+ ASSERT_OK(out->ValidateFull());
+ AssertArraysEqual(*exp, *out);
+ }
+
+ // (Array, Array)
+ void AssertBinop(BinaryFunction func, const std::shared_ptr<Array>& lhs,
+ const std::shared_ptr<Array>& rhs,
+ const std::shared_ptr<Array>& expected) {
+ ASSERT_OK_AND_ASSIGN(Datum result, func(lhs, rhs, nullptr));
+ std::shared_ptr<Array> out = result.make_array();
+ ASSERT_OK(out->ValidateFull());
+ AssertArraysEqual(*expected, *out);
+ }
+
+ void AssertBinop(BinaryFunction func, const std::string& lhs, const std::string& rhs,
+ const std::string& expected) {
+ auto input_type = TypeTraits<InputType>::type_singleton();
+ auto output_type = TypeTraits<OutputType>::type_singleton();
+ AssertBinop(func, ArrayFromJSON(input_type, lhs), ArrayFromJSON(input_type, rhs),
+ ArrayFromJSON(output_type, expected));
}
};
-template <typename ArrowType>
-class TestArithmeticKernelFloating : public TestArithmeticKernel<ArrowType> {};
-TYPED_TEST_SUITE(TestArithmeticKernelFloating, RealArrowTypes);
+template <typename TypePair>
+class TestBinaryArithmeticsIntegral : public TestBinaryArithmetics<TypePair> {};
+
+template <typename TypePair>
+class TestBinaryArithmeticsSigned : public TestBinaryArithmeticsIntegral<TypePair> {};
+
+template <typename TypePair>
+class TestBinaryArithmeticsUnsigned : public TestBinaryArithmeticsIntegral<TypePair> {};
-template <typename ArrowType>
-class TestArithmeticKernelIntegral : public TestArithmeticKernel<ArrowType> {};
-TYPED_TEST_SUITE(TestArithmeticKernelIntegral, IntegralArrowTypes);
+template <typename TypePair>
+class TestBinaryArithmeticsFloating : public TestBinaryArithmetics<TypePair> {};
-TYPED_TEST(TestArithmeticKernelFloating, Add) {
- this->AssertAdd("[]", "[]", "[]");
+// InputType - OutputType pairs
+using IntegralPairs =
+ testing::Types<std::pair<Int8Type, Int8Type>, std::pair<Int16Type, Int16Type>,
+ std::pair<Int32Type, Int32Type>, std::pair<Int64Type, Int64Type>,
+ std::pair<UInt8Type, UInt8Type>, std::pair<UInt16Type, UInt16Type>,
+ std::pair<UInt32Type, UInt32Type>, std::pair<UInt64Type, UInt64Type>>;
- this->AssertAdd("[3.4, 2.6, 6.3]", "[1, 0, 2]", "[4.4, 2.6, 8.3]");
+using SignedIntegerPairs =
+ testing::Types<std::pair<Int8Type, Int8Type>, std::pair<Int16Type, Int16Type>,
+ std::pair<Int32Type, Int32Type>, std::pair<Int64Type, Int64Type>>;
- this->AssertAdd("[1.1, 2.4, 3.5, 4.3, 5.1, 6.8, 7.3]", "[0, 1, 2, 3, 4, 5, 6]",
- "[1.1, 3.4, 5.5, 7.3, 9.1, 11.8, 13.3]");
+using UnsignedIntegerPairs =
+ testing::Types<std::pair<UInt8Type, UInt8Type>, std::pair<UInt16Type, UInt16Type>,
+ std::pair<UInt32Type, UInt32Type>, std::pair<UInt64Type, UInt64Type>>;
- this->AssertAdd("[7, 6, 5, 4, 3, 2, 1]", "[6, 5, 4, 3, 2, 1, 0]",
- "[13, 11, 9, 7, 5, 3, 1]");
+// TODO(kszucs): add half-float
+using FloatingPairs =
+ testing::Types<std::pair<FloatType, FloatType>, std::pair<DoubleType, DoubleType>>;
- this->AssertAdd("[10.4, 12, 4.2, 50, 50.3, 32, 11]", "[2, 0, 6, 1, 5, 3, 4]",
- "[12.4, 12, 10.2, 51, 55.3, 35, 15]");
+TYPED_TEST_SUITE(TestBinaryArithmeticsIntegral, IntegralPairs);
+TYPED_TEST_SUITE(TestBinaryArithmeticsSigned, SignedIntegerPairs);
+TYPED_TEST_SUITE(TestBinaryArithmeticsUnsigned, UnsignedIntegerPairs);
+TYPED_TEST_SUITE(TestBinaryArithmeticsFloating, FloatingPairs);
- this->AssertAdd("[null, 1, 3.3, null, 2, 5.3]", "[1, 4, 2, 5, 0, 3]",
- "[null, 5, 5.3, null, 2, 8.3]");
+TYPED_TEST(TestBinaryArithmeticsIntegral, Add) {
+ this->AssertBinop(arrow::compute::Add, "[]", "[]", "[]");
+ this->AssertBinop(arrow::compute::Add, "[null]", "[null]", "[null]");
+ this->AssertBinop(arrow::compute::Add, "[3, 2, 6]", "[1, 0, 2]", "[4, 2, 8]");
+
+ this->AssertBinop(arrow::compute::Add, "[1, 2, 3, 4, 5, 6, 7]", "[0, 1, 2, 3, 4, 5, 6]",
+ "[1, 3, 5, 7, 9, 11, 13]");
+
+ this->AssertBinop(arrow::compute::Add, "[10, 12, 4, 50, 50, 32, 11]",
+ "[2, 0, 6, 1, 5, 3, 4]", "[12, 12, 10, 51, 55, 35, 15]");
+
+ this->AssertBinop(arrow::compute::Add, "[null, 1, 3, null, 2, 5]", "[1, 4, 2, 5, 0, 3]",
+ "[null, 5, 5, null, 2, 8]");
+
+ this->AssertBinop(arrow::compute::Add, 10, "[null, 1, 3, null, 2, 5]",
+ "[null, 11, 13, null, 12, 15]");
+}
+
+TYPED_TEST(TestBinaryArithmeticsIntegral, Sub) {
+ this->AssertBinop(arrow::compute::Subtract, "[]", "[]", "[]");
+ this->AssertBinop(arrow::compute::Subtract, "[null]", "[null]", "[null]");
+ this->AssertBinop(arrow::compute::Subtract, "[3, 2, 6]", "[1, 0, 2]", "[2, 2, 4]");
+
+ this->AssertBinop(arrow::compute::Subtract, "[1, 2, 3, 4, 5, 6, 7]",
+ "[0, 1, 2, 3, 4, 5, 6]", "[1, 1, 1, 1, 1, 1, 1]");
+
+ this->AssertBinop(arrow::compute::Subtract, 10, "[null, 1, 3, null, 2, 5]",
+ "[null, 9, 7, null, 8, 5]");
}
-TYPED_TEST(TestArithmeticKernelIntegral, Add) {
- this->AssertAdd("[]", "[]", "[]");
+TYPED_TEST(TestBinaryArithmeticsIntegral, Mul) {
+ this->AssertBinop(arrow::compute::Multiply, "[]", "[]", "[]");
+ this->AssertBinop(arrow::compute::Multiply, "[null]", "[null]", "[null]");
+ this->AssertBinop(arrow::compute::Multiply, "[3, 2, 6]", "[1, 0, 2]", "[3, 0, 12]");
+
+ this->AssertBinop(arrow::compute::Multiply, "[1, 2, 3, 4, 5, 6, 7]",
+ "[0, 1, 2, 3, 4, 5, 6]", "[0, 2, 6, 12, 20, 30, 42]");
+
+ this->AssertBinop(arrow::compute::Multiply, "[7, 6, 5, 4, 3, 2, 1]",
+ "[6, 5, 4, 3, 2, 1, 0]", "[42, 30, 20, 12, 6, 2, 0]");
+
+ this->AssertBinop(arrow::compute::Multiply, "[null, 1, 3, null, 2, 5]",
+ "[1, 4, 2, 5, 0, 3]", "[null, 4, 6, null, 0, 15]");
+
+ this->AssertBinop(arrow::compute::Multiply, 3, "[null, 1, 3, null, 2, 5]",
+ "[null, 3, 9, null, 6, 15]");
+}
+
+TYPED_TEST(TestBinaryArithmeticsSigned, Add) {
+ this->AssertBinop(arrow::compute::Add, "[-7, 6, 5, 4, 3, 2, 1]",
+ "[-6, 5, -4, 3, -2, 1, 0]", "[-13, 11, 1, 7, 1, 3, 1]");
+}
+
+TYPED_TEST(TestBinaryArithmeticsUnsigned, OverflowWraps) {
+ using InputCType = typename TestFixture::InputCType;
+
+ auto min = std::numeric_limits<InputCType>::min();
+ auto max = std::numeric_limits<InputCType>::max();
+ {
+ // Addition
+ auto left = this->MakeInputArray({max, min, max});
+ auto right = this->MakeInputArray({1, 1, max});
+ auto expected = this->MakeOutputArray({0, 1, static_cast<InputCType>(max - 1)});
+ this->AssertBinop(arrow::compute::Add, left, right, expected);
+ }
+ {
+ // Subtraction
+ auto left = this->MakeInputArray({min, max});
+ auto right = this->MakeInputArray({1, max});
+ auto expected = this->MakeOutputArray({max, min});
+ this->AssertBinop(arrow::compute::Subtract, left, right, expected);
+ }
+ {
+ // Multiplication
+ auto left = this->MakeInputArray({min, max, max});
+ auto right = this->MakeInputArray({max, 2, max});
+ auto expected = this->MakeOutputArray({min, static_cast<InputCType>(max - 1), 1});
+ this->AssertBinop(arrow::compute::Multiply, left, right, expected);
+ }
+}
+
+TYPED_TEST(TestBinaryArithmeticsSigned, OverflowWraps) {
+ using InputCType = typename TestFixture::InputCType;
+
+ auto min = std::numeric_limits<InputCType>::min();
+ auto max = std::numeric_limits<InputCType>::max();
+ {
+ // Addition
+ auto left = this->MakeInputArray({max, min});
+ auto right = this->MakeInputArray({1, -1});
+ auto expected = this->MakeOutputArray({min, max});
+ this->AssertBinop(arrow::compute::Add, left, right, expected);
+ }
+ {
+ // Subtraction
+ auto left = this->MakeInputArray({min, max});
+ auto right = this->MakeInputArray({1, -1});
+ auto expected = this->MakeOutputArray({max, min});
+ this->AssertBinop(arrow::compute::Subtract, left, right, expected);
+ }
+ {
+ // Multiplication
+ auto left = this->MakeInputArray({min, max});
+ auto right = this->MakeInputArray({-1, 2});
+ auto expected = this->MakeOutputArray({min, -2});
+ this->AssertBinop(arrow::compute::Multiply, left, right, expected);
+ }
+}
+
+TYPED_TEST(TestBinaryArithmeticsSigned, Sub) {
+ this->AssertBinop(arrow::compute::Subtract, "[0, 1, 2, 3, 4, 5, 6]",
+ "[1, 2, 3, 4, 5, 6, 7]", "[-1, -1, -1, -1, -1, -1, -1]");
+
+ this->AssertBinop(arrow::compute::Subtract, "[0, 0, 0, 0, 0, 0, 0]",
+ "[6, 5, 4, 3, 2, 1, 0]", "[-6, -5, -4, -3, -2, -1, 0]");
+
+ this->AssertBinop(arrow::compute::Subtract, "[10, 12, 4, 50, 50, 32, 11]",
+ "[2, 0, 6, 1, 5, 3, 4]", "[8, 12, -2, 49, 45, 29, 7]");
+
+ this->AssertBinop(arrow::compute::Subtract, "[null, 1, 3, null, 2, 5]",
+ "[1, 4, 2, 5, 0, 3]", "[null, -3, 1, null, 2, 2]");
+}
+
+TYPED_TEST(TestBinaryArithmeticsSigned, Mul) {
+ this->AssertBinop(arrow::compute::Multiply, "[-10, 12, 4, 50, -5, 32, 11]",
+ "[-2, 0, -6, 1, 5, 3, 4]", "[20, 0, -24, 50, -25, 96, 44]");
+}
+
+TYPED_TEST(TestBinaryArithmeticsFloating, Add) {
+ this->AssertBinop(arrow::compute::Add, "[]", "[]", "[]");
+
+ this->AssertBinop(arrow::compute::Add, "[3.4, 2.6, 6.3]", "[1, 0, 2]",
+ "[4.4, 2.6, 8.3]");
+
+ this->AssertBinop(arrow::compute::Add, "[1.1, 2.4, 3.5, 4.3, 5.1, 6.8, 7.3]",
+ "[0, 1, 2, 3, 4, 5, 6]", "[1.1, 3.4, 5.5, 7.3, 9.1, 11.8, 13.3]");
+
+ this->AssertBinop(arrow::compute::Add, "[7, 6, 5, 4, 3, 2, 1]", "[6, 5, 4, 3, 2, 1, 0]",
+ "[13, 11, 9, 7, 5, 3, 1]");
+
+ this->AssertBinop(arrow::compute::Add, "[10.4, 12, 4.2, 50, 50.3, 32, 11]",
+ "[2, 0, 6, 1, 5, 3, 4]", "[12.4, 12, 10.2, 51, 55.3, 35, 15]");
+
+ this->AssertBinop(arrow::compute::Add, "[null, 1, 3.3, null, 2, 5.3]",
+ "[1, 4, 2, 5, 0, 3]", "[null, 5, 5.3, null, 2, 8.3]");
+
+ this->AssertBinop(arrow::compute::Add, 1.1, "[null, 1, 3.3, null, 2, 5.3]",
+ "[null, 2.1, 4.4, null, 3.1, 6.4]");
+}
+
+TYPED_TEST(TestBinaryArithmeticsFloating, Sub) {
+ this->AssertBinop(arrow::compute::Subtract, "[]", "[]", "[]");
+
+ this->AssertBinop(arrow::compute::Subtract, "[3.4, 2.6, 6.3]", "[1, 0, 2]",
+ "[2.4, 2.6, 4.3]");
- this->AssertAdd("[3, 2, 6]", "[1, 0, 2]", "[4, 2, 8]");
+ // this->AssertBinop(arrow::compute::Subtract,
Review comment:
The array equality check fails despite that after printing out both sides the values are the same. I'm not sure why, perhaps a float precision problem?
----------------------------------------------------------------
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] kszucs commented on pull request #7341: ARROW-9022: [C++][Compute] Make Add function safe for numeric limits
Posted by GitBox <gi...@apache.org>.
kszucs commented on pull request #7341:
URL: https://github.com/apache/arrow/pull/7341#issuecomment-639190763
Just fixed 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #7341: ARROW-9022: [C++][Compute] Make Add function safe for numeric limits
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #7341:
URL: https://github.com/apache/arrow/pull/7341#discussion_r435292993
##########
File path: cpp/src/arrow/compute/api_scalar.h
##########
@@ -48,6 +48,26 @@ namespace compute {
ARROW_EXPORT
Result<Datum> Add(const Datum& left, const Datum& right, ExecContext* ctx = NULLPTR);
+/// \brief Subtract two values. Array values must be the same length. If a
+/// value is null in either addend, the result is null
+///
+/// \param[in] left the first value
+/// \param[in] right the second value
+/// \param[in] ctx the function execution context, optional
+/// \return the elementwise addition of the values
+ARROW_EXPORT
+Result<Datum> Sub(const Datum& left, const Datum& right, ExecContext* ctx = NULLPTR);
Review comment:
```suggestion
/// \brief Subtract two values. Array values must be the same length. If the
/// minuend or minuend is null the result will be null.
///
/// \param[in] minuend the value subtracted from
/// \param[in] subtrahend the value by which the minuend is reduced
/// \param[in] ctx the function execution context, optional
/// \return the elementwise difference of the values
ARROW_EXPORT
Result<Datum> Sub(const Datum& left, const Datum& right, ExecContext* ctx = NULLPTR);
```
##########
File path: cpp/src/arrow/compute/api_scalar.h
##########
@@ -48,6 +48,26 @@ namespace compute {
ARROW_EXPORT
Result<Datum> Add(const Datum& left, const Datum& right, ExecContext* ctx = NULLPTR);
+/// \brief Subtract two values. Array values must be the same length. If a
+/// value is null in either addend, the result is null
+///
+/// \param[in] left the first value
+/// \param[in] right the second value
+/// \param[in] ctx the function execution context, optional
+/// \return the elementwise addition of the values
+ARROW_EXPORT
+Result<Datum> Sub(const Datum& left, const Datum& right, ExecContext* ctx = NULLPTR);
+
+/// \brief Multiply two values. Array values must be the same length. If a
+/// value is null in either addend, the result is null
+///
+/// \param[in] left the first value
+/// \param[in] right the second value
+/// \param[in] ctx the function execution context, optional
+/// \return the elementwise addition of the values
+ARROW_EXPORT
+Result<Datum> Mul(const Datum& left, const Datum& right, ExecContext* ctx = NULLPTR);
Review comment:
```suggestion
/// \brief Multiply two values. Array values must be the same length. If either
/// factor is null, the result is null
///
/// \param[in] left the first factor
/// \param[in] right the second factor
/// \param[in] ctx the function execution context, optional
/// \return the elementwise product of the factors
ARROW_EXPORT
Result<Datum> Mul(const Datum& left, const Datum& right, ExecContext* ctx = NULLPTR);
```
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -16,25 +16,41 @@
// under the License.
#include "arrow/compute/kernels/common.h"
+#include "arrow/util/int_util.h"
+#include "iostream"
Review comment:
```suggestion
```
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -16,25 +16,41 @@
// under the License.
#include "arrow/compute/kernels/common.h"
+#include "arrow/util/int_util.h"
+#include "iostream"
namespace arrow {
namespace compute {
struct Add {
- template <typename OUT, typename ARG0, typename ARG1>
- static constexpr OUT Call(KernelContext*, ARG0 left, ARG1 right) {
- return left + right;
+ template <typename T>
+ static constexpr T Call(KernelContext*, T left, T right) {
+ return left - right;
+ }
+};
+
+struct Sub {
+ template <typename T>
+ static constexpr T Call(KernelContext*, T left, T right) {
+ return left - right;
+ }
+};
+
+struct Mul {
+ template <typename T>
+ static constexpr T Call(KernelContext*, T left, T right) {
+ return left * right;
}
};
namespace codegen {
template <typename Op>
-void MakeBinaryFunction(std::string name, FunctionRegistry* registry) {
+void AddBinaryFunction(std::string name, FunctionRegistry* registry) {
auto func = std::make_shared<ScalarFunction>(name, Arity::Binary());
- for (const std::shared_ptr<DataType>& ty : NumericTypes()) {
- DCHECK_OK(func->AddKernel({InputType::Array(ty), InputType::Array(ty)}, ty,
- NumericEqualTypesBinary<Op>(*ty)));
+ for (const auto& ty : NumericTypes()) {
+ auto exec = codegen::NumericEqualTypesBinary<Op>(ty);
Review comment:
This invokes undefined behavior in the case of signed types. Please add a kernel wrapper which `View`s the signed types as the corresponding unsigned type.
----------------------------------------------------------------
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] wesm commented on pull request #7341: ARROW-9022: [C++][Compute] Make Add function safe for numeric limits
Posted by GitBox <gi...@apache.org>.
wesm commented on pull request #7341:
URL: https://github.com/apache/arrow/pull/7341#issuecomment-638284817
Per the mailing list discussion, I think we should apply the strategies used by open source analytic DBMS that we have access to and not think too hard about it:
* Do signed arithmetic as unsigned to prevent UB
* Do not promote small integers
* Do not check for overflows
----------------------------------------------------------------
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] kszucs commented on a change in pull request #7341: ARROW-9022: [C++][Compute] Make Add function safe for numeric limits
Posted by GitBox <gi...@apache.org>.
kszucs commented on a change in pull request #7341:
URL: https://github.com/apache/arrow/pull/7341#discussion_r434501653
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -21,21 +21,44 @@ namespace arrow {
namespace compute {
struct Add {
- template <typename OUT, typename ARG0, typename ARG1>
- static constexpr OUT Call(KernelContext*, ARG0 left, ARG1 right) {
+ template <typename ARG0, typename ARG1>
+ static constexpr auto Call(KernelContext*, ARG0 left, ARG1 right)
+ -> decltype(left + right) {
return left + right;
}
};
namespace codegen {
+template <typename Op, typename ArgType, typename OutType>
+void AddBinaryKernel(const std::shared_ptr<ScalarFunction>& func) {
+ // create an exec function with the requested signature
+ ArrayKernelExec exec = ScalarBinaryEqualTypes<OutType, ArgType, Op>::Exec;
Review comment:
Using `ScalarBinaryEqualTypes` make the kernel support both Scalar and Array shaped inputs.
----------------------------------------------------------------
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 #7341: ARROW-9042: [C++] Add Subtract and Multiply arithmetic kernels with wrap-around behavior
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7341:
URL: https://github.com/apache/arrow/pull/7341#issuecomment-639466642
https://issues.apache.org/jira/browse/ARROW-9042
----------------------------------------------------------------
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] kszucs commented on a change in pull request #7341: ARROW-9022: [C++][Compute] Make Add function safe for numeric limits
Posted by GitBox <gi...@apache.org>.
kszucs commented on a change in pull request #7341:
URL: https://github.com/apache/arrow/pull/7341#discussion_r435471744
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -16,25 +16,41 @@
// under the License.
#include "arrow/compute/kernels/common.h"
+#include "arrow/util/int_util.h"
+#include "iostream"
namespace arrow {
namespace compute {
struct Add {
- template <typename OUT, typename ARG0, typename ARG1>
- static constexpr OUT Call(KernelContext*, ARG0 left, ARG1 right) {
- return left + right;
+ template <typename T>
+ static constexpr T Call(KernelContext*, T left, T right) {
+ return left - right;
+ }
+};
+
+struct Sub {
+ template <typename T>
+ static constexpr T Call(KernelContext*, T left, T right) {
+ return left - right;
+ }
+};
+
+struct Mul {
+ template <typename T>
+ static constexpr T Call(KernelContext*, T left, T right) {
+ return left * right;
}
};
namespace codegen {
template <typename Op>
-void MakeBinaryFunction(std::string name, FunctionRegistry* registry) {
+void AddBinaryFunction(std::string name, FunctionRegistry* registry) {
auto func = std::make_shared<ScalarFunction>(name, Arity::Binary());
- for (const std::shared_ptr<DataType>& ty : NumericTypes()) {
- DCHECK_OK(func->AddKernel({InputType::Array(ty), InputType::Array(ty)}, ty,
- NumericEqualTypesBinary<Op>(*ty)));
+ for (const auto& ty : NumericTypes()) {
+ auto exec = codegen::NumericEqualTypesBinary<Op>(ty);
Review comment:
So changing the kernel's exec function's input types to unsigned counterparts and leaving the output eliminates the signed overflow (at least ubsan doesn't complain anymore).
----------------------------------------------------------------
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] wesm commented on a change in pull request #7341: ARROW-9022: [C++][Compute] Make Add function safe for numeric limits
Posted by GitBox <gi...@apache.org>.
wesm commented on a change in pull request #7341:
URL: https://github.com/apache/arrow/pull/7341#discussion_r435307017
##########
File path: cpp/src/arrow/compute/kernels/codegen_internal.h
##########
@@ -522,53 +503,56 @@ struct ScalarUnaryNotNull {
// // implementation
// }
// };
-template <typename OutType, typename Arg0Type, typename Arg1Type, typename Op,
- typename FlippedOp = Op>
+template <typename OutType, typename Arg0Type, typename Arg1Type, typename Op>
struct ScalarBinary {
using OUT = typename GetOutputType<OutType>::T;
using ARG0 = typename GetViewType<Arg0Type>::T;
using ARG1 = typename GetViewType<Arg1Type>::T;
- template <typename ChosenOp>
static void ArrayArray(KernelContext* ctx, const ExecBatch& batch, Datum* out) {
ArrayIterator<Arg0Type> arg0(*batch[0].array());
ArrayIterator<Arg1Type> arg1(*batch[1].array());
OutputAdapter<OutType>::Write(ctx, out, [&]() -> OUT {
- return ChosenOp::template Call(ctx, arg0(), arg1());
+ return Op::template Call(ctx, arg0(), arg1());
});
}
- template <typename ChosenOp>
static void ArrayScalar(KernelContext* ctx, const ExecBatch& batch, Datum* out) {
ArrayIterator<Arg0Type> arg0(*batch[0].array());
auto arg1 = UnboxScalar<Arg1Type>::Unbox(batch[1]);
OutputAdapter<OutType>::Write(ctx, out, [&]() -> OUT {
- return ChosenOp::template Call(ctx, arg0(), arg1);
+ return Op::template Call(ctx, arg0(), arg1);
+ });
+ }
+
+ static void ScalarArray(KernelContext* ctx, const ExecBatch& batch, Datum* out) {
Review comment:
~~I'm not thrilled about this because you're generating extra compiled code that wasn't there before~~
Sorry I think I was mistaken about this. This is fine
----------------------------------------------------------------
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] kszucs commented on pull request #7341: ARROW-9042: [C++] Add Subtract and Multiply arithmetic kernels with wrap-around behavior
Posted by GitBox <gi...@apache.org>.
kszucs commented on pull request #7341:
URL: https://github.com/apache/arrow/pull/7341#issuecomment-640911140
Nice! Thanks for upgrading 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kszucs commented on a change in pull request #7341: ARROW-9042: [C++] Add Subtract and Multiply arithmetic kernels with wrap-around behavior
Posted by GitBox <gi...@apache.org>.
kszucs commented on a change in pull request #7341:
URL: https://github.com/apache/arrow/pull/7341#discussion_r437361312
##########
File path: cpp/src/arrow/compute/kernels/codegen_internal.h
##########
@@ -337,25 +337,6 @@ void ScalarPrimitiveExecUnary(KernelContext* ctx, const ExecBatch& batch, Datum*
}
}
-template <typename Op, typename OutType, typename Arg0Type, typename Arg1Type>
-void ScalarPrimitiveExecBinary(KernelContext* ctx, const ExecBatch& batch, Datum* out) {
- using OUT = typename OutType::c_type;
- using ARG0 = typename Arg0Type::c_type;
- using ARG1 = typename Arg1Type::c_type;
-
- if (batch[0].kind() == Datum::SCALAR || batch[1].kind() == Datum::SCALAR) {
- ctx->SetStatus(Status::NotImplemented("NYI"));
- } else {
- ArrayData* out_arr = out->mutable_array();
- auto out_data = out_arr->GetMutableValues<OUT>(1);
- auto arg0_data = batch[0].array()->GetValues<ARG0>(1);
- auto arg1_data = batch[1].array()->GetValues<ARG1>(1);
- for (int64_t i = 0; i < batch.length; ++i) {
- *out_data++ = Op::template Call<OUT, ARG0, ARG1>(ctx, *arg0_data++, *arg1_data++);
- }
- }
-}
Review comment:
Writing a benchmark, the jira to track it https://issues.apache.org/jira/browse/ARROW-9079
----------------------------------------------------------------
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] kszucs commented on a change in pull request #7341: ARROW-9022: [C++][Compute] Make Add function safe for numeric limits
Posted by GitBox <gi...@apache.org>.
kszucs commented on a change in pull request #7341:
URL: https://github.com/apache/arrow/pull/7341#discussion_r435454679
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -16,25 +16,41 @@
// under the License.
#include "arrow/compute/kernels/common.h"
+#include "arrow/util/int_util.h"
+#include "iostream"
namespace arrow {
namespace compute {
struct Add {
- template <typename OUT, typename ARG0, typename ARG1>
- static constexpr OUT Call(KernelContext*, ARG0 left, ARG1 right) {
- return left + right;
+ template <typename T>
+ static constexpr T Call(KernelContext*, T left, T right) {
+ return left - right;
+ }
+};
+
+struct Sub {
+ template <typename T>
+ static constexpr T Call(KernelContext*, T left, T right) {
+ return left - right;
+ }
+};
+
+struct Mul {
+ template <typename T>
+ static constexpr T Call(KernelContext*, T left, T right) {
+ return left * right;
}
};
namespace codegen {
template <typename Op>
-void MakeBinaryFunction(std::string name, FunctionRegistry* registry) {
+void AddBinaryFunction(std::string name, FunctionRegistry* registry) {
auto func = std::make_shared<ScalarFunction>(name, Arity::Binary());
- for (const std::shared_ptr<DataType>& ty : NumericTypes()) {
- DCHECK_OK(func->AddKernel({InputType::Array(ty), InputType::Array(ty)}, ty,
- NumericEqualTypesBinary<Op>(*ty)));
+ for (const auto& ty : NumericTypes()) {
+ auto exec = codegen::NumericEqualTypesBinary<Op>(ty);
Review comment:
Working on it, probably will need your help.
----------------------------------------------------------------
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] wesm commented on a change in pull request #7341: ARROW-9042: [C++] Add Subtract and Multiply arithmetic kernels with wrap-around behavior
Posted by GitBox <gi...@apache.org>.
wesm commented on a change in pull request #7341:
URL: https://github.com/apache/arrow/pull/7341#discussion_r437083061
##########
File path: cpp/src/arrow/compute/kernels/codegen_internal.h
##########
@@ -337,25 +337,6 @@ void ScalarPrimitiveExecUnary(KernelContext* ctx, const ExecBatch& batch, Datum*
}
}
-template <typename Op, typename OutType, typename Arg0Type, typename Arg1Type>
-void ScalarPrimitiveExecBinary(KernelContext* ctx, const ExecBatch& batch, Datum* out) {
- using OUT = typename OutType::c_type;
- using ARG0 = typename Arg0Type::c_type;
- using ARG1 = typename Arg1Type::c_type;
-
- if (batch[0].kind() == Datum::SCALAR || batch[1].kind() == Datum::SCALAR) {
- ctx->SetStatus(Status::NotImplemented("NYI"));
- } else {
- ArrayData* out_arr = out->mutable_array();
- auto out_data = out_arr->GetMutableValues<OUT>(1);
- auto arg0_data = batch[0].array()->GetValues<ARG0>(1);
- auto arg1_data = batch[1].array()->GetValues<ARG1>(1);
- for (int64_t i = 0; i < batch.length; ++i) {
- *out_data++ = Op::template Call<OUT, ARG0, ARG1>(ctx, *arg0_data++, *arg1_data++);
- }
- }
-}
Review comment:
If you're going to remove this, you absolutely must write benchmarks to show that the more general version is not slower.
----------------------------------------------------------------
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] wesm commented on pull request #7341: ARROW-9022: [C++][Compute] Make Add function safe for numeric limits
Posted by GitBox <gi...@apache.org>.
wesm commented on pull request #7341:
URL: https://github.com/apache/arrow/pull/7341#issuecomment-639151369
We could also simply not generate kernels for 8- and 16-byte integer types.
----------------------------------------------------------------
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 #7341: ARROW-9022: [C++][Compute] Make Add function safe for numeric limits
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7341:
URL: https://github.com/apache/arrow/pull/7341#issuecomment-638151502
https://issues.apache.org/jira/browse/ARROW-9022
----------------------------------------------------------------
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] kszucs commented on pull request #7341: ARROW-9022: [C++][Compute] Make Add function safe for numeric limits
Posted by GitBox <gi...@apache.org>.
kszucs commented on pull request #7341:
URL: https://github.com/apache/arrow/pull/7341#issuecomment-639123573
The `(uint16 * uint16)` operation triggers the following ubsan error:
```
runtime error: signed integer overflow: 65535 * 65535 cannot be represented in type 'int'
```
According to this [SO post](https://stackoverflow.com/questions/24371868/why-must-a-short-be-converted-to-an-int-before-arithmetic-operations-in-c-and-c) we might need a special case for uint16 (or perhaps it depends on the arch) multiplication.
----------------------------------------------------------------
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 pull request #7341: ARROW-9022: [C++][Compute] Make Add function safe for numeric limits
Posted by GitBox <gi...@apache.org>.
bkietz commented on pull request #7341:
URL: https://github.com/apache/arrow/pull/7341#issuecomment-638241193
> we should use `(int8, int8) -> int16`
I'm not sure that's desirable. For one thing this leads to inconsistent handling of 64 bit integer types, which are currently allowed to overflow (NB: that means this kernel includes undefined behavior for int64).
There are a few other approaches we could take (ordered by personal preference):
- define explicit overflow behavior for signed integer operands (for example if we declared that `add(i8(a), i8(b))` will always be equivalent to `i8(i16(a) + i16(b))` then we could instantiate only unsigned addition kernels)
- raise an error on signed overflow
- provide `ArithmeticOptions::overflow_behavior` and allow users to choose between these
- require users to pass arguments which will not overflow
@wesm ?
This is probably nuanced enough to merit a mailing list discussion.
----------------------------------------------------------------
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] kszucs commented on a change in pull request #7341: ARROW-9022: [C++][Compute] Make Add function safe for numeric limits
Posted by GitBox <gi...@apache.org>.
kszucs commented on a change in pull request #7341:
URL: https://github.com/apache/arrow/pull/7341#discussion_r435454559
##########
File path: cpp/src/arrow/compute/api_scalar.h
##########
@@ -48,6 +48,26 @@ namespace compute {
ARROW_EXPORT
Result<Datum> Add(const Datum& left, const Datum& right, ExecContext* ctx = NULLPTR);
+/// \brief Subtract two values. Array values must be the same length. If a
Review comment:
Sure.
----------------------------------------------------------------
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