GH-43017: [C++] Make the set of casts and hash kernels involving float16 consistent with other floating types #43018
Conversation
|
|
…pe to GenerateNumeric<>
…fFloatType to GenerateNumeric<>" This reverts commit 436071f.
felipecrv
force-pushed
the
half_float_casts
branch
from
19c70b1 to
7fe3942
Compare
| @@ -913,6 +913,16 @@ TEST(RowSegmenter, RowConstantBatch) { | |||
| } | |||
| } | |||
|
|
|||
| // XXX: float16 is not part of NumericTypes() yet | |||
| auto& AllNumericTypes() { | |||
There was a problem hiding this comment.
Can we make this const auto&, to avoid potentially mutating it from a callee?
| @@ -959,6 +959,10 @@ KernelType GenerateNumeric(detail::GetTypeId get_id) { | |||
| return Generator<Type0, FloatType, Args...>::Exec; | |||
| case Type::DOUBLE: | |||
| return Generator<Type0, DoubleType, Args...>::Exec; | |||
| case Type::HALF_FLOAT: | |||
| // NOTE: Type::HALF_FLOAT used to not be part of the list of numeric types, | |||
| // so users of this template might start failing to compiler after Arrow 17.x. | |||
There was a problem hiding this comment.
These are internal helpers, so if Arrow still compiles, I don't think the comment is warranted.
There was a problem hiding this comment.
Cool. It took me a while to realize these were internal. I will remove the comments.
| @@ -590,6 +590,13 @@ void AddArraySortingKernels(VectorKernel base, VectorFunction* func) { | |||
| base.exec = GenerateNumeric<ExecTemplate, UInt64Type>(*physical_type); | |||
| DCHECK_OK(func->AddKernel(base)); | |||
| } | |||
| { | |||
| // XXX: float16() is not in NumericTypes() yet | |||
| auto physical_type = GetPhysicalType(float16()); | |||
There was a problem hiding this comment.
What is the underlying physical type btw? We should be careful that float16 NaNs sort as described as https://arrow.apache.org/docs/cpp/compute.html#sorts-and-partitions, and we should test for that.
There was a problem hiding this comment.
It's float16 itself. I'm changing the code to make that more obvious.
| @@ -697,6 +698,12 @@ void AddHashKernels(VectorFunction* func, VectorKernel base, OutputType out_ty) | |||
| base.signature = KernelSignature::Make({ty}, out_ty); | |||
| DCHECK_OK(func->AddKernel(base)); | |||
| } | |||
| { | |||
| // XXX: float16() is not in PrimitiveTypes() | |||
There was a problem hiding this comment.
We should eventually deal with this somehow. I think we could add a less stable AllPrimitiveTypes() that includes FLOAT16 and eventually mark PrimitiveTypes() as deprecated.
github-actions
bot
added
awaiting changes
github-actions
bot
added
awaiting change review
|
@felipecrv Do we have tests for sorting float16 values and NaNs already? Otherwise, we should add some. Also, is only |
|
@felipecrv Is this ready for review again? |
No. Adding these tests you requested creates a cascade of requirements regarding half-float support that I haven't been able to fix even after putting a lot of effort into it. |
Rationale for this change
To simplify loops and unit tests involving numeric types. Special-casing float16 adds complexity.
What changes are included in this PR?
Are these changes tested?
By existing tests that are now extended to run with
float16as input.