You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/07/24 09:46:30 UTC
[GitHub] [arrow] liyafan82 opened a new pull request #7826: ARROW-9495: [C++] Equality assertions don't handle Inf / -Inf properly
liyafan82 opened a new pull request #7826:
URL: https://github.com/apache/arrow/pull/7826
See https://issues.apache.org/jira/browse/ARROW-9495
----------------------------------------------------------------
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
[GitHub] [arrow] github-actions[bot] commented on pull request #7826: ARROW-9495: [C++] Equality assertions don't handle Inf / -Inf properly
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7826:
URL: https://github.com/apache/arrow/pull/7826#issuecomment-663461523
https://issues.apache.org/jira/browse/ARROW-9495
----------------------------------------------------------------
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
[GitHub] [arrow] liyafan82 commented on pull request #7826: ARROW-9495: [C++] Equality assertions don't handle Inf / -Inf properly
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on pull request #7826:
URL: https://github.com/apache/arrow/pull/7826#issuecomment-673213609
@kiszk Thanks a lot for your feedback.
@pitrou could you please give some comments?
----------------------------------------------------------------
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
[GitHub] [arrow] pitrou closed pull request #7826: ARROW-9495: [C++] Equality assertions don't handle Inf / -Inf properly
Posted by GitBox <gi...@apache.org>.
pitrou closed pull request #7826:
URL: https://github.com/apache/arrow/pull/7826
----------------------------------------------------------------
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
[GitHub] [arrow] liyafan82 commented on a change in pull request #7826: ARROW-9495: [C++] Equality assertions don't handle Inf / -Inf properly
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #7826:
URL: https://github.com/apache/arrow/pull/7826#discussion_r469918164
##########
File path: cpp/src/arrow/compare.cc
##########
@@ -113,8 +113,9 @@ inline bool FloatingApproxEquals(const NumericArray<ArrowType>& left,
return (fabs(x - y) <= epsilon) || (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);
+ });
Review comment:
Revised accordingly. Thanks a lot for your kind reminder.
----------------------------------------------------------------
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
[GitHub] [arrow] kiszk commented on a change in pull request #7826: ARROW-9495: [C++] Equality assertions don't handle Inf / -Inf properly
Posted by GitBox <gi...@apache.org>.
kiszk commented on a change in pull request #7826:
URL: https://github.com/apache/arrow/pull/7826#discussion_r460345631
##########
File path: cpp/src/arrow/array/array_test.cc
##########
@@ -1520,6 +1520,72 @@ 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();
+
+ // 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));
+ ASSERT_TRUE(b->ApproxEquals(a));
+ 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);
+ std::cout << a->ToString() << std::endl;
Review comment:
nit: we could remove this line.
----------------------------------------------------------------
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
[GitHub] [arrow] kiszk commented on pull request #7826: ARROW-9495: [C++] Equality assertions don't handle Inf / -Inf properly
Posted by GitBox <gi...@apache.org>.
kiszk commented on pull request #7826:
URL: https://github.com/apache/arrow/pull/7826#issuecomment-673034321
Looks good to me
----------------------------------------------------------------
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
[GitHub] [arrow] pitrou commented on a change in pull request #7826: ARROW-9495: [C++] Equality assertions don't handle Inf / -Inf properly
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #7826:
URL: https://github.com/apache/arrow/pull/7826#discussion_r469893346
##########
File path: cpp/src/arrow/compare.cc
##########
@@ -113,8 +113,9 @@ inline bool FloatingApproxEquals(const NumericArray<ArrowType>& left,
return (fabs(x - y) <= epsilon) || (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);
+ });
Review comment:
You must also change the `nans_equal` path above.
----------------------------------------------------------------
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
[GitHub] [arrow] liyafan82 commented on a change in pull request #7826: ARROW-9495: [C++] Equality assertions don't handle Inf / -Inf properly
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #7826:
URL: https://github.com/apache/arrow/pull/7826#discussion_r460610939
##########
File path: cpp/src/arrow/array/array_test.cc
##########
@@ -1520,6 +1520,72 @@ 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();
+
+ // 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));
+ ASSERT_TRUE(b->ApproxEquals(a));
+ 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);
+ std::cout << a->ToString() << std::endl;
Review comment:
Removed. Thanks for your kind reminder.
----------------------------------------------------------------
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