Repository navigation
GH-55503: [C++] Fix Float16 memo table keys built from raw bits - #55610
AnuragRaut08 wants to merge 1 commit into
Conversation
|
|
|
We don't need AI only contribution. |
|
@Reranko05 I understand the concern about AI-generated contributions. I did use AI assistance during development, but I want to clarify that I personally worked through the reproduction, ran the regression tests against the original implementation, validated the fix, and checked the affected code paths. I'm happy to explain the implementation and the reasoning behind each change. If there are specific concerns about the contribution or its implementation, I'd appreciate the opportunity to address them. I respect the project's contribution guidelines and will make sure my future contributions demonstrate that ownership more clearly. |
|
Thanks for clarifying. The PR was closed because the AI declaration in the description indicated that the code was written and reviewed only by AI, with no human review selected. As per Apache Arrow's contribution policy, AI-assisted contributions require human review before submission. https://arrow.apache.org/docs/developers/overview.html#ai-generated-code You're welcome to open a new PR with the changes properly reviewed by a human and the AI declaration updated accordingly. |
Rationale for this change
Float16 dictionary unification and
count_distinctproduce incorrect results because rawuint16_tstorage values are interpreted as numeric values instead of Float16 bit patterns.For example, the Float16 bit pattern
0x3C00, representing1.0, is interpreted as15360.0when passed through the converting constructor.This causes dictionary values to be corrupted during dictionary unification and distinct Float16 values to be incorrectly merged by
count_distinct.What changes are included in this PR?
Float16::FromBits()when constructing memo table keys from raw Float16 values inDictionaryUnifierImpl::Unify.CountDistinctImpl::Consume.UnboxScalar<HalfFloatType>::Unbox.Are these changes tested?
Added regression tests:
TestDictionaryUnifier.HalfFloat: verifies unified dictionary values and transpose maps.TestCountDistinctKernel.HalfFloat: covers distinct Float16 values, duplicates, nulls, chunked arrays, and scalar input.Verified that both new regression tests fail with the original implementation and pass with the fix.
Validation performed:
arrow-array-test: 346 tests passed (2 disabled).arrow-compute-aggregate-test: 346 tests passed.clang-format --dry-run --Werror: passed.git diff --check: passed.Are there any user-facing changes?
Fixes incorrect Float16 values returned by dictionary unification and incorrect distinct counts for Float16 arrays.
Critical Fix
This PR fixes a bug that produces incorrect data. Float16 dictionary values could be corrupted during unification, and
count_distinctcould undercount distinct Float16 values.Was AI used for this PR?
PR code and description written by:
Reviewed before submission by:
combine_chunks,concat_arrays,unify_dictionaries, Feather) #55503