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/24 13:04:46 UTC
[GitHub] [arrow] liyafan82 opened a new pull request #8036: ARROW-9811: [C++] Unchecked floating point division by 0 should succeed
liyafan82 opened a new pull request #8036:
URL: https://github.com/apache/arrow/pull/8036
See https://issues.apache.org/jira/browse/ARROW-9811
----------------------------------------------------------------
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] kiszk commented on a change in pull request #8036: ARROW-9811: [C++] Unchecked floating point division by 0 should succeed
Posted by GitBox <gi...@apache.org>.
kiszk commented on a change in pull request #8036:
URL: https://github.com/apache/arrow/pull/8036#discussion_r477005207
##########
File path: cpp/src/arrow/compare.cc
##########
@@ -875,9 +879,26 @@ class ScalarEqualsVisitor {
return Status::OK();
}
+ template <typename T>
+ typename std::enable_if<std::is_base_of<FloatScalar, T>::value ||
+ std::is_base_of<DoubleScalar, T>::value,
+ Status>::type
+ Visit(const T& left_) {
+ const auto& right = checked_cast<const T&>(right_);
+ if (options_.nans_equal()) {
+ result_ = right.value == left_.value ||
+ (std::isnan(right.value) && std::isnan(left_.value));
Review comment:
Just confirmation. Is it fine with `(NAN, -NAN) -> true`?
----------------------------------------------------------------
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] liyafan82 commented on a change in pull request #8036: ARROW-9811: [C++] Unchecked floating point division by 0 should succeed
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #8036:
URL: https://github.com/apache/arrow/pull/8036#discussion_r478277035
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -191,8 +193,15 @@ struct Divide {
template <typename T, typename Arg0, typename Arg1>
static enable_if_floating_point<T> Call(KernelContext* ctx, Arg0 left, Arg1 right) {
if (ARROW_PREDICT_FALSE(right == 0)) {
Review comment:
After changing the code, the AMD64 Ubuntu 18.04 C++ ASAN UBSAN integration test is failing again.
It seems this is a bug of clang, and some special flags are required:
https://bugs.llvm.org/show_bug.cgi?id=17000#c1
----------------------------------------------------------------
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 #8036: ARROW-9811: [C++] Unchecked floating point division by 0 should succeed
Posted by GitBox <gi...@apache.org>.
pitrou closed pull request #8036:
URL: https://github.com/apache/arrow/pull/8036
----------------------------------------------------------------
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] liyafan82 commented on a change in pull request #8036: ARROW-9811: [C++] Unchecked floating point division by 0 should succeed
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #8036:
URL: https://github.com/apache/arrow/pull/8036#discussion_r478382465
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -191,8 +193,15 @@ struct Divide {
template <typename T, typename Arg0, typename Arg1>
static enable_if_floating_point<T> Call(KernelContext* ctx, Arg0 left, Arg1 right) {
if (ARROW_PREDICT_FALSE(right == 0)) {
Review comment:
@pitrou Thanks for your effort. It is working like a charm!
----------------------------------------------------------------
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] liyafan82 commented on a change in pull request #8036: ARROW-9811: [C++] Unchecked floating point division by 0 should succeed
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #8036:
URL: https://github.com/apache/arrow/pull/8036#discussion_r478231513
##########
File path: cpp/src/arrow/compare.cc
##########
@@ -968,6 +989,7 @@ class ScalarEqualsVisitor {
protected:
const Scalar& right_;
bool result_;
+ EqualOptions options_;
Review comment:
Sure. It should be const.
----------------------------------------------------------------
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] liyafan82 commented on a change in pull request #8036: ARROW-9811: [C++] Unchecked floating point division by 0 should succeed
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #8036:
URL: https://github.com/apache/arrow/pull/8036#discussion_r478230891
##########
File path: cpp/src/arrow/compare.cc
##########
@@ -862,7 +862,11 @@ class TypeEqualsVisitor {
class ScalarEqualsVisitor {
public:
- explicit ScalarEqualsVisitor(const Scalar& right) : right_(right), result_(false) {}
+ explicit ScalarEqualsVisitor(const Scalar& right)
+ : right_(right), result_(false), options_(EqualOptions()) {}
+
+ explicit ScalarEqualsVisitor(const Scalar& right, const EqualOptions& opts)
Review comment:
Good suggestion. Thank you.
##########
File path: cpp/src/arrow/compare.h
##########
@@ -113,4 +113,11 @@ bool ARROW_EXPORT TypeEquals(const DataType& left, const DataType& right,
/// \param[in] right a Scalar
bool ARROW_EXPORT ScalarEquals(const Scalar& left, const Scalar& right);
+/// Returns true if scalars are equal
+/// \param[in] left a Scalar
+/// \param[in] right a Scalar
+/// \param[in] options comparison options
+bool ARROW_EXPORT ScalarEquals(const Scalar& left, const Scalar& right,
+ const EqualOptions& options);
Review comment:
Accepted. 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] liyafan82 commented on a change in pull request #8036: ARROW-9811: [C++] Unchecked floating point division by 0 should succeed
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #8036:
URL: https://github.com/apache/arrow/pull/8036#discussion_r478230727
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -191,8 +193,15 @@ struct Divide {
template <typename T, typename Arg0, typename Arg1>
static enable_if_floating_point<T> Call(KernelContext* ctx, Arg0 left, Arg1 right) {
if (ARROW_PREDICT_FALSE(right == 0)) {
Review comment:
That's awesome! Thank you for the good suggestion.
----------------------------------------------------------------
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 #8036: ARROW-9811: [C++] Unchecked floating point division by 0 should succeed
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8036:
URL: https://github.com/apache/arrow/pull/8036#discussion_r476467763
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -165,12 +166,17 @@ class TestBinaryArithmetic : public TestBase {
void ValidateAndAssertApproxEqual(const std::shared_ptr<Array>& actual,
const std::shared_ptr<Array>& expected) {
ASSERT_OK(actual->ValidateFull());
- AssertArraysApproxEqual(*expected, *actual, /*verbose=*/true);
+ AssertArraysApproxEqual(*expected, *actual, /*verbose=*/true, equal_options_);
}
void SetOverflowCheck(bool value = true) { options_.check_overflow = value; }
+ void SetNansEqual(bool value = false) {
Review comment:
Also, can you test `Nan` in the inputs in `TestBinaryArithmeticFloating` now?
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -191,8 +193,15 @@ struct Divide {
template <typename T, typename Arg0, typename Arg1>
static enable_if_floating_point<T> Call(KernelContext* ctx, Arg0 left, Arg1 right) {
if (ARROW_PREDICT_FALSE(right == 0)) {
Review comment:
This condition shouldn't be necessary anymore. You should be able to call [`feholdexcept`](https://en.cppreference.com/w/cpp/numeric/fenv/feholdexcept) at the beginning of the kernel, so that the FPU does the right thing. Also, restore the previous FP environment using `fesetexcept` at the end.
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -165,12 +166,17 @@ class TestBinaryArithmetic : public TestBase {
void ValidateAndAssertApproxEqual(const std::shared_ptr<Array>& actual,
const std::shared_ptr<Array>& expected) {
ASSERT_OK(actual->ValidateFull());
- AssertArraysApproxEqual(*expected, *actual, /*verbose=*/true);
+ AssertArraysApproxEqual(*expected, *actual, /*verbose=*/true, equal_options_);
}
void SetOverflowCheck(bool value = true) { options_.check_overflow = value; }
+ void SetNansEqual(bool value = false) {
Review comment:
This is unnecessary, just always set it to true.
##########
File path: cpp/src/arrow/compare.cc
##########
@@ -862,7 +862,11 @@ class TypeEqualsVisitor {
class ScalarEqualsVisitor {
public:
- explicit ScalarEqualsVisitor(const Scalar& right) : right_(right), result_(false) {}
+ explicit ScalarEqualsVisitor(const Scalar& right)
+ : right_(right), result_(false), options_(EqualOptions()) {}
+
+ explicit ScalarEqualsVisitor(const Scalar& right, const EqualOptions& opts)
Review comment:
`const EqualOptions& opt = EqualOptions::Defaults()`
##########
File path: cpp/src/arrow/compare.h
##########
@@ -113,4 +113,11 @@ bool ARROW_EXPORT TypeEquals(const DataType& left, const DataType& right,
/// \param[in] right a Scalar
bool ARROW_EXPORT ScalarEquals(const Scalar& left, const Scalar& right);
+/// Returns true if scalars are equal
+/// \param[in] left a Scalar
+/// \param[in] right a Scalar
+/// \param[in] options comparison options
+bool ARROW_EXPORT ScalarEquals(const Scalar& left, const Scalar& right,
+ const EqualOptions& options);
Review comment:
`const EqualOptions& options = EqualOptions::Defaults()`, just like `ArrayEquals` above.
##########
File path: cpp/src/arrow/compare.cc
##########
@@ -968,6 +989,7 @@ class ScalarEqualsVisitor {
protected:
const Scalar& right_;
bool result_;
+ EqualOptions options_;
Review comment:
Perhaps `const` here.
----------------------------------------------------------------
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] liyafan82 commented on a change in pull request #8036: ARROW-9811: [C++] Unchecked floating point division by 0 should succeed
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #8036:
URL: https://github.com/apache/arrow/pull/8036#discussion_r478231393
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -165,12 +166,17 @@ class TestBinaryArithmetic : public TestBase {
void ValidateAndAssertApproxEqual(const std::shared_ptr<Array>& actual,
const std::shared_ptr<Array>& expected) {
ASSERT_OK(actual->ValidateFull());
- AssertArraysApproxEqual(*expected, *actual, /*verbose=*/true);
+ AssertArraysApproxEqual(*expected, *actual, /*verbose=*/true, equal_options_);
}
void SetOverflowCheck(bool value = true) { options_.check_overflow = value; }
+ void SetNansEqual(bool value = false) {
Review comment:
Sounds reasonable.
I have made the default value to true, and provided a test case for NaN in the input.
----------------------------------------------------------------
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 #8036: ARROW-9811: [C++] Unchecked floating point division by 0 should succeed
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8036:
URL: https://github.com/apache/arrow/pull/8036#issuecomment-679113683
https://issues.apache.org/jira/browse/ARROW-9811
----------------------------------------------------------------
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 #8036: ARROW-9811: [C++] Unchecked floating point division by 0 should succeed
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8036:
URL: https://github.com/apache/arrow/pull/8036#discussion_r477141994
##########
File path: cpp/src/arrow/compare.cc
##########
@@ -875,9 +879,26 @@ class ScalarEqualsVisitor {
return Status::OK();
}
+ template <typename T>
+ typename std::enable_if<std::is_base_of<FloatScalar, T>::value ||
+ std::is_base_of<DoubleScalar, T>::value,
+ Status>::type
+ Visit(const T& left_) {
+ const auto& right = checked_cast<const T&>(right_);
+ if (options_.nans_equal()) {
+ result_ = right.value == left_.value ||
+ (std::isnan(right.value) && std::isnan(left_.value));
Review comment:
`-NAN` isn't really a thing. As per IEEE, every NaN is different and unequal (even to itself). Here we treat them as all equal, because that's vastly more convenient for testing.
----------------------------------------------------------------
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