pitrou commented on a change in pull request #10896:
URL: https://github.com/apache/arrow/pull/10896#discussion_r685338746
##########
File path: cpp/src/arrow/compute/kernels/scalar_validity.cc
##########
@@ -76,6 +77,22 @@ struct IsInfOperator {
struct IsNullOperator {
static Status Call(KernelContext* ctx, const Scalar& in, Scalar* out) {
+ bool is_nan_value = false;
+ bool is_floating = false;
+ if (in.type == float32()) {
+ is_nan_value = std::isnan(internal::UnboxScalar<FloatType>::Unbox(in));
+ is_floating = true;
+ } else if (in.type == float64()) {
+ is_nan_value = std::isnan(internal::UnboxScalar<DoubleType>::Unbox(in));
+ is_floating = true;
+ }
Review comment:
This can probably made much simpler, e.g.:
```c++
bool* out_value = &checked_cast<BooleanScalar*>(out)->value;
if (in.type->id() == Type::FLOAT) {
*out_value = !options.nan_is_null ||
!std::isnan(internal::UnboxScalar<FloatType>::Unbox(in));
}
else if (in.type->id() == Type::DOUBLE) {
*out_value = !options.nan_is_null ||
!std::isnan(internal::UnboxScalar<DoubleType>::Unbox(in));
}
else {
*out_value = true;
}
return Status::OK();
```
##########
File path: cpp/src/arrow/compute/kernels/scalar_validity.cc
##########
@@ -76,6 +77,22 @@ struct IsInfOperator {
struct IsNullOperator {
static Status Call(KernelContext* ctx, const Scalar& in, Scalar* out) {
+ bool is_nan_value = false;
+ bool is_floating = false;
+ if (in.type == float32()) {
Review comment:
This is comparing pointer values, which is incorrect, even though it
will do what you expect most of the time.
(one could e.g. instantiate a separate instance using
`std::make_shared<FloatType>()`)
##########
File path: cpp/src/arrow/compute/kernels/scalar_validity.cc
##########
@@ -202,23 +221,29 @@ const FunctionDoc is_inf_doc(
("For each input value, emit true iff the value is infinite (inf or
-inf)."),
{"values"});
-const FunctionDoc is_null_doc("Return true if null",
- ("For each input value, emit true iff the value
is null."),
- {"values"});
+const FunctionDoc is_null_doc(
+ "Return true if null, NaN can be considered as null",
+ ("For each input value, emit true iff the value is null. Default behavior
is to emit "
+ "false for NaN values. True can be emitted for NaN values by toggling "
+ "NanNullOptions flag."),
+ {"values"}, "NanNullOptions");
const FunctionDoc is_nan_doc("Return true if NaN",
("For each input value, emit true iff the value
is NaN."),
{"values"});
} // namespace
+FunctionOptions kNanNullOptions = NanNullOptions::Defaults();
Review comment:
You can put this in the anonymous namespace 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.
To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
