pitrou commented on code in PR #46829:
URL: https://github.com/apache/arrow/pull/46829#discussion_r2154479549
##########
cpp/src/arrow/compute/kernels/base_arithmetic_internal.h:
##########
@@ -489,6 +490,19 @@ struct Negate {
}
};
+// Can't tell the difference between HalfFloatType and UInt16Type in
+// enable_if_unsigned_integer_value above, so seperate operator needed
+struct NegateHalfFloat {
+ template <typename T, typename Arg>
+ static constexpr enable_if_unsigned_integer_value<T> Call(KernelContext*,
Arg arg,
+ Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ using ::arrow::util::Float16;
+ auto val = Float16::FromBits(static_cast<uint16_t>(arg));
+ return (-val).bits();
+ }
+};
Review Comment:
I think the templating can be simplified since this is for a single data
type.
```suggestion
struct NegateHalfFloat {
template <typename T, typename Arg>
static constexpr T Call(KernelContext*, Arg arg, Status*) {
static_assert(std::is_same_v<Arg, uint16_t>, "");
static_assert(std::is_same_v<T, uint16_t>, "");
using ::arrow::util::Float16;
auto val = Float16::FromBits(arg);
return (-val).bits();
}
};
```
##########
cpp/src/arrow/compute/kernels/base_arithmetic_internal.h:
##########
@@ -652,6 +666,18 @@ struct Sign {
}
};
+struct SignHalfFloat {
+ template <typename T, typename Arg>
+ static constexpr enable_if_unsigned_integer_value<Arg, T>
Call(KernelContext*, Arg arg,
Review Comment:
Same here.
##########
cpp/src/arrow/compute/kernels/scalar_validity.cc:
##########
@@ -60,13 +61,25 @@ Status IsValidExec(KernelContext* ctx, const ExecSpan&
batch, ExecResult* out) {
struct IsFiniteOperator {
template <typename OutType, typename InType>
static constexpr OutType Call(KernelContext*, const InType& value, Status*) {
+ // this is HalfFloatType, because int types call a different operator
+ if constexpr(std::is_same_v<InType, uint16_t>) {
+ using ::arrow::util::Float16;
+ return Float16::FromBits(static_cast<uint16_t>(value)).is_finite();
Review Comment:
Nit: you don't need the `static_cast` here since `value` is *already* a
`uint16_t`.
##########
cpp/src/arrow/compute/kernels/scalar_validity.cc:
##########
@@ -60,13 +61,25 @@ Status IsValidExec(KernelContext* ctx, const ExecSpan&
batch, ExecResult* out) {
struct IsFiniteOperator {
template <typename OutType, typename InType>
static constexpr OutType Call(KernelContext*, const InType& value, Status*) {
+ // this is HalfFloatType, because int types call a different operator
+ if constexpr(std::is_same_v<InType, uint16_t>) {
+ using ::arrow::util::Float16;
+ return Float16::FromBits(static_cast<uint16_t>(value)).is_finite();
+ }
+
return std::isfinite(value);
}
};
struct IsInfOperator {
template <typename OutType, typename InType>
static constexpr OutType Call(KernelContext*, const InType& value, Status*) {
+ // this is HalfFloatType, because int types call a different operator
+ if constexpr(std::is_same_v<InType, uint16_t>) {
+ using ::arrow::util::Float16;
+ return Float16::FromBits(static_cast<uint16_t>(value)).is_infinity();
Review Comment:
Same here and below, of course.
##########
cpp/src/arrow/compute/kernels/scalar_validity_test.cc:
##########
@@ -106,6 +107,12 @@ TEST(TestValidityKernels, IsFinite) {
CheckScalar("is_finite",
{ArrayFromJSON(duration(TimeUnit::SECOND), "[0, 1, 42, null]")},
ArrayFromJSON(boolean(), "[true, true, true, null]"));
+ CheckScalar("is_finite", {ArrayFromJSON(float32(), "[1.0, NaN, Inf, null]")},
Review Comment:
Why not reuse `TestFloatingPointValidityKernels` instead?
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc:
##########
@@ -2926,6 +2933,22 @@ TYPED_TEST(TestUnaryArithmeticFloating, Sign) {
this->AssertUnaryOp(sign, this->MakeScalar(max), this->MakeScalar(1));
}
+TYPED_TEST(TestUnaryArithmeticHalfFloat, Negate) {
Review Comment:
Can't we just reuse `TestUnaryArithmeticFloating` for half-float?
##########
cpp/src/arrow/compute/kernels/base_arithmetic_internal.h:
##########
@@ -489,6 +490,19 @@ struct Negate {
}
};
+// Can't tell the difference between HalfFloatType and UInt16Type in
+// enable_if_unsigned_integer_value above, so seperate operator needed
+struct NegateHalfFloat {
+ template <typename T, typename Arg>
+ static constexpr enable_if_unsigned_integer_value<T> Call(KernelContext*,
Arg arg,
+ Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ using ::arrow::util::Float16;
Review Comment:
This `using` declaration can probably be moved to the top of the file.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
