Refactoring DecimalBuilder constructors #2517
Conversation
psvri
changed the title
| /// Creates a new [`Decimal128Builder`], `capacity` is the number of bytes in the values | ||
| /// array |
There was a problem hiding this comment.
This needs to change the definition of capacity to be the number of decimal elements, instead of the number of decimal bytes. Eliminating this inconsistency is the major motivator for #2054
There was a problem hiding this comment.
It occurs to me I didn't catch this on #2516 ....
There was a problem hiding this comment.
I took the liberty of fixing this
| Box::new(Decimal128Builder::new(capacity, *precision, *scale)) | ||
| } | ||
| DataType::Decimal128(precision, scale) => Box::new( | ||
| Decimal128Builder::with_capacity(capacity, *precision, *scale), |
There was a problem hiding this comment.
It turns out a lot of callsites were assuming this capacity was in terms of elements, when in actuality it was in terms of bytes (which was an extremely confusing API)
|
Benchmark runs are scheduled for baseline = 9921cd5 and contender = 65cae43. 65cae43 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
|
Just curious , how can we see ursa bots benchmarks for the merges. |
|
I don't believe ursabot is currently setup to work with the Rust repo, I think someone got part way through integrating it, so it comments on the PRs but doesn't actually do anything 😅 |
|
Okay |
Which issue does this PR close?
Partially implements #2054
Rationale for this change
Refactor apis to make a consistent new and with_capacity functions
What changes are included in this PR?
Removes capacity from new and added with_capacity function instead.
Are there any user-facing changes?
Yes