ARROW-9811: [C++] Unchecked floating point division by 0 should succeed #8036
Conversation
eced3b0 to
caf2587
Compare
caf2587 to
95da7ef
Compare
pitrou
left a comment
There was a problem hiding this comment.
Thank you for doing this. You'll find some comments below.
| struct Divide { | ||
| template <typename T, typename Arg0, typename Arg1> | ||
| static enable_if_floating_point<T> Call(KernelContext* ctx, Arg0 left, Arg1 right) { | ||
| if (ARROW_PREDICT_FALSE(right == 0)) { |
There was a problem hiding this comment.
This condition shouldn't be necessary anymore. You should be able to call feholdexcept at the beginning of the kernel, so that the FPU does the right thing. Also, restore the previous FP environment using fesetexcept at the end.
There was a problem hiding this comment.
That's awesome! Thank you for the good suggestion.
There was a problem hiding this comment.
After changing the code, the AMD64 Ubuntu 18.04 C++ ASAN UBSAN integration test is failing again.
It seems this is a bug of clang, and some special flags are required:
There was a problem hiding this comment.
@pitrou Thanks for your effort. It is working like a charm!
cpp/src/arrow/compare.cc
Outdated
| explicit ScalarEqualsVisitor(const Scalar& right) | ||
| : right_(right), result_(false), options_(EqualOptions()) {} | ||
|
|
||
| explicit ScalarEqualsVisitor(const Scalar& right, const EqualOptions& opts) |
There was a problem hiding this comment.
const EqualOptions& opt = EqualOptions::Defaults()
There was a problem hiding this comment.
Good suggestion. Thank you.
cpp/src/arrow/compare.h
Outdated
| /// \param[in] right a Scalar | ||
| /// \param[in] options comparison options | ||
| bool ARROW_EXPORT ScalarEquals(const Scalar& left, const Scalar& right, | ||
| const EqualOptions& options); |
There was a problem hiding this comment.
const EqualOptions& options = EqualOptions::Defaults(), just like ArrayEquals above.
There was a problem hiding this comment.
Accepted. Thank you.
|
|
||
| void SetOverflowCheck(bool value = true) { options_.check_overflow = value; } | ||
|
|
||
| void SetNansEqual(bool value = false) { |
There was a problem hiding this comment.
This is unnecessary, just always set it to true.
There was a problem hiding this comment.
Also, can you test Nan in the inputs in TestBinaryArithmeticFloating now?
There was a problem hiding this comment.
Sounds reasonable.
I have made the default value to true, and provided a test case for NaN in the input.
cpp/src/arrow/compare.cc
Outdated
| protected: | ||
| const Scalar& right_; | ||
| bool result_; | ||
| EqualOptions options_; |
There was a problem hiding this comment.
Sure. It should be const.
| const auto& right = checked_cast<const T&>(right_); | ||
| if (options_.nans_equal()) { | ||
| result_ = right.value == left_.value || | ||
| (std::isnan(right.value) && std::isnan(left_.value)); |
There was a problem hiding this comment.
Just confirmation. Is it fine with (NAN, -NAN) -> true?
There was a problem hiding this comment.
-NAN isn't really a thing. As per IEEE, every NaN is different and unequal (even to itself). Here we treat them as all equal, because that's vastly more convenient for testing.
See https://issues.apache.org/jira/browse/ARROW-9811