GH-50695: [C++] Use the full unsigned range for unsigned dictionary index types - #50696
Open
mkzung wants to merge 1 commit into
Open
GH-50695: [C++] Use the full unsigned range for unsigned dictionary index types#50696mkzung wants to merge 1 commit into
mkzung wants to merge 1 commit into
Conversation
…nary index types An unsigned dictionary index previously widened on the signed thresholds because the dictionary builder reuses the signed AdaptiveIntBuilder: a uint8 index widened after 128 distinct values rather than 256. Follow-up to apacheGH-37476 suggested by the review there. Give AdaptiveIntBuilder an opt-in unsigned mode: with use_unsigned_range the appended values are taken as non-negative, width detection and lane writes go through the existing unsigned helpers (DetectUIntWidth, DowncastUInts), and the reported type is unsigned. The dictionary builder passes its use_unsigned_index flag into the indices builder and no longer needs the MaybeUnsignedIndexType mapping, which is removed: the builder now reports the index type natively on every path (type, FinishInternal, FinishDelta, the NullType specialization). The index-type tests assert the new thresholds: 256 distinct values keep a uint8 index, with the indices round-tripping intact through the one-byte lanes, and the 257th widens to uint16; the signed thresholds are unchanged.
|
|
github-actions
Bot
added
Component: C++
Component: Python
awaiting review
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
Follow-up to #50475, suggested in review there. An unsigned dictionary index widened on the signed thresholds because the dictionary builder reuses the signed AdaptiveIntBuilder, so a uint8 index only held 128 distinct values before widening.
What changes are included in this PR?
AdaptiveIntBuilder gets an opt-in
use_unsigned_rangemode: width detection and lane writes go through the existing unsigned helpers (DetectUIntWidth, DowncastUInts) and the reported type is unsigned. The dictionary builder passes its flag into the indices builder, and the MaybeUnsignedIndexType mapping from #50475 is removed since every path now reports the index type natively.Are these changes tested?
Yes. 256 distinct values keep a uint8 index with the indices round-tripping intact, the 257th widens to uint16, and the signed thresholds are unchanged; the FinishDelta path is covered separately, and the python width test asserts the new thresholds.
Are there any user-facing changes?
An unsigned dictionary index now widens on the unsigned thresholds (a uint8 index at 257 distinct values rather than 129).