Skip to content

ARROW-9495: [C++] Equality assertions don't handle Inf / -Inf properly #7826

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
liyafan82 wants to merge 1 commit into from
Closed

ARROW-9495: [C++] Equality assertions don't handle Inf / -Inf properly #7826

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 73 additions & 1 deletion cpp/src/arrow/array/array_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1501,7 +1501,7 @@ void CheckFloatingNanEquality() {

// NaN != non-NaN
ArrayFromVector<TYPE>(type, {false, true}, {0.5, nan_value}, &a);
ArrayFromVector<TYPE>(type, {false, true}, {0.5, 0.0}, &a);
ArrayFromVector<TYPE>(type, {false, true}, {0.5, 0.0}, &b);
ASSERT_FALSE(a->Equals(b));
ASSERT_FALSE(b->Equals(a));
ASSERT_FALSE(a->Equals(b, EqualOptions().nans_equal(true)));
Expand All @@ -1520,6 +1520,73 @@ void CheckFloatingNanEquality() {
ASSERT_TRUE(b->RangeEquals(a, 0, 1, 0));
}

template <typename TYPE>
void CheckFloatingInfinityEquality() {
std::shared_ptr<Array> a, b;
std::shared_ptr<DataType> type = TypeTraits<TYPE>::type_singleton();

const auto infinity = std::numeric_limits<typename TYPE::c_type>::infinity();

for (auto nans_equal : {false, true}) {
// Infinity in a null entry
ArrayFromVector<TYPE>(type, {true, false}, {0.5, infinity}, &a);
ArrayFromVector<TYPE>(type, {true, false}, {0.5, -infinity}, &b);
ASSERT_TRUE(a->Equals(b));
ASSERT_TRUE(b->Equals(a));
ASSERT_TRUE(a->ApproxEquals(b, EqualOptions().atol(1e-5).nans_equal(nans_equal)));
ASSERT_TRUE(b->ApproxEquals(a, EqualOptions().atol(1e-5).nans_equal(nans_equal)));
ASSERT_TRUE(a->RangeEquals(b, 0, 2, 0));
ASSERT_TRUE(b->RangeEquals(a, 0, 2, 0));
ASSERT_TRUE(a->RangeEquals(b, 1, 2, 1));
ASSERT_TRUE(b->RangeEquals(a, 1, 2, 1));

// Infinity in a valid entry
ArrayFromVector<TYPE>(type, {false, true}, {0.5, infinity}, &a);
ArrayFromVector<TYPE>(type, {false, true}, {0.5, infinity}, &b);
ASSERT_TRUE(a->Equals(b));
ASSERT_TRUE(b->Equals(a));
ASSERT_TRUE(a->ApproxEquals(b, EqualOptions().atol(1e-5).nans_equal(nans_equal)));
ASSERT_TRUE(b->ApproxEquals(a, EqualOptions().atol(1e-5).nans_equal(nans_equal)));
ASSERT_TRUE(a->ApproxEquals(b, EqualOptions().atol(1e-5).nans_equal(nans_equal)));
ASSERT_TRUE(b->ApproxEquals(a, EqualOptions().atol(1e-5).nans_equal(nans_equal)));
// Infinity in tested range
ASSERT_TRUE(a->RangeEquals(b, 0, 2, 0));
ASSERT_TRUE(b->RangeEquals(a, 0, 2, 0));
ASSERT_TRUE(a->RangeEquals(b, 1, 2, 1));
ASSERT_TRUE(b->RangeEquals(a, 1, 2, 1));
// Infinity not in tested range
ASSERT_TRUE(a->RangeEquals(b, 0, 1, 0));
ASSERT_TRUE(b->RangeEquals(a, 0, 1, 0));

// Infinity != non-infinity
ArrayFromVector<TYPE>(type, {false, true}, {0.5, -infinity}, &a);
ArrayFromVector<TYPE>(type, {false, true}, {0.5, 0.0}, &b);
ASSERT_FALSE(a->Equals(b));
ASSERT_FALSE(b->Equals(a));
ASSERT_FALSE(a->ApproxEquals(b, EqualOptions().atol(1e-5).nans_equal(nans_equal)));
ASSERT_FALSE(b->ApproxEquals(a));
ASSERT_FALSE(a->ApproxEquals(b, EqualOptions().atol(1e-5).nans_equal(nans_equal)));
ASSERT_FALSE(b->ApproxEquals(a, EqualOptions().atol(1e-5).nans_equal(nans_equal)));
// Infinity != Negative infinity
ArrayFromVector<TYPE>(type, {true, true}, {0.5, -infinity}, &a);
ArrayFromVector<TYPE>(type, {true, true}, {0.5, infinity}, &b);
ASSERT_FALSE(a->Equals(b));
ASSERT_FALSE(b->Equals(a));
ASSERT_FALSE(a->ApproxEquals(b));
ASSERT_FALSE(b->ApproxEquals(a));
ASSERT_FALSE(a->ApproxEquals(b, EqualOptions().atol(1e-5).nans_equal(nans_equal)));
ASSERT_FALSE(b->ApproxEquals(a, EqualOptions().atol(1e-5).nans_equal(nans_equal)));
// Infinity in tested range
ASSERT_FALSE(a->RangeEquals(b, 0, 2, 0));
ASSERT_FALSE(b->RangeEquals(a, 0, 2, 0));
ASSERT_FALSE(a->RangeEquals(b, 1, 2, 1));
ASSERT_FALSE(b->RangeEquals(a, 1, 2, 1));
// Infinity not in tested range
ASSERT_TRUE(a->RangeEquals(b, 0, 1, 0));
ASSERT_TRUE(b->RangeEquals(a, 0, 1, 0));
}
}

TEST(TestPrimitiveAdHoc, FloatingApproxEquals) {
CheckApproxEquals<FloatType>();
CheckApproxEquals<DoubleType>();
Expand All @@ -1535,6 +1602,11 @@ TEST(TestPrimitiveAdHoc, FloatingNanEquality) {
CheckFloatingNanEquality<DoubleType>();
}

TEST(TestPrimitiveAdHoc, FloatingInfinityEquality) {
CheckFloatingInfinityEquality<FloatType>();
CheckFloatingInfinityEquality<DoubleType>();
}

// ----------------------------------------------------------------------
// FixedSizeBinary tests

Expand Down
7 changes: 4 additions & 3 deletions cpp/src/arrow/compare.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,11 +110,12 @@ inline bool FloatingApproxEquals(const NumericArray<ArrowType>& left,

if (opts.nans_equal()) {
return BaseFloatingEquals<ArrowType>(left, right, [epsilon](T x, T y) -> bool {
return (fabs(x - y) <= epsilon) || (std::isnan(x) && std::isnan(y));
return (fabs(x - y) <= epsilon) || (x == y) || (std::isnan(x) && std::isnan(y));
});
} else {
return BaseFloatingEquals<ArrowType>(
left, right, [epsilon](T x, T y) -> bool { return fabs(x - y) <= epsilon; });
return BaseFloatingEquals<ArrowType>(left, right, [epsilon](T x, T y) -> bool {
return (fabs(x - y) <= epsilon) || (x == y);
});
Copy link
Member

@pitrou pitrou Aug 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You must also change the nans_equal path above.

Copy link
Contributor Author

@liyafan82 liyafan82 Aug 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revised accordingly. Thanks a lot for your kind reminder.

}
}

Expand Down