pitrou commented on code in PR #49290:
URL: https://github.com/apache/arrow/pull/49290#discussion_r3394914211
##########
cpp/src/arrow/compare.h:
##########
@@ -59,27 +63,29 @@ class EqualOptions {
return res;
}
- /// Whether the "atol" property is used in the comparison.
- ///
- /// This option only affects the Equals methods
- /// and has no effect on ApproxEquals methods.
- bool use_atol() const { return use_atol_; }
+ /// The absolute tolerance for approximate comparisons of floating-point
values.
+ std::optional<double> atol() const { return atol_; }
- /// Return a new EqualOptions object with the "use_atol" property changed.
- EqualOptions use_atol(bool v) const {
Review Comment:
Can we keep this method for now?
`use_atol(false)` could set `ulp_distance_` to `std::nullopt`, while
`use_atol(true)` would set `ulp_distance_` to `kDefaultAbsoluteTolerance`.
##########
cpp/src/arrow/chunked_array_test.cc:
##########
@@ -201,8 +201,7 @@ TEST_F(TestChunkedArray, ApproxEquals) {
ASSERT_OK_AND_ASSIGN(auto chunked_array_2, ChunkedArray::Make({chunk_2}));
auto options = EqualOptions::Defaults().atol(1e-3);
- ASSERT_FALSE(chunked_array_1->Equals(chunked_array_2));
Review Comment:
Why remove this test? By default `ChunkedArray::Equals` should perform exact
comparisons, so this test should still hold.
##########
cpp/src/arrow/array/array_test.cc:
##########
@@ -2198,12 +2198,10 @@ void CheckFloatApproxEqualsWithAtol() {
ArrayFromVector<TYPE>(type, {true}, {static_cast<c_type>(0.6)}, &b);
auto options = EqualOptions::Defaults().atol(0.2);
- ASSERT_FALSE(a->Equals(b));
- ASSERT_TRUE(a->Equals(b, options.use_atol(true)));
+ ASSERT_TRUE(a->Equals(b, options));
ASSERT_TRUE(a->ApproxEquals(b, options));
- ASSERT_FALSE(a->RangeEquals(0, 1, 0, b, options));
Review Comment:
Why not change this to a slightly different test:
```c++
ASSERT_FALSE(a->RangeEquals(0, 1, 0, b));
```
##########
cpp/src/arrow/compare.h:
##########
@@ -59,27 +63,29 @@ class EqualOptions {
return res;
}
- /// Whether the "atol" property is used in the comparison.
- ///
- /// This option only affects the Equals methods
- /// and has no effect on ApproxEquals methods.
- bool use_atol() const { return use_atol_; }
Review Comment:
Can we keep this method for now and just mark it deprecated?
##########
cpp/src/arrow/record_batch_test.cc:
##########
@@ -208,8 +208,7 @@ TEST_F(TestRecordBatchEqualOptions, Approx) {
EXPECT_FALSE(b1->ApproxEquals(*b2,
EqualOptions::Defaults().nans_equal(true)));
auto options = EqualOptions::Defaults().nans_equal(true).atol(0.1);
- EXPECT_FALSE(b1->Equals(*b2, options));
Review Comment:
Or perhaps change this test to:
```c++
EXPECT_FALSE(b1->Equals(*b2));
```
##########
cpp/src/arrow/scalar_test.cc:
##########
@@ -40,6 +41,7 @@
#include "arrow/testing/util.h"
#include "arrow/type_traits.h"
#include "arrow/util/float16.h"
+#include "gtest/gtest-spi.h"
Review Comment:
Why is this include needed? If it is, it should be moved up above and use
brackets (i.e. `#include <gtest/gtest-spi.h>`)
##########
cpp/src/arrow/array/array_test.cc:
##########
@@ -2198,12 +2198,10 @@ void CheckFloatApproxEqualsWithAtol() {
ArrayFromVector<TYPE>(type, {true}, {static_cast<c_type>(0.6)}, &b);
auto options = EqualOptions::Defaults().atol(0.2);
- ASSERT_FALSE(a->Equals(b));
Review Comment:
Why not keep this test?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
