mbutrovich commented on code in PR #11427:
URL: https://github.com/apache/arrow-rs/pull/11427#discussion_r4233106832
##########
arrow-arith/benches/decimal_arithmetic.rs:
##########
@@ -62,5 +66,109 @@ fn decimal_arithmetic(c: &mut Criterion) {
benchmark::<Decimal256Type>(c, "decimal256");
}
-criterion_group!(benches, decimal_arithmetic);
+type Kernel = fn(&dyn Datum, &dyn Datum) -> Result<ArrayRef, ArrowError>;
+
+/// Creates an array of `len` random values with `digits` significant digits
+/// (no leading zero, half of them negative) for the given `scale`
+fn random_decimal<T: DecimalType>(
+ len: usize,
+ digits: u32,
+ scale: i8,
+ seed: u64,
+) -> PrimitiveArray<T> {
+ let mut rng = StdRng::seed_from_u64(seed);
+ let ten = T::Native::usize_as(10);
+ let values = (0..len).map(|_| {
+ let mut value = T::Native::usize_as(rng.random_range(1..10));
+ for _ in 1..digits {
+ value = value
+ .mul_wrapping(ten)
+ .add_wrapping(T::Native::usize_as(rng.random_range(0..10)));
+ }
+ if rng.random::<bool>() {
+ value.neg_wrapping()
+ } else {
+ value
+ }
+ });
+ decimal::<T>(values, scale)
+}
+
+/// Benchmarks dividing values with scale 10 by a scalar with scale 2, for each
+/// pair of value and divisor digit counts in `cases`
+fn benchmark_scalar<T: DecimalType>(c: &mut Criterion, name: &str, cases:
[(u32, u32); 4]) {
+ let mut group = c.benchmark_group(format!("{name}_scalar"));
+ group.throughput(Throughput::Elements(SIZE as u64));
+ for (digits, divisor_digits) in cases {
+ let array = random_decimal::<T>(SIZE, digits, 10, 42);
+ let divisor = Scalar::new(random_decimal::<T>(1, divisor_digits, 2,
42));
+ for (op, kernel) in [("div", div as Kernel), ("rem", rem)] {
Review Comment:
> Could `rem` get its own cases, so each one exercises what its name says?
`rem` now has its own cases. Its divisor has the same scale as the values,
so `rem` doesn't rescale it, and the digit counts in each name are the digit
counts of the operands `rem` divides. The `Decimal128` cases are 10 by 4, 30 by
4 and 30 by 21 digits, and the `Decimal256` cases are 30 by 4, 60 by 4 and 60
by 40 digits. `rem` has no overflow case. With the benchmark's seed, no
remainder equals its dividend, and the 21-digit and 40-digit divisors are 68
and 131 bits wide.
##########
arrow-arith/benches/decimal_arithmetic.rs:
##########
@@ -62,5 +66,109 @@ fn decimal_arithmetic(c: &mut Criterion) {
benchmark::<Decimal256Type>(c, "decimal256");
}
-criterion_group!(benches, decimal_arithmetic);
+type Kernel = fn(&dyn Datum, &dyn Datum) -> Result<ArrayRef, ArrowError>;
+
+/// Creates an array of `len` random values with `digits` significant digits
+/// (no leading zero, half of them negative) for the given `scale`
+fn random_decimal<T: DecimalType>(
+ len: usize,
+ digits: u32,
+ scale: i8,
+ seed: u64,
+) -> PrimitiveArray<T> {
+ let mut rng = StdRng::seed_from_u64(seed);
+ let ten = T::Native::usize_as(10);
+ let values = (0..len).map(|_| {
+ let mut value = T::Native::usize_as(rng.random_range(1..10));
+ for _ in 1..digits {
+ value = value
+ .mul_wrapping(ten)
+ .add_wrapping(T::Native::usize_as(rng.random_range(0..10)));
+ }
+ if rng.random::<bool>() {
+ value.neg_wrapping()
+ } else {
+ value
+ }
+ });
+ decimal::<T>(values, scale)
+}
+
+/// Benchmarks dividing values with scale 10 by a scalar with scale 2, for each
+/// pair of value and divisor digit counts in `cases`
+fn benchmark_scalar<T: DecimalType>(c: &mut Criterion, name: &str, cases:
[(u32, u32); 4]) {
+ let mut group = c.benchmark_group(format!("{name}_scalar"));
+ group.throughput(Throughput::Elements(SIZE as u64));
+ for (digits, divisor_digits) in cases {
+ let array = random_decimal::<T>(SIZE, digits, 10, 42);
+ let divisor = Scalar::new(random_decimal::<T>(1, divisor_digits, 2,
42));
+ for (op, kernel) in [("div", div as Kernel), ("rem", rem)] {
+ group.bench_function(
+
format!("{op}_{digits}_digits_by_{divisor_digits}_digits/no_nulls"),
+ |b| b.iter(|| hint::black_box(kernel(&array,
&divisor).unwrap())),
+ );
+ }
+ }
+
+ // `try_unary` visits only the valid values when the array has nulls
+ let (digits, divisor_digits) = cases[0];
+ let array = random_decimal::<T>(SIZE, digits, 10, 42);
+ let nulls = NullBuffer::from_iter((0..SIZE).map(|i| i % 5 != 0));
+ let with_nulls = PrimitiveArray::<T>::new(array.values().clone(),
Some(nulls))
+ .with_data_type(array.data_type().clone());
+ let divisor = Scalar::new(random_decimal::<T>(1, divisor_digits, 2, 42));
+ for (op, kernel) in [("div", div as Kernel), ("rem", rem)] {
+ group.bench_function(
+
format!("{op}_{digits}_digits_by_{divisor_digits}_digits/mixed_nulls"),
+ |b| b.iter(|| hint::black_box(kernel(&with_nulls,
&divisor).unwrap())),
+ );
+ }
+
+ // Small arrays, where the cost of setting up a division can outweigh the
per-value work
+ let (digits, divisor_digits) = cases[1];
+ let divisor = Scalar::new(random_decimal::<T>(1, divisor_digits, 2, 42));
+ for len in [1, 16, 128] {
+ let array = random_decimal::<T>(len, digits, 10, 42);
+ group.throughput(Throughput::Elements(len as u64));
+ group.bench_function(
+
format!("div_{digits}_digits_by_{divisor_digits}_digits/{len}_values"),
+ |b| b.iter(|| hint::black_box(div(&array, &divisor).unwrap())),
+ );
+ }
+ group.finish();
+}
+
+/// Decimal32 and Decimal64 divide with the same instructions as Int32 and
Int64,
+/// which the `integer_division` benchmark covers
Review Comment:
> I don't think this holds.
You're right. `Decimal32` and `Decimal64` now have their own cases, with
values and divisors at scale 2, so `div` scales the values by 10^6 as it does
for the wider types. `benchmark_scalar` takes the scale as a parameter now.
`Decimal32` has a case that fits after scaling (3 by 4 digits) and one where
every value overflows (6 by 4 digits). `Decimal64` has the same two (3 by 4 and
15 by 4 digits) plus values wider than 32 bits after scaling (12 by 4 digits).
The overflow cases use values whose quotient still fits in the type, because
otherwise `scaled_div` returns an error as well. Each type also has a `rem`
case and the same nulls and small-array cases as the wider types. I removed the
claim from the comment and from the PR description.
--
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]
