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/08/06 15:04:53 UTC
[GitHub] [arrow] bkietz commented on a change in pull request #7784: ARROW-9402: [C++] Rework portable wrappers for checked integer arithmetic
bkietz commented on a change in pull request #7784:
URL: https://github.com/apache/arrow/pull/7784#discussion_r465889920
##########
File path: cpp/src/arrow/compute/kernels/test_util.cc
##########
@@ -33,68 +33,109 @@ namespace compute {
namespace {
-void CheckScalarUnaryNonRecursive(const std::string& func_name,
- const std::shared_ptr<Array>& input,
- const std::shared_ptr<Array>& expected,
- const FunctionOptions* options) {
- ASSERT_OK_AND_ASSIGN(Datum out, CallFunction(func_name, {input}, options));
+template <typename T>
+std::vector<Datum> GetDatums(const std::vector<T>& inputs) {
+ std::vector<Datum> datums;
+ for (const auto& input : inputs) {
+ datums.emplace_back(input);
+ }
+ return datums;
+}
+
+void CheckScalarNonRecursive(const std::string& func_name,
+ const std::vector<std::shared_ptr<Array>>& inputs,
+ const std::shared_ptr<Array>& expected,
+ const FunctionOptions* options) {
+ ASSERT_OK_AND_ASSIGN(Datum out, CallFunction(func_name, GetDatums(inputs), options));
std::shared_ptr<Array> actual = std::move(out).make_array();
ASSERT_OK(actual->ValidateFull());
AssertArraysEqual(*expected, *actual, /*verbose=*/true);
}
-} // namespace
+template <typename... SliceArgs>
+std::vector<std::shared_ptr<Array>> SliceAll(
+ const std::vector<std::shared_ptr<Array>>& inputs, SliceArgs... slice_args) {
+ std::vector<std::shared_ptr<Array>> sliced;
+ for (const auto& input : inputs) {
+ sliced.push_back(input->Slice(slice_args...));
+ }
+ return sliced;
+}
-void CheckScalarUnary(std::string func_name, std::shared_ptr<Array> input,
- std::shared_ptr<Array> expected, const FunctionOptions* options) {
- CheckScalarUnaryNonRecursive(func_name, input, expected, options);
+std::vector<std::shared_ptr<Scalar>> GetScalars(
+ const std::vector<std::shared_ptr<Array>>& inputs, int64_t index) {
+ std::vector<std::shared_ptr<Scalar>> scalars;
Review comment:
```suggestion
ScalarVector GetScalars(
const ArrayVector& inputs, int64_t index) {
ScalarVector scalars;
```
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -53,35 +69,84 @@ class TestBinaryArithmetic : public TestBase {
void SetUp() { 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, Scalar)
void AssertBinop(BinaryFunction func, CType lhs, CType rhs, CType expected) {
- ASSERT_OK_AND_ASSIGN(auto left, MakeScalar(type_singleton(), lhs));
- ASSERT_OK_AND_ASSIGN(auto right, MakeScalar(type_singleton(), rhs));
- ASSERT_OK_AND_ASSIGN(auto exp, MakeScalar(type_singleton(), expected));
+ auto left = MakeScalar(lhs);
+ auto right = MakeScalar(rhs);
+ auto exp = MakeScalar(expected);
ASSERT_OK_AND_ASSIGN(auto actual, func(left, right, options_, nullptr));
- AssertScalarsEqual(*exp, *actual.scalar(), true);
+ AssertScalarsEqual(*exp, *actual.scalar(), /*verbose=*/true);
}
// (Scalar, Array)
void AssertBinop(BinaryFunction func, CType lhs, const std::string& rhs,
const std::string& expected) {
- ASSERT_OK_AND_ASSIGN(auto left, MakeScalar(type_singleton(), lhs));
+ auto left = MakeScalar(lhs);
+ AssertBinop(func, left, rhs, expected);
+ }
+
+ // (Scalar, Array)
+ void AssertBinop(BinaryFunction func, const std::shared_ptr<Scalar>& left,
+ const std::string& rhs, const std::string& expected) {
auto right = ArrayFromJSON(type_singleton(), rhs);
auto exp = ArrayFromJSON(type_singleton(), expected);
ASSERT_OK_AND_ASSIGN(auto actual, func(left, right, options_, nullptr));
ValidateAndAssertApproxEqual(actual.make_array(), expected);
}
+ // (Array, Scalar)
+ void AssertBinop(BinaryFunction func, const std::string& lhs, CType rhs,
+ const std::string& expected) {
+ auto right = MakeScalar(rhs);
+ AssertBinop(func, lhs, right, expected);
+ }
+
+ // (Array, Scalar)
+ void AssertBinop(BinaryFunction func, const std::string& lhs,
+ const std::shared_ptr<Scalar>& right, const std::string& expected) {
+ auto left = ArrayFromJSON(type_singleton(), lhs);
+ auto exp = ArrayFromJSON(type_singleton(), expected);
+
+ ASSERT_OK_AND_ASSIGN(auto actual, func(left, right, options_, nullptr));
+ ValidateAndAssertApproxEqual(actual.make_array(), expected);
+ }
+
// (Array, Array)
void AssertBinop(BinaryFunction func, const std::string& lhs, const std::string& rhs,
const std::string& expected) {
auto left = ArrayFromJSON(type_singleton(), lhs);
auto right = ArrayFromJSON(type_singleton(), rhs);
+ AssertBinop(func, ArrayFromJSON(type_singleton(), lhs),
+ ArrayFromJSON(type_singleton(), rhs), expected);
Review comment:
```suggestion
AssertBinop(func, left, right, expected);
```
##########
File path: cpp/src/arrow/compute/kernels/test_util.h
##########
@@ -103,6 +103,16 @@ void CheckScalarUnary(std::string func_name, std::shared_ptr<Scalar> input,
std::shared_ptr<Scalar> expected,
const FunctionOptions* options = nullptr);
+void CheckScalarBinary(std::string func_name, std::shared_ptr<Scalar> left_input,
Review comment:
:+1:
##########
File path: cpp/src/parquet/arrow/reader_internal.cc
##########
@@ -55,6 +55,8 @@
#include "parquet/schema.h"
#include "parquet/statistics.h"
#include "parquet/types.h"
+// Required after "arrow/util/int_util_internal.h" (for OPTIONAL)
Review comment:
```suggestion
// Required after "arrow/util/int_util_internal.h" (for OPTIONAL)
```
I'm not sure if clang-format will always recognize this comment as breaking this #include out of the above block. I suppose it's alphabetically always going to be lower in the sort, though
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -72,35 +77,19 @@ struct Add {
};
struct AddChecked {
-#if __has_builtin(__builtin_add_overflow)
- template <typename T>
- static enable_if_integer<T> Call(KernelContext* ctx, T left, T right) {
+ template <typename T, typename Arg0, typename Arg1>
+ enable_if_integer<T> Call(KernelContext* ctx, Arg0 left, Arg1 right) {
+ static_assert(std::is_same<T, Arg0>::value && std::is_same<T, Arg1>::value, "");
T result;
- if (ARROW_PREDICT_FALSE(__builtin_add_overflow(left, right, &result))) {
+ if (ARROW_PREDICT_FALSE(AddWithOverflow(left, right, &result))) {
ctx->SetStatus(Status::Invalid("overflow"));
}
return result;
}
-#else
- template <typename T>
- static enable_if_unsigned_integer<T> Call(KernelContext* ctx, T left, T right) {
- if (ARROW_PREDICT_FALSE(arrow::internal::HasPositiveAdditionOverflow(left, right))) {
- ctx->SetStatus(Status::Invalid("overflow"));
- }
- return left + right;
- }
-
- template <typename T>
- static enable_if_signed_integer<T> Call(KernelContext* ctx, T left, T right) {
- if (ARROW_PREDICT_FALSE(arrow::internal::HasSignedAdditionOverflow(left, right))) {
- ctx->SetStatus(Status::Invalid("overflow"));
- }
- return left + right;
- }
-#endif
- template <typename T>
- static constexpr enable_if_floating_point<T> Call(KernelContext*, T left, T right) {
+ template <typename T, typename Arg0, typename Arg1>
+ enable_if_floating_point<T> Call(KernelContext*, Arg0 left, Arg1 right) {
+ static_assert(std::is_same<T, Arg0>::value && std::is_same<T, Arg1>::value, "");
Review comment:
What happens if we remove those template arguments?
```suggestion
template <typename T>
enable_if_floating_point<T> Call(KernelContext*, T left, T right) {
```
##########
File path: cpp/src/arrow/compute/kernels/test_util.cc
##########
@@ -33,68 +33,109 @@ namespace compute {
namespace {
-void CheckScalarUnaryNonRecursive(const std::string& func_name,
- const std::shared_ptr<Array>& input,
- const std::shared_ptr<Array>& expected,
- const FunctionOptions* options) {
- ASSERT_OK_AND_ASSIGN(Datum out, CallFunction(func_name, {input}, options));
+template <typename T>
+std::vector<Datum> GetDatums(const std::vector<T>& inputs) {
+ std::vector<Datum> datums;
+ for (const auto& input : inputs) {
+ datums.emplace_back(input);
+ }
+ return datums;
+}
+
+void CheckScalarNonRecursive(const std::string& func_name,
+ const std::vector<std::shared_ptr<Array>>& inputs,
+ const std::shared_ptr<Array>& expected,
+ const FunctionOptions* options) {
+ ASSERT_OK_AND_ASSIGN(Datum out, CallFunction(func_name, GetDatums(inputs), options));
std::shared_ptr<Array> actual = std::move(out).make_array();
ASSERT_OK(actual->ValidateFull());
AssertArraysEqual(*expected, *actual, /*verbose=*/true);
}
-} // namespace
+template <typename... SliceArgs>
+std::vector<std::shared_ptr<Array>> SliceAll(
+ const std::vector<std::shared_ptr<Array>>& inputs, SliceArgs... slice_args) {
+ std::vector<std::shared_ptr<Array>> sliced;
Review comment:
Nit: use readability typedefs
```suggestion
ArrayVector SliceAll(
const ArrayVector& inputs, SliceArgs... slice_args) {
ArrayVector sliced;
```
----------------------------------------------------------------
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