GH-41017: [C++] Preserve ordered flag in DictionaryBuilder#49797
Open
tinezivic wants to merge 1 commit intoapache:mainfrom
Open
GH-41017: [C++] Preserve ordered flag in DictionaryBuilder#49797tinezivic wants to merge 1 commit intoapache:mainfrom
tinezivic wants to merge 1 commit intoapache:mainfrom
Conversation
|
|
DictionaryBuilderBase::type() did not pass the ordered parameter to ::arrow::dictionary(), causing it to always default to false. This meant that building a DictionaryArray via MakeBuilder or MakeDictionaryBuilder with an ordered DictionaryType would produce an array with ordered=false. Fix: Add ordered_ member and set_ordered() to DictionaryBuilderBase (both the primary template and the NullType specialization). The DictionaryBuilderCase in builder.cc now propagates the ordered flag from the input DictionaryType to the builder after construction. Generated-by: GitHub Copilot
tinezivic
force-pushed
the
fix-dictionary-builder-ordered
branch
from
7960ab8 to
d3323b0
Compare
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
DictionaryBuilderBase::type()does not pass theorderedparameter to::arrow::dictionary(), causing it to always default tofalse. This means that anyDictionaryArraybuilt throughMakeBuilder()orMakeDictionaryBuilder()with an orderedDictionaryTypewill produce an array wheretype().ordered() == false.This affects PyArrow users:
pa.array(data, type=pa.dictionary(pa.int8(), pa.string(), ordered=True))returns an array withordered=False.Reported in: #41017
Also caused: pandas-dev/pandas#58152
What changes are included in this PR?
bool ordered_ = falsemember andset_ordered()method toDictionaryBuilderBase(both the primary template and theNullTypespecialization)ordered_to::arrow::dictionary()intype()andFinishInternal()bool orderedfield toDictionaryBuilderCaseinbuilder.ccdict_type.ordered()throughMakeBuilderImpl::Visit()andMakeDictionaryBuilder()Are these changes tested?
Yes. Added 3 C++ tests in
array_dict_test.cc:MakeBuilderPreservesOrdered— verifiesMakeBuilderwith ordered dict type produces ordered arrayMakeBuilderUnorderedByDefault— verifies unordered stays unorderedMakeDictionaryBuilderPreservesOrdered— verifiesMakeDictionaryBuilderpreserves orderedAre there any user-facing changes?
No API changes.
DictionaryBuildernow correctly preserves theorderedflag from the inputDictionaryType, fixing the silent data loss.