Skip to content

Conversation

@bu2
Copy link
Contributor

@bu2 bu2 commented Dec 28, 2020

Add a "is_nan" kernel to check for NaN "equality" for FloatArray and DoubleArray (based on std::isnan()). The kernel signature is based on "is_null" kernel so I put my code in arrow/compute/kernels/scalar_validity.cc... but the implementation take some inspiration from "compare" kernel.

@github-actions
Copy link

Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

Thank you for the PR @bu2 ! This looks basically ok, here are a number of small comments.

bu2 added 5 commits January 5, 2021 17:43
Fix following reviewer's comment:
* iff not a typo,
* test for nulls,
* update docs/source/cpp/compute.rst

Then:
* apply clang-format to pass CI lint check.
Fix following reviewer's comment:
    * update docs/source/cpp/compute.rst
Fix following reviewer's comment:
* Update "since version number"
* Improve kernel documentation (remove "according to std::isnan()")
* Improve code style (replace typedef aliases by using aliases)
* Minor fix (float32 literal without f suffix)
Fix following reviewer's comment:
* Move <cmath> include from common.h to kernel source.
@bu2
Copy link
Contributor Author

bu2 commented Jan 5, 2021

PR rebased!

Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

+1, thank you

@pitrou pitrou closed this in dab1eeb Jan 5, 2021
@jorisvandenbossche
Copy link
Member

Thanks a lot @bu2 !

@bu2
Copy link
Contributor Author

bu2 commented Jan 5, 2021

Thanks to @cyb70289 for the initial review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants