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.
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
ARROW-9811: [C++] Unchecked floating point division by 0 should succeed #8036
Conversation
eced3b0
to
caf2587
Compare
caf2587
to
95da7ef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for doing this. You'll find some comments below.
@@ -191,8 +193,15 @@ 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's awesome! Thank you for the good suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const EqualOptions& opt = EqualOptions::Defaults()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const EqualOptions& options = EqualOptions::Defaults()
, just like ArrayEquals
above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accepted. Thank you.
} | ||
|
||
void SetOverflowCheck(bool value = true) { options_.check_overflow = value; } | ||
|
||
void SetNansEqual(bool value = false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unnecessary, just always set it to true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, can you test Nan
in the inputs in TestBinaryArithmeticFloating
now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -968,6 +989,7 @@ class ScalarEqualsVisitor { | |||
protected: | |||
const Scalar& right_; | |||
bool result_; | |||
EqualOptions options_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps const
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just confirmation. Is it fine with (NAN, -NAN) -> true
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-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 Closes apache#8036 from liyafan82/fly_0824_div0 Lead-authored-by: liyafan82 <fan_li_ya@foxmail.com> Co-authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>
See https://issues.apache.org/jira/browse/ARROW-9811 Closes apache#8036 from liyafan82/fly_0824_div0 Lead-authored-by: liyafan82 <fan_li_ya@foxmail.com> Co-authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>
See https://issues.apache.org/jira/browse/ARROW-9811