Simplify DictionaryBuilder constructors (#2684) (#2054) #2685
Conversation
github-actions
bot
added
arrow
| values_builder: StringBuilder::with_capacity(value_capacity, string_capacity), | ||
| } | ||
| } | ||
|
|
||
| /// Creates a new `StringDictionaryBuilder` from a keys builder and a dictionary |
There was a problem hiding this comment.
| /// Creates a new `StringDictionaryBuilder` from a keys builder and a dictionary | |
| /// Creates a new `StringDictionaryBuilder` from a keys capacity and a dictionary |
| pub fn new() -> Self { | ||
| Self { | ||
| keys_builder, | ||
| values_builder, | ||
| keys_builder: PrimitiveBuilder::new(), | ||
| values_builder: PrimitiveBuilder::new(), |
There was a problem hiding this comment.
Although I think the updated API is more clear, I just wonder if the issue is provided key builder might be not empty, we can check the length of the key builder when it is passed in? The API could be unchanged.
There was a problem hiding this comment.
For sure, however, I'd argue it is a good opportunity to bring this API closer into alignment with the other builders. We don't require passing the various different inner builders to StringArray, for example. The only other place we do is ListArray, which again I think is a little odd.
There was a problem hiding this comment.
I see. Yea, it is somehow weird to provide key and value builders from outside of these dictionary builder.
Co-authored-by: Liang-Chi Hsieh <[email protected]>
|
Benchmark runs are scheduled for baseline = 2ee09bb and contender = 5e2b4c7. 5e2b4c7 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #2684
Part of #2054
Rationale for this change
The previous constructors were confusing, unnecessarily verbose, and allowed creating invalid DictionaryArrays by passing in a non-empty keys builder
What changes are included in this PR?
Are there any user-facing changes?
Yes