GH-37476: [C++][Python] Preserve unsigned dictionary index types when building from values#50475
Open
mkzung wants to merge 1 commit into
Open
GH-37476: [C++][Python] Preserve unsigned dictionary index types when building from values#50475mkzung wants to merge 1 commit into
mkzung wants to merge 1 commit into
Conversation
…s when building from values
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
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
An unsigned dictionary index type is silently replaced by the signed one of the same width:
It also makes
pa.chunked_array(values, dict_type)fail withArrowTypeError: Array chunks must all be same type, because the chunk it builds internally comes back with a different type than the one requested.The cause is in
DictionaryBuilderCase::CreateFor()inbuilder.cc. The requested index type is reduced toindex_type->byte_width()and handed toAdaptiveIntBuilderas a starting width, so everything except the width is dropped, and the dictionary type is then rebuilt from whatever the indices builder reports. That builder only ever reports signed types.@jorisvandenbossche suggested using
AdaptiveUIntBuilderfor unsigned index types. I tried that first and it does not work: it makesMakeDictionaryBuilderreturn a different builder class, andutil/converter.h,json/from_string.ccand the R binding all cast the result toDictionaryBuilder<T>.TestDictionaryUnifier.ChunkedArrayNestedDictdies on achecked_pointer_castDCHECK as soon as you try it.So this keeps one builder class and preserves the requested signedness where the index type is reported. Indices are non-negative and the signed and unsigned types of a given width have the same layout, so reporting one as the other is value-preserving and free. The width stays adaptive, as it is for signed index types.
What changes are included in this PR?
DictionaryBuilderBaseand itsNullTypespecialization record whether an unsigned index type was requested and map the indices builder's signed type to the unsigned one of the same width intype(),FinishInternal()andFinishDelta().CreateFor()passes the signedness through instead of discarding it.util/converter.h,python_to_arrow.ccandr_to_arrow.cppare not touched, so R is fixed rather than broken.Are these changes tested?
Ten tests in
array_dict_test.ccand four parametrized ones intest_array.py, covering all four unsigned index types, a signed regression guard, nulls,FinishDelta, the supplied-dictionary constructor, fixed-size-binary values,orderedtogether with an unsigned index, and the exact-index builder. Eight of the ten C++ tests fail without the change.The width still adapts: a requested
uint8grows touint16and thenuint32as the dictionary grows, and stays unsigned at each step.arrow-array-testpasses (1056), as doarrow-compute-scalar-cast-testandarrow-c-bridge-test, and the pyarrow suites are unchanged from main.One thing I noticed while in here: the supplied-dictionary constructor starts the indices builder at its default width rather than the requested one, so the requested width is dropped on that path. That is not new, a requested
int32reportsint8there on main too. This preserves the signedness there and leaves the width alone.Are there any user-facing changes?
pa.arrayandpa.chunked_arraynow return the unsigned index type that was requested.One consequence is worth flagging.
dictionary(uint64(), ...)now genuinely produces uint64 indices, andarrow_to_pandas.ccdeliberately refuses to convert those ("Converting UInt64 dictionary indices to pandas is not supported", since pandas categorical codes are signed). Soto_pandas()on such an array raises now, where before it worked by accident on int32 indices. uint8, uint16 and uint32 are unaffected. I think that is the bug surfacing rather than a new one, but adding uint64 support to the pandas conversion may be worth a follow-up, and there is a test pinning the current behaviour.I checked the rest of the surface for every unsigned width:
take,filter,cast,dictionary_decode, IPC, a Parquet write and read back, the compute kernels and an Acerogroup_byall work and keep the index type. Pandas conversion of uint64 is the only thing that changes.AI usage
I used an AI coding assistant while working on this, to help track down the root cause and to draft tests. I built Arrow and pyarrow from source, ran the C++ and Python suites and the lint hooks myself, and I understand and take responsibility for every line.