[GitHub] [arrow] pitrou commented on a change in pull request #8036: ARROW-9811: [C++] Unchecked floating point division by 0 should succeed

[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