arrow icon indicating copy to clipboard operation
arrow copied to clipboard

GH-43017: [C++] Make the set of casts and hash kernels involving float16 consistent with other floating types

Open felipecrv opened this issue 1 year ago • 4 comments

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?

  • Complete the set of casts involving half-float casts (with the exception of half-float -> decimal128/256 casts)
  • Make half-floats supported in hash kernels (kernels defined in terms of the equality of physical representation of values)

Are these changes tested?

By existing tests that are now extended to run with float16 as input.

  • GitHub Issue: #43017

felipecrv avatar Jun 24 '24 03:06 felipecrv

:warning: GitHub issue #43017 has been automatically assigned in GitHub to PR creator.

github-actions[bot] avatar Jun 24 '24 03:06 github-actions[bot]

@ianmcook @ClifHouck

felipecrv avatar Jun 24 '24 03:06 felipecrv

@pitrou

felipecrv avatar Jun 26 '24 20:06 felipecrv

@felipecrv Do we have tests for sorting float16 values and NaNs already? Otherwise, we should add some.

Also, is only array_sort handled, or does the more general sort also accept float16?

pitrou avatar Jun 27 '24 17:06 pitrou

@felipecrv Is this ready for review again?

pitrou avatar Jul 22 '24 11:07 pitrou

@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.

felipecrv avatar Jul 23 '24 02:07 felipecrv