You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "pitrou (via GitHub)" <gi...@apache.org> on 2025/06/19 15:11:54 UTC
[PR] GH-45083: [C++] Add HalfFloat kernels for is_nan, is_inf, is_finite, negate, negate_checked, sign [arrow]
pitrou opened a new pull request, #46866:
URL: https://github.com/apache/arrow/pull/46866
### Rationale for this change
Half-float is currently supported by none of the arithmetic compute functions.
### What changes are included in this PR?
Add half-float support for a small subset of arithmetic compute functions: `is_nan`, `is_inf`, `is_finite`, `negate`, `negate_checked`, `sign`.
Also improve the kernel generation infrastructure so as to make further half-float kernels easier to implement.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
No.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45083: [C++] Add HalfFloat kernels for is_nan, is_inf, is_finite, negate, negate_checked, sign [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #46866:
URL: https://github.com/apache/arrow/pull/46866#issuecomment-3004846022
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 2c51ceb5dd416bee287dd45f9fa3cd70140350c7.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/44770856354) has more details.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45083: [C++] Add HalfFloat kernels for is_nan, is_inf, is_finite, negate, negate_checked, sign [arrow]
Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on code in PR #46866:
URL: https://github.com/apache/arrow/pull/46866#discussion_r2166977331
##########
cpp/src/arrow/util/hashing.h:
##########
@@ -143,6 +144,21 @@ struct ScalarHelper<Scalar, AlgNum, enable_if_t<std::is_floating_point<Scalar>::
}
};
+template <typename Scalar, uint64_t AlgNum>
+struct ScalarHelper<Scalar, AlgNum,
+ enable_if_t<std::is_same_v<Scalar, ::arrow::util::Float16>>>
+ : public ScalarHelperBase<Scalar, AlgNum> {
+ // ScalarHelper specialization for Float16
+
+ static bool CompareScalars(Scalar u, Scalar v) {
+ if (u.is_nan()) {
+ // XXX should we do a bit-precise comparison?
Review Comment:
FWIW: I meant to say that I think the current behavior of considering all NaNs equivalent is correct and the comment should be removed.
##########
cpp/src/arrow/util/hashing.h:
##########
@@ -143,6 +144,21 @@ struct ScalarHelper<Scalar, AlgNum, enable_if_t<std::is_floating_point<Scalar>::
}
};
+template <typename Scalar, uint64_t AlgNum>
+struct ScalarHelper<Scalar, AlgNum,
+ enable_if_t<std::is_same_v<Scalar, ::arrow::util::Float16>>>
+ : public ScalarHelperBase<Scalar, AlgNum> {
+ // ScalarHelper specialization for Float16
+
+ static bool CompareScalars(Scalar u, Scalar v) {
+ if (u.is_nan()) {
+ // XXX should we do a bit-precise comparison?
Review Comment:
FWIW: I meant to say that I think the current behavior of considering all NaNs equivalent is correct and the comments should be removed.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45083: [C++] Add HalfFloat kernels for is_nan, is_inf, is_finite, negate, negate_checked, sign [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #46866:
URL: https://github.com/apache/arrow/pull/46866#discussion_r2166038605
##########
cpp/src/arrow/util/hashing.h:
##########
@@ -143,6 +144,21 @@ struct ScalarHelper<Scalar, AlgNum, enable_if_t<std::is_floating_point<Scalar>::
}
};
+template <typename Scalar, uint64_t AlgNum>
+struct ScalarHelper<Scalar, AlgNum,
+ enable_if_t<std::is_same_v<Scalar, ::arrow::util::Float16>>>
+ : public ScalarHelperBase<Scalar, AlgNum> {
+ // ScalarHelper specialization for Float16
+
+ static bool CompareScalars(Scalar u, Scalar v) {
+ if (u.is_nan()) {
+ // XXX should we do a bit-precise comparison?
Review Comment:
This is the same comment as for other FP types above. I think it should be a separate issue, because it's not obvious what the behaviour should really be.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45083: [C++] Add HalfFloat kernels for is_nan, is_inf, is_finite, negate, negate_checked, sign [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on PR #46866:
URL: https://github.com/apache/arrow/pull/46866#issuecomment-3000509725
I'll be out for a few days. Hopefully next week I can take a look.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45083: [C++] Add HalfFloat kernels for is_nan, is_inf, is_finite, negate, negate_checked, sign [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #46866:
URL: https://github.com/apache/arrow/pull/46866#issuecomment-3001217917
Rebased.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45083: [C++] Add HalfFloat kernels for is_nan, is_inf, is_finite, negate, negate_checked, sign [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #46866:
URL: https://github.com/apache/arrow/pull/46866#issuecomment-3001216514
@github-actions crossbow submit -g cpp
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45083: [C++] Add HalfFloat kernels for is_nan, is_inf, is_finite, negate, negate_checked, sign [arrow]
Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on code in PR #46866:
URL: https://github.com/apache/arrow/pull/46866#discussion_r2164691585
##########
cpp/src/arrow/util/hashing.h:
##########
@@ -143,6 +144,21 @@ struct ScalarHelper<Scalar, AlgNum, enable_if_t<std::is_floating_point<Scalar>::
}
};
+template <typename Scalar, uint64_t AlgNum>
+struct ScalarHelper<Scalar, AlgNum,
+ enable_if_t<std::is_same_v<Scalar, ::arrow::util::Float16>>>
+ : public ScalarHelperBase<Scalar, AlgNum> {
+ // ScalarHelper specialization for Float16
+
+ static bool CompareScalars(Scalar u, Scalar v) {
+ if (u.is_nan()) {
+ // XXX should we do a bit-precise comparison?
Review Comment:
I think we should consider any two NaNs equivalent. Bit-precise comparison can be accomplished by viewing as `uint16_t`
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc:
##########
@@ -57,8 +61,11 @@ using SignedIntegerTypes = testing::Types<Int8Type, Int16Type, Int32Type, Int64T
using UnsignedIntegerTypes =
testing::Types<UInt8Type, UInt16Type, UInt32Type, UInt64Type>;
-// TODO(kszucs): add half-float
Review Comment:
👍
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45083: [C++] Add HalfFloat kernels for is_nan, is_inf, is_finite, negate, negate_checked, sign [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #46866:
URL: https://github.com/apache/arrow/pull/46866#issuecomment-2988548626
Revision: 0a908746ab80bd5ad4177b8f8b10c2a0a7da2216
Submitted crossbow builds: [ursacomputing/crossbow @ actions-030e37b35b](https://github.com/ursacomputing/crossbow/branches/all?query=actions-030e37b35b)
|Task|Status|
|----|------|
|example-cpp-minimal-build-static|[](https://github.com/ursacomputing/crossbow/actions/runs/15761754350/job/44429853818)|
|example-cpp-minimal-build-static-system-dependency|[](https://github.com/ursacomputing/crossbow/actions/runs/15761754334/job/44429853681)|
|example-cpp-tutorial|[](https://github.com/ursacomputing/crossbow/actions/runs/15761754572/job/44429854347)|
|test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions/runs/15761754285/job/44429853466)|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/15761754741/job/44429854888)|
|test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/actions/runs/15761754605/job/44429854441)|
|test-cuda-cpp-ubuntu-22.04-cuda-11.7.1|[](https://github.com/ursacomputing/crossbow/actions/runs/15761754681/job/44429854728)|
|test-debian-12-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/15761753860/job/44429852145)|
|test-debian-12-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions/runs/15761754635/job/44429854607)|
|test-fedora-39-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/15761754353/job/44429853807)|
|test-ubuntu-22.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/15761754496/job/44429854174)|
|test-ubuntu-22.04-cpp-20|[](https://github.com/ursacomputing/crossbow/actions/runs/15761754106/job/44429852769)|
|test-ubuntu-22.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/15761754389/job/44429853875)|
|test-ubuntu-22.04-cpp-emscripten|[](https://github.com/ursacomputing/crossbow/actions/runs/15761753920/job/44429852207)|
|test-ubuntu-22.04-cpp-no-threading|[](https://github.com/ursacomputing/crossbow/actions/runs/15761753895/job/44429852137)|
|test-ubuntu-24.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/15761754311/job/44429853571)|
|test-ubuntu-24.04-cpp-bundled-offline|[](https://github.com/ursacomputing/crossbow/actions/runs/15761754208/job/44429853206)|
|test-ubuntu-24.04-cpp-gcc-13-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/15761754342/job/44429853706)|
|test-ubuntu-24.04-cpp-gcc-14|[](https://github.com/ursacomputing/crossbow/actions/runs/15761754577/job/44429854381)|
|test-ubuntu-24.04-cpp-minimal-with-formats|[](https://github.com/ursacomputing/crossbow/actions/runs/15761754324/job/44429853609)|
|test-ubuntu-24.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions/runs/15761754116/job/44429852777)|
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45083: [C++] Add HalfFloat kernels for is_nan, is_inf, is_finite, negate, negate_checked, sign [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #46866:
URL: https://github.com/apache/arrow/pull/46866
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45083: [C++] Add HalfFloat kernels for is_nan, is_inf, is_finite, negate, negate_checked, sign [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #46866:
URL: https://github.com/apache/arrow/pull/46866#issuecomment-3001222884
Revision: 17c04577d4579822b70a152cb5ab48b12b9f3276
Submitted crossbow builds: [ursacomputing/crossbow @ actions-8bf71d8a31](https://github.com/ursacomputing/crossbow/branches/all?query=actions-8bf71d8a31)
|Task|Status|
|----|------|
|example-cpp-minimal-build-static|[](https://github.com/ursacomputing/crossbow/actions/runs/15856741786/job/44703874512)|
|example-cpp-minimal-build-static-system-dependency|[](https://github.com/ursacomputing/crossbow/actions/runs/15856741591/job/44703873621)|
|example-cpp-tutorial|[](https://github.com/ursacomputing/crossbow/actions/runs/15856741608/job/44703873605)|
|test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions/runs/15856742014/job/44703875460)|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/15856742002/job/44703875450)|
|test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/actions/runs/15856741636/job/44703873762)|
|test-cuda-cpp-ubuntu-22.04-cuda-11.7.1|[](https://github.com/ursacomputing/crossbow/actions/runs/15856741601/job/44703873669)|
|test-debian-12-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/15856741818/job/44703874569)|
|test-debian-12-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions/runs/15856741836/job/44703874676)|
|test-fedora-39-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/15856741847/job/44703874678)|
|test-ubuntu-22.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/15856741794/job/44703874523)|
|test-ubuntu-22.04-cpp-20|[](https://github.com/ursacomputing/crossbow/actions/runs/15856741675/job/44703873933)|
|test-ubuntu-22.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/15856741807/job/44703874547)|
|test-ubuntu-22.04-cpp-emscripten|[](https://github.com/ursacomputing/crossbow/actions/runs/15856741641/job/44703873703)|
|test-ubuntu-22.04-cpp-no-threading|[](https://github.com/ursacomputing/crossbow/actions/runs/15856742020/job/44703875598)|
|test-ubuntu-24.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/15856741975/job/44703875157)|
|test-ubuntu-24.04-cpp-bundled-offline|[](https://github.com/ursacomputing/crossbow/actions/runs/15856741598/job/44703873595)|
|test-ubuntu-24.04-cpp-gcc-13-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/15856741898/job/44703874823)|
|test-ubuntu-24.04-cpp-gcc-14|[](https://github.com/ursacomputing/crossbow/actions/runs/15856741804/job/44703874501)|
|test-ubuntu-24.04-cpp-minimal-with-formats|[](https://github.com/ursacomputing/crossbow/actions/runs/15856741587/job/44703873632)|
|test-ubuntu-24.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions/runs/15856741728/job/44703874201)|
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45083: [C++] Add HalfFloat kernels for is_nan, is_inf, is_finite, negate, negate_checked, sign [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #46866:
URL: https://github.com/apache/arrow/pull/46866#issuecomment-2988542457
@github-actions crossbow submit -g cpp
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45083: [C++] Add HalfFloat kernels for is_nan, is_inf, is_finite, negate, negate_checked, sign [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #46866:
URL: https://github.com/apache/arrow/pull/46866#issuecomment-3003743440
Thanks for the review @bkietz . I'll merge now, because changing the hashtable behavior of NaN values is more of an open question and should be a separate issue.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45083: [C++] Add HalfFloat kernels for is_nan, is_inf, is_finite, negate, negate_checked, sign [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #46866:
URL: https://github.com/apache/arrow/pull/46866#discussion_r2166987625
##########
cpp/src/arrow/util/hashing.h:
##########
@@ -143,6 +144,21 @@ struct ScalarHelper<Scalar, AlgNum, enable_if_t<std::is_floating_point<Scalar>::
}
};
+template <typename Scalar, uint64_t AlgNum>
+struct ScalarHelper<Scalar, AlgNum,
+ enable_if_t<std::is_same_v<Scalar, ::arrow::util::Float16>>>
+ : public ScalarHelperBase<Scalar, AlgNum> {
+ // ScalarHelper specialization for Float16
+
+ static bool CompareScalars(Scalar u, Scalar v) {
+ if (u.is_nan()) {
+ // XXX should we do a bit-precise comparison?
Review Comment:
Ah, sorry for misunderstanding!
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org