andygrove commented on code in PR #4911:
URL: https://github.com/apache/datafusion-comet/pull/4911#discussion_r3692717562
##########
native/spark-expr/src/math_funcs/floor.rs:
##########
@@ -76,11 +81,32 @@ pub fn spark_floor(
}
#[inline]
-fn decimal_floor_f(scale: &i8) -> impl Fn(i128) -> i128 {
- let div = 10_i128.pow_wrapping(*scale as u32);
+fn decimal_floor_f(scale: i8) -> impl Fn(i128) -> i128 {
+ let div = 10_i128.pow_wrapping(scale as u32);
move |x: i128| div_floor(x, div)
}
+/// Floor-divides an unscaled decimal by `10^EXP`.
+///
+/// `EXP` is a compile-time constant so that the divisor is folded in and the
division lowered to a
+/// multiply-and-shift. A 128-bit division is always a libcall, even by a
constant, so values that
+/// fit in 64 bits take a 64-bit path; unscaled decimals rarely exceed that
range.
+#[inline]
+fn decimal_floor_pow10<const EXP: u32>(x: i128) -> i128 {
+ match i64::try_from(x) {
+ Ok(x) => div_floor(x, const { 10_i64.pow(EXP) }) as i128,
+ Err(_) => decimal_floor_wide(x, const { 10_i128.pow(EXP) }),
+ }
+}
+
+/// Kept out of line so that the libcall and its stack frame stay out of the
loop body of every
+/// [`decimal_floor_pow10`] instantiation.
+#[cold]
+#[inline(never)]
+fn decimal_floor_wide(x: i128, div: i128) -> i128 {
Review Comment:
Added in 826b2b7f0, using your test as written — the expected values check
out, including the negative row flooring to `-20_000_000_000_001` rather than
toward zero.
I confirmed the gap you described is real rather than assuming it, by
mutating `decimal_floor_wide` to return `div_floor(x, div) + 1`: all nine
pre-existing tests still passed, and only the new wide-path tests failed. So
the wide path was genuinely unguarded.
While in there I covered two adjacent paths that were also untested:
- `test_floor_decimal128_i64_boundary_array` straddles `i64::MAX` and
`i64::MIN`, the exact point where `decimal_floor_pow10` switches between the
64-bit and 128-bit divisions. Each pair differs by one unscaled unit and must
floor to the same value, so the two paths are asserted to agree at the seam in
both signs, not just tested independently.
- `test_floor_decimal128_large_scale_array` uses scale 19, where
`dispatch_pow10!` stops specializing and falls back to the runtime
`decimal_floor_f`. That fallback was previously only reached through the scalar
path, so the array fallback branch had no coverage either.
All 11 floor tests pass and `cargo clippy --all-targets` is clean.
--
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]
