GH-50512: [C++][Compute] Support float16 in hash kernels (dictionary_encode, unique, value_counts)#50513
Open
fornwall wants to merge 1 commit into
Open
GH-50512: [C++][Compute] Support float16 in hash kernels (dictionary_encode, unique, value_counts)#50513fornwall wants to merge 1 commit into
fornwall wants to merge 1 commit into
Conversation
…nts) dictionary_encode(), unique() and value_counts() failed on half_float input with: ArrowNotImplementedError: Function 'dictionary_encode' has no kernel matching input types (halffloat) AddHashKernels() registers kernels by iterating PrimitiveTypes(), which is derived from FloatingPointTypes() and only contains float32 and float64 -- float16 is absent, so no half_float kernel was ever registered. GetHashInit() also had no HALF_FLOAT case. Rather than widening the public FloatingPointTypes()/NumericTypes()/ PrimitiveTypes() lists (which many other kernel registrations consume, e.g. aggregate_basic.cc, and which would change behaviour well beyond this bug), register the float16 kernel explicitly in AddHashKernels() and dispatch HALF_FLOAT in GetHashInit(). HALF_FLOAT is hashed via RegularHashKernel<UInt16Type>, i.e. by its raw bit pattern. This mirrors the existing treatment of float32 (hashed via UInt32Type) and float64 (via UInt64Type), so NaN/-0.0 semantics are consistent with what those types already do. HalfFloatType::c_type is already uint16_t, and the hash kernel keeps the original type for its output, so the resulting dictionary is correctly typed float16. Signed-off-by: Fredrik Fornwall <fredrik@fornwall.net>
fornwall
force-pushed
the
fix-float16-hash-kernels
branch
from
cfc957b to
b61813c
Compare
fornwall
changed the title
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
Asd missing support for
float16in hash kernels.What changes are included in this PR?
Support
float16in hash kernels:dictionary_encode,unique,value_counts.Are these changes tested?
Added
UniqueHalfFloat,ValueCountsHalfFloatandDictEncodeHalfFloattovector_hash_test.cc, following the conventions in that file. Coverage includes nulls, repeated values, a no-nulls case, and sliced input.float16is not inPrimitiveTypes(), so it isn't picked up by the existingTestHashKernelPrimitivetyped suite and needs its ownTEST_Fcases.NaN/-0.0cases are deliberately not tested, matching the existing float32/float64 tests.Built and ran locally:
*HashKernel*tests pass.arrow-compute-vector-testbinary passes: 1135 tests from 150 test suites, no regressions.Are there any user-facing changes?
No (except for filling out the feature gap).