Constrain List/ListView builder offset, size types to 32 and 64 bit types - #9053
Open
robert3005 wants to merge 4 commits into
Open
Constrain List/ListView builder offset, size types to 32 and 64 bit types#9053robert3005 wants to merge 4 commits into
robert3005 wants to merge 4 commits into
Conversation
Remove `append_list_array` and `append_listview_array` from the `ArrayBuilder` trait; they only ever applied to list-typed builders. The implementations now live as inherent methods on `ListBuilder<O>` and `ListViewBuilder<O, S>`, and the List/ListView vtables dispatch through a new `match_each_list_builder!` macro that trial-downcasts the `dyn ArrayBuilder` against every concrete list builder instantiation over the integer offset/size types. Non-list builders fail with the same error as the old default trait implementations. Signed-off-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NaXX3wiHuyhFvisiZV7SCn
Add a sealed `ListOffsetPType` trait (implemented for exactly u32, i32, u64, and i64) and use it as the bound on `ListBuilder<O>` and `ListViewBuilder<O, S>`, so builders with other integer widths can no longer be constructed. This makes the `match_each_list_builder!` matcher provably exhaustive and shrinks it from 72 downcast attempts to 20. Incoming arrays are unaffected: their offsets/sizes still dispatch over all integer ptypes when appended into a builder. The sparse encoding now picks its builder offset type via a new `match_smallest_list_offset_type!` (u32/u64), and arbitrary list generation and tests drop the no-longer-constructible narrow widths. Signed-off-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NaXX3wiHuyhFvisiZV7SCn
Merging this PR will improve performance by 13.06%
Performance Changes
Tip Curious why this is faster? Comment Comparing 
Footnotes
|
Signed-off-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NaXX3wiHuyhFvisiZV7SCn
Sparse list canonicalization now picks its builder offset type via match_smallest_list_offset_type!, whose smallest choice is u32, so the canonical offsets for a source outgrowing u8 are u32 rather than u16. Signed-off-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NaXX3wiHuyhFvisiZV7SCn
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
2 participants
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.
In #8609 we have inverted the flow of the append_to_builder methods. The side effect of that is we match on the builders but for lists the number of match cases is excessive due to trait bounds being arbitrary integer types. Realistically types smaller than u32 are not that useful as they define the upper bound on the size of the array. This pr introduces
OffsetBuilderPTypewhich constraints the offset and sizes buffer types in ListBuilder.We will use that on VarBinBuilder in a follow up