Skip to content

ARROW-11043: [C++] Add "is_nan" kernel - #9023

Closed
bu2 wants to merge 5 commits into
apache:masterfrom
bu2:ARROW-11043
Closed

ARROW-11043: [C++] Add "is_nan" kernel#9023
bu2 wants to merge 5 commits into
apache:masterfrom
bu2:ARROW-11043

Conversation

@bu2

@bu2bu2 commented Dec 28, 2020

Copy link
Copy Markdown
Contributor

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.

cyb70289
cyb70289 reviewed Dec 28, 2020
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_validity.cc Outdated
@github-actions

github-actionsBot commented Dec 28, 2020

Copy link
Copy Markdown

https://issues.apache.org/jira/browse/ARROW-11043

cyb70289
cyb70289 reviewed Dec 28, 2020
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_validity_test.cc Outdated
cyb70289
cyb70289 reviewed Dec 28, 2020
View reviewed changes
Comment threadcpp/src/arrow/compute/api_scalar.h Outdated
cyb70289
cyb70289 reviewed Dec 29, 2020
View reviewed changes
Comment threaddocs/source/cpp/compute.rst Outdated
pitrou
pitrou reviewed Jan 5, 2021
View reviewed changes

@pitroupitrou left a comment

Copy link
Copy Markdown
Member

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.

Comment threadcpp/src/arrow/compute/api_scalar.h Outdated
Comment threadcpp/src/arrow/compute/kernels/common.h Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_validity.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_validity_test.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_validity_test.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_validity_test.cc Outdated
Comment threaddocs/source/cpp/compute.rst Outdated
bu2 added 5 commits January 5, 2021 17:43
@bu2
ARROW-11043: [C++] Add "is_nan" kernel
32b8f1f
@bu2
ARROW-11043: [C++] Add "is_nan" kernel
e596a9e
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.
@bu2
ARROW-11043: [C++] Add "is_nan" kernel
c0cc3fe
Fix following reviewer's comment:
* update docs/source/cpp/compute.rst
@bu2
ARROW-11043: [C++] Add "is_nan" kernel
850354f
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)
@bu2
ARROW-11043: [C++] Add "is_nan" kernel
64d45c3
Fix following reviewer's comment:
* Move <cmath> include from common.h to kernel source.
@bu2

bu2 commented Jan 5, 2021

Copy link
Copy Markdown
ContributorAuthor

PR rebased!

pitrou
pitrou approved these changes Jan 5, 2021
View reviewed changes

@pitroupitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1, thank you

@pitroupitrou closed this in dab1eebJan 5, 2021
@jorisvandenbossche

jorisvandenbossche commented Jan 5, 2021

Copy link
Copy Markdown
Member

Thanks a lot @bu2 !

@bu2

bu2 commented Jan 5, 2021

Copy link
Copy Markdown
ContributorAuthor

Thanks to @cyb70289 for the initial review.

@bu2bu2 mentioned this pull request Jan 5, 2021
Closed
@asfimportasfimport mentioned this pull request Jan 5, 2021
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cyb70289cyb70289cyb70289 left review comments
@pitroupitroupitrou approved these changes

Assignees

No one assigned

Labels

Component: C++

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@bu2@jorisvandenbossche@pitrou@cyb70289