andygrove commented on code in PR #4926:
URL: https://github.com/apache/datafusion-comet/pull/4926#discussion_r3603835592
##########
native/spark-expr/src/math_funcs/ceil.rs:
##########
@@ -78,7 +78,20 @@ pub fn spark_ceil(
#[inline]
fn decimal_ceil_f(scale: &i8) -> impl Fn(i128) -> i128 {
let div = 10_i128.pow_wrapping(*scale as u32);
- move |x: i128| div_ceil(x, div)
+ // 128-bit division is a compiler intrinsic call, while 64-bit division
maps to a single
+ // hardware instruction. Decimals with precision <= 18 always fit in 64
bits, so take that
+ // path whenever both the value and the divisor do, and fall back to
128-bit otherwise.
+ let div_i64 = i64::try_from(div).ok();
+ move |x: i128| match (div_i64, i64::try_from(x)) {
+ (Some(d), Ok(x)) => {
+ let quotient = x / d;
+ // `d` is positive, so a positive remainder means the truncated
quotient is below
+ // the true value and has to be rounded up.
+ let adjusted = if x % d > 0 { quotient + 1 } else { quotient };
+ adjusted as i128
+ }
+ _ => div_ceil(x, div),
+ }
}
Review Comment:
Added `test_ceil_decimal128_wide_array` with `Decimal128(38, 6)` unscaled
values above `i64::MAX` (positive, negative, exact multiple), so the wide
fallback arm and the negative-remainder branch are covered.
##########
native/spark-expr/src/math_funcs/utils.rs:
##########
@@ -57,7 +57,7 @@ pub(crate) fn make_decimal_array(
array: &ArrayRef,
precision: u8,
scale: i8,
- f: &dyn Fn(i128) -> i128,
+ f: impl Fn(i128) -> i128,
Review Comment:
Adopted the `dispatch_pow10` mechanism shared with `spark_floor` (#4911).
The per-row runtime `i64::try_from` branch is gone; the fast path is a
compile-time `decimal_ceil_pow10::<EXP>` monomorphization and the wide case is
an out-of-line `#[cold]` fallback. Callsite passes `f` by value.
##########
native/spark-expr/src/math_funcs/utils.rs:
##########
Review Comment:
Unified `make_decimal_scalar` on `impl Fn` in the base #4911 commit so both
helpers are symmetric; ceil now passes `f` by value everywhere.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
