mbutrovich commented on code in PR #4926:
URL: https://github.com/apache/datafusion-comet/pull/4926#discussion_r3591143087
##########
native/spark-expr/src/math_funcs/utils.rs:
##########
Review Comment:
`utils.rs:49` still declares `f: &dyn Fn(i128) -> i128` and `ceil.rs:68`
passes `&f`. The PR's stated rationale is "the per-element rounding closure is
monomorphized rather than called through a `&dyn Fn` vtable," but that only
happened for the array path. The scalar path is a single element so the perf
cost is irrelevant, however the two helpers now have inconsistent signatures
for no reason, which is a maintenance trap.
Suggested change: change `make_decimal_scalar` at `utils.rs:49` to `f: impl
Fn(i128) -> i128` and pass `f` by value at `ceil.rs:68`, matching
`make_decimal_array`. This keeps the two helpers symmetric.
##########
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:
`ceil.rs:97-254` tests only small unscaled values (max `12999`), which all
take the new i64 fast path. The entire point of this PR is the branch at
`ceil.rs:85`, and the fallback arm (`_ => div_ceil(x, div)` at `ceil.rs:93`)
has zero direct coverage. A regression that broke only the wide path would pass
CI.
Suggested change: add an array test with a `Decimal128(38, s)` input whose
unscaled values exceed `i64::MAX` (positive and negative), asserting the ceil
result. Example: unscaled `20_000_000_000_000_000_000` (> i64::MAX) at scale 6
targeting `Decimal128(38, 0)`, plus a negative sibling and an exact multiple,
so both the fast and fallback arms plus the negative-remainder branch are
covered in one suite.
##########
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:
`utils.rs:60` now takes `f: impl Fn(i128) -> i128`, but the array call site
at `ceil.rs:51` still passes `&f`. This compiles because `&F: Fn` via the
blanket impl, so `F` monomorphizes to `&closure` and dispatch is static, but it
is now a stray reference that obscures the intent of the change and adds a
pointer indirection the PR was trying to remove.
Suggested change: pass the closure by value at `ceil.rs:51`:
`make_decimal_array(array, precision, scale, f)`.
--
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]
