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/07/16 11:49:19 UTC
[GitHub] [arrow] pitrou opened a new pull request #7784: ARROW-9402: [C++] Rework portable wrappers for checked integer arithmetic
pitrou opened a new pull request #7784:
URL: https://github.com/apache/arrow/pull/7784
Vendor relevant code from the portable-snippets library (~ public domain):
https://github.com/nemequ/portable-snippets/tree/master/safe-math
Also fix some bugs in checked arithmetic (null values had their value slots checked as well),
add compute scaffolding for stateful binary scalar functions.
----------------------------------------------------------------
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 #7784: ARROW-9402: [C++] Rework portable wrappers for checked integer arithmetic
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #7784:
URL: https://github.com/apache/arrow/pull/7784#issuecomment-669322223
Rebased. @wesm could you perhaps review 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #7784: ARROW-9402: [C++] Rework portable wrappers for checked integer arithmetic
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #7784:
URL: https://github.com/apache/arrow/pull/7784#issuecomment-659504776
I think I finally squashed the horrid Windows preprocessor macro issues (Windows headers define OPTIONAL, but you can't undef it blindly because other subsequent includes may require it...).
AppVeyor build: https://ci.appveyor.com/project/pitrou/arrow/builds/34135787
----------------------------------------------------------------
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 #7784: ARROW-9402: [C++] Rework portable wrappers for checked integer arithmetic
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #7784:
URL: https://github.com/apache/arrow/pull/7784#issuecomment-669322336
Or perhaps @bkietz .
----------------------------------------------------------------
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 #7784: ARROW-9402: [C++] Rework portable wrappers for checked integer arithmetic
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #7784:
URL: https://github.com/apache/arrow/pull/7784#issuecomment-671356665
I believe I addressed your comments @bkietz , so I'm going to merge 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] pitrou commented on a change in pull request #7784: ARROW-9402: [C++] Rework portable wrappers for checked integer arithmetic
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #7784:
URL: https://github.com/apache/arrow/pull/7784#discussion_r467869720
##########
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:
Good idea, thank you.
----------------------------------------------------------------
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 #7784: ARROW-9402: [C++] Rework portable wrappers for checked integer arithmetic
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [arrow] github-actions[bot] commented on pull request #7784: ARROW-9402: [C++] Rework portable wrappers for checked integer arithmetic
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7784:
URL: https://github.com/apache/arrow/pull/7784#issuecomment-659371394
https://issues.apache.org/jira/browse/ARROW-9402
----------------------------------------------------------------
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 a change in pull request #7784: ARROW-9402: [C++] Rework portable wrappers for checked integer arithmetic
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #7784:
URL: https://github.com/apache/arrow/pull/7784#discussion_r467870651
##########
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:
Indeed :-)
----------------------------------------------------------------
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 #7784: ARROW-9402: [C++] Rework portable wrappers for checked integer arithmetic
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #7784:
URL: https://github.com/apache/arrow/pull/7784#issuecomment-659505609
@wesm You'll probably want to take a look at the codegen changes when you have some time.
----------------------------------------------------------------
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 a change in pull request #7784: ARROW-9402: [C++] Rework portable wrappers for checked integer arithmetic
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #7784:
URL: https://github.com/apache/arrow/pull/7784#discussion_r467867983
##########
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:
It doesn't compile, because `Call` is invoked with explicit template arguments by the kernel execution machinery.
----------------------------------------------------------------
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 closed pull request #7784: ARROW-9402: [C++] Rework portable wrappers for checked integer arithmetic
Posted by GitBox <gi...@apache.org>.
pitrou closed pull request #7784:
URL: https://github.com/apache/arrow/pull/7784
----------------------------------------------------------------
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