Refactor Binary Builder and String Builder Constructors #2592
Conversation
github-actions
bot
added
arrow
|
I have replace all |
| null_buffer_builder: NullBufferBuilder::new(1024), | ||
| } | ||
| pub fn new() -> Self { | ||
| Self::with_capacity(1024, 1024) |
There was a problem hiding this comment.
Is it a reasonable assumption that the array contains the same number of elements and bytes?
There was a problem hiding this comment.
When we used new previously , it was using 1024 as no. of items, hence I have retained the same.
There was a problem hiding this comment.
It's not a reasonable assumption no, but it is a decent guess to avoid worst case bump allocation, probably 😅
| @@ -2355,7 +2355,7 @@ where | |||
| let values = cast_values.as_any().downcast_ref::<StringArray>().unwrap(); | |||
|
|
|||
| let keys_builder = PrimitiveBuilder::<K>::with_capacity(values.len()); | |||
| let values_builder = StringBuilder::new(values.len()); | |||
| let values_builder = StringBuilder::with_capacity(1024, values.len()); | |||
There was a problem hiding this comment.
In this case I'd be tempted to just go with new, this estimate isn't obviously better
There was a problem hiding this comment.
Yeah, I should have retained new.
|
Benchmark runs are scheduled for baseline = ff81dee and contender = 744412f. 744412f 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 #2054.
Rationale for this change
Removes capacity from new constructors
What changes are included in this PR?
Removes capacity from new constructors
Are there any user-facing changes?
Yes