andishgar commented on code in PR #47969:
URL: https://github.com/apache/arrow/pull/47969#discussion_r2508031667
##########
cpp/src/arrow/array/statistics_test.cc:
##########
@@ -25,12 +25,20 @@
namespace arrow {
-TEST(TestArrayStatistics, NullCount) {
+TEST(TestArrayStatistics, NullCountExact) {
ArrayStatistics statistics;
ASSERT_FALSE(statistics.null_count.has_value());
statistics.null_count = 29;
ASSERT_TRUE(statistics.null_count.has_value());
- ASSERT_EQ(29, statistics.null_count.value());
+ ASSERT_EQ(29, std::get<int64_t>(statistics.null_count.value()));
+}
+
+TEST(TestArrayStatistics, NullCountApproximate) {
+ ArrayStatistics statistics;
+ ASSERT_FALSE(statistics.null_count.has_value());
+ statistics.null_count = 29.0;
+ ASSERT_TRUE(statistics.null_count.has_value());
+ ASSERT_EQ(29.0, std::get<double>(statistics.null_count.value()));
Review Comment:
> I mean a line like the one below. I'm aware of why such an assertion is
used: since floating-point numbers are approximated, this assertion relies on
ULPs (units in the last place) to prevent unintended behavior for
floating-point comparisons across different OSes and CPU architectures. Given
this, should an assertion like the one below be replaced with
`ASSERT_Double_EQ`?
>
>
https://github.com/apache/arrow/blob/5eaf553bfc7aa639fd67bd622b6b808e71fbba39/cpp/src/arrow/array/statistics_test.cc#L138-L141
@kou Regarding the previous comment, the other tests are correct. That was
my mistake; sorry for the confusion.
--
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]
