Rich-T-kid commented on PR #10911:
URL: https://github.com/apache/arrow-rs/pull/10911#issuecomment-5495840862
> That bench group won't say much about this branch.
`arrow-arith/benches/decimal_arithmetic.rs:48-51` only registers `add` and
`sub`, so all 16 rows are on paths this PR doesn't touch, and the 1.09 on
`decimal128_equal_scale` is inside its own ±335ns spread.
>
> Nothing in-tree covers decimal `div`. I can add a criterion group for it
if you want the number before this goes in.
@bharadwaj-pendyala I don't think we need a regression benchmark here.
There's no real baseline to compare the overflow branch against.
On main:
```rust
l.mul_checked(l_mul)?.div_checked(r.mul_checked(r_mul)?)
```
If l.mul_checked(l_mul) overflows, the ? just returns an error right away.
Nothing else runs.
This PR:
```rust
match l.mul_checked(l_mul) {
Ok(scaled) => scaled.div_checked(r.mul_checked(r_mul)?),
Err(_) => scaled_div::<T>(l, r, mul_pow),
}
```
The success path is exactly the same as main, so no perf difference there.
The overflow path is brand new behavior, we're not comparing old cost to new
cost of the same operation, we're comparing "return an error" to "return a
correct value via scaled_div.".
in the future if someone thinks of a way to optimize the algorithm they
could introduce benchmarks to compare against that
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
