kou commented on code in PR #46422:
URL: https://github.com/apache/arrow/pull/46422#discussion_r2128118634
##########
cpp/src/arrow/array/statistics_test.cc:
##########
@@ -96,6 +99,61 @@ TEST(ArrayStatisticsTest, TestEquality) {
ASSERT_NE(statistics1, statistics2);
statistics2.is_max_exact = true;
ASSERT_EQ(statistics1, statistics2);
+
+ // Test different ArrayStatistics::ValueType
+ statistics1.max = static_cast<uint64_t>(29);
+ statistics1.max = static_cast<int64_t>(29);
+ ASSERT_NE(statistics1, statistics2);
+}
+class TestEqualityDoubleValue : public ::testing::Test {
Review Comment:
We want to use `TestArrayStatistics` prefix for all tests in this file
because we mix multiple test source files for one test binary.
See also:
https://github.com/apache/arrow/blob/23017e3c48c1033d48674415ee4453476dc61bf3/cpp/src/arrow/CMakeLists.txt#L1164-L1175
```suggestion
class TestArrayStatisticsEqualityDoubleValue : public ::testing::Test {
```
##########
cpp/src/arrow/compare.cc:
##########
@@ -1522,5 +1524,54 @@ bool TypeEquals(const DataType& left, const DataType&
right, bool check_metadata
return visitor.result();
}
}
+namespace {
+
+using ValueType = ArrayStatistics::ValueType;
+
+bool DoubleEquals(const double& left, const double& right, const EqualOptions&
options) {
+ bool result;
+ auto visitor = [&](auto&& compare_func) { result = compare_func(left,
right); };
+ VisitFloatingEquality<double>(options, options.use_atol(),
std::move(visitor));
+ return result;
+}
+bool ArrayStatisticsValueTypeEquals(const std::optional<ValueType>& left,
+ const std::optional<ValueType>& right,
Review Comment:
`ValueType` is only used here.
Can we remove `using ValueType = ArrayStatistics::ValueType;`?
##########
cpp/src/arrow/compare.h:
##########
@@ -57,8 +57,18 @@ class EqualOptions {
res.signed_zeros_equal_ = v;
return res;
}
+ /// Whether the "atol" property is used in the comparison.
Review Comment:
```suggestion
/// Whether the "atol" property is used in the comparison.
```
##########
cpp/src/arrow/array/statistics_test.cc:
##########
@@ -96,6 +99,61 @@ TEST(ArrayStatisticsTest, TestEquality) {
ASSERT_NE(statistics1, statistics2);
statistics2.is_max_exact = true;
ASSERT_EQ(statistics1, statistics2);
+
+ // Test different ArrayStatistics::ValueType
+ statistics1.max = static_cast<uint64_t>(29);
+ statistics1.max = static_cast<int64_t>(29);
+ ASSERT_NE(statistics1, statistics2);
+}
+class TestEqualityDoubleValue : public ::testing::Test {
+ protected:
+ ArrayStatistics statistics1_;
+ ArrayStatistics statistics2_;
+ EqualOptions options_ = EqualOptions::Defaults();
+};
+
+TEST_F(TestEqualityDoubleValue, ExactValue) {
+ statistics2_.min = 29.0;
+ ASSERT_NE(statistics1_, statistics2_);
+ statistics1_.min = 29.0;
+ ASSERT_EQ(statistics1_, statistics2_);
+ statistics2_.min = 30.0;
+ ASSERT_NE(statistics1_, statistics2_);
+}
+
+TEST_F(TestEqualityDoubleValue, SignedZero) {
+ statistics1_.min = +0.0;
+ statistics2_.min = -0.0;
+ ASSERT_TRUE(statistics1_.Equals(statistics2_,
options_.signed_zeros_equal(true)));
+ ASSERT_FALSE(statistics1_.Equals(statistics2_,
options_.signed_zeros_equal(false)));
+}
+
+TEST_F(TestEqualityDoubleValue, Infinity) {
+ auto infinity = std::numeric_limits<double>::infinity();
+ statistics1_.min = infinity;
+ statistics2_.min = infinity;
+ ASSERT_EQ(statistics1_, statistics2_);
+ statistics1_.min = -infinity;
+ ASSERT_NE(statistics1_, statistics2_);
+ statistics1_.min = 0.0;
+ ASSERT_NE(statistics1_, statistics2_);
+}
+
+TEST_F(TestEqualityDoubleValue, NaN) {
+ statistics1_.min = std::numeric_limits<double>::quiet_NaN();
+ statistics2_.min = std::numeric_limits<double>::quiet_NaN();
+ ASSERT_TRUE(statistics1_.Equals(statistics2_, options_.nans_equal(true)));
+ ASSERT_FALSE(statistics1_.Equals(statistics2_, options_.nans_equal(false)));
+}
+TEST_F(TestEqualityDoubleValue, ApproximateEquals) {
+ statistics1_.max = 0.5001f;
+ statistics2_.max = 0.5;
+ ASSERT_FALSE(statistics1_.Equals(statistics2_,
options_.atol(1e-3).use_atol(false)));
+
+ ASSERT_TRUE(statistics1_.Equals(statistics2_, options_.atol(1e-3)));
+ ASSERT_TRUE(statistics2_.Equals(statistics1_, options_.atol(1e-3)));
+ ASSERT_FALSE(statistics1_.Equals(statistics2_, options_.atol(1e-5)));
+ ASSERT_FALSE(statistics2_.Equals(statistics1_, options_.atol(1e-5)));
Review Comment:
Why do we need this?
##########
cpp/src/arrow/array/statistics_test.cc:
##########
@@ -96,6 +99,61 @@ TEST(ArrayStatisticsTest, TestEquality) {
ASSERT_NE(statistics1, statistics2);
statistics2.is_max_exact = true;
ASSERT_EQ(statistics1, statistics2);
+
+ // Test different ArrayStatistics::ValueType
+ statistics1.max = static_cast<uint64_t>(29);
+ statistics1.max = static_cast<int64_t>(29);
+ ASSERT_NE(statistics1, statistics2);
+}
+class TestEqualityDoubleValue : public ::testing::Test {
+ protected:
+ ArrayStatistics statistics1_;
+ ArrayStatistics statistics2_;
+ EqualOptions options_ = EqualOptions::Defaults();
+};
+
+TEST_F(TestEqualityDoubleValue, ExactValue) {
+ statistics2_.min = 29.0;
+ ASSERT_NE(statistics1_, statistics2_);
+ statistics1_.min = 29.0;
+ ASSERT_EQ(statistics1_, statistics2_);
+ statistics2_.min = 30.0;
+ ASSERT_NE(statistics1_, statistics2_);
+}
+
+TEST_F(TestEqualityDoubleValue, SignedZero) {
+ statistics1_.min = +0.0;
+ statistics2_.min = -0.0;
+ ASSERT_TRUE(statistics1_.Equals(statistics2_,
options_.signed_zeros_equal(true)));
+ ASSERT_FALSE(statistics1_.Equals(statistics2_,
options_.signed_zeros_equal(false)));
+}
+
+TEST_F(TestEqualityDoubleValue, Infinity) {
+ auto infinity = std::numeric_limits<double>::infinity();
+ statistics1_.min = infinity;
+ statistics2_.min = infinity;
+ ASSERT_EQ(statistics1_, statistics2_);
+ statistics1_.min = -infinity;
+ ASSERT_NE(statistics1_, statistics2_);
+ statistics1_.min = 0.0;
+ ASSERT_NE(statistics1_, statistics2_);
+}
+
+TEST_F(TestEqualityDoubleValue, NaN) {
+ statistics1_.min = std::numeric_limits<double>::quiet_NaN();
+ statistics2_.min = std::numeric_limits<double>::quiet_NaN();
+ ASSERT_TRUE(statistics1_.Equals(statistics2_, options_.nans_equal(true)));
+ ASSERT_FALSE(statistics1_.Equals(statistics2_, options_.nans_equal(false)));
+}
+TEST_F(TestEqualityDoubleValue, ApproximateEquals) {
+ statistics1_.max = 0.5001f;
+ statistics2_.max = 0.5;
+ ASSERT_FALSE(statistics1_.Equals(statistics2_,
options_.atol(1e-3).use_atol(false)));
+
+ ASSERT_TRUE(statistics1_.Equals(statistics2_, options_.atol(1e-3)));
+ ASSERT_TRUE(statistics2_.Equals(statistics1_, options_.atol(1e-3)));
Review Comment:
Why do we need this?
##########
cpp/src/arrow/array/statistics_test.cc:
##########
@@ -15,9 +15,12 @@
// specific language governing permissions and limitations
// under the License.
+#include <cmath>
+
Review Comment:
Can we remove this?
```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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
