mbutrovich commented on code in PR #4911:
URL: https://github.com/apache/datafusion-comet/pull/4911#discussion_r3632006307
##########
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:
`decimal_floor_wide` and the `Err(_)` arm of `decimal_floor_pow10` are never
reached by a test. `test_floor_decimal128_array` uses unscaled values that all
fit in i64, so a regression that broke only the wide path or the
negative-remainder rounding would pass CI. The benchmark exercises the cold
path but asserts nothing, so it does not guard correctness.
The ceil sibling (#4926) closed this exact gap with
`test_ceil_decimal128_wide_array`. Please add the floor analogue so the shared
path is covered on the base PR:
```rust
#[test]
fn test_floor_decimal128_wide_array() -> Result<()> {
// Unscaled values above i64::MAX (~9.22e18) take the wide fallback in
decimal_floor_pow10.
// Scale 6 targeting Decimal128(38, 0):
// 20_000_000_000_000_000_000 / 10^6 = 20_000_000_000_000 (exact
multiple)
// 20_000_000_000_000_000_001 / 10^6 -> floor 20_000_000_000_000
(toward zero)
// -20_000_000_000_000_000_001 / 10^6 -> floor -20_000_000_000_001
(toward -inf)
let array = Decimal128Array::from(vec![
Some(20_000_000_000_000_000_000_i128),
Some(20_000_000_000_000_000_001_i128),
Some(-20_000_000_000_000_000_001_i128),
None,
])
.with_precision_and_scale(38, 6)?;
let args = vec![ColumnarValue::Array(Arc::new(array))];
let ColumnarValue::Array(result) = spark_floor(&args,
&DataType::Decimal128(38, 0))? else {
unreachable!()
};
let expected = Decimal128Array::from(vec![
Some(20_000_000_000_000_i128),
Some(20_000_000_000_000_i128),
Some(-20_000_000_000_001_i128),
None,
])
.with_precision_and_scale(38, 0)?;
let actual = result.as_any().downcast_ref::<Decimal128Array>().unwrap();
assert_eq!(actual, &expected);
Ok(())
}
```
The negative row is the one worth pinning, since floor rounds toward
negative infinity and that arm is otherwise untested.
--
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]
