This is an automated email from the ASF dual-hosted git repository.
andygrove pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/datafusion-comet.git
The following commit(s) were added to refs/heads/main by this push:
new 2cba02f669 perf: optimize `spark_floor` (up to 4x faster) (#4911)
2cba02f669 is described below
commit 2cba02f6695ad57941dccd43767ac923cbc0ec26
Author: Andy Grove <[email protected]>
AuthorDate: Mon Aug 3 20:17:29 2026 -0600
perf: optimize `spark_floor` (up to 4x faster) (#4911)
* perf: optimize spark_floor in datafusion-comet-spark-expr
* refactor: unify make_decimal_scalar/array signatures on impl Fn
Adopts impl Fn on both helpers so they are symmetric and no callsite needs
to pass &f, addressing coordination feedback with #4926 which uses impl Fn too.
Floor's scalar path drops its stray &f.
* test: cover the wide and large-scale decimal paths in spark_floor
`decimal_floor_wide` and the `Err(_)` arm of `decimal_floor_pow10` had no
test:
`test_floor_decimal128_array` uses unscaled values that all fit in i64, so a
regression confined to the wide path passed CI, and the benchmark that
drives
that path asserts nothing.
- `test_floor_decimal128_wide_array`: unscaled values above i64::MAX,
including
a negative one, since floor rounds toward negative infinity and that arm
was
otherwise untested. Mirrors `test_ceil_decimal128_wide_array` from the
ceil
sibling so the shared path is covered on this PR too.
- `test_floor_decimal128_i64_boundary_array`: straddles i64::MAX and
i64::MIN,
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.
- `test_floor_decimal128_large_scale_array`: 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.
Verified by mutation: with `decimal_floor_wide` returning `div_floor(x,
div) + 1`,
the two new wide-path tests fail and all nine pre-existing tests still pass.
---
native/spark-expr/Cargo.toml | 1 +
native/spark-expr/src/math_funcs/floor.rs | 86 +++++++++++++++++++++++++++++++
2 files changed, 87 insertions(+)
diff --git a/native/spark-expr/Cargo.toml b/native/spark-expr/Cargo.toml
index 5d518387f5..b85153a59d 100644
--- a/native/spark-expr/Cargo.toml
+++ b/native/spark-expr/Cargo.toml
@@ -214,3 +214,4 @@ harness = false
[[bench]]
name = "unscaled_value"
harness = false
+
diff --git a/native/spark-expr/src/math_funcs/floor.rs
b/native/spark-expr/src/math_funcs/floor.rs
index 973bbab708..f97c66e2c8 100644
--- a/native/spark-expr/src/math_funcs/floor.rs
+++ b/native/spark-expr/src/math_funcs/floor.rs
@@ -213,6 +213,92 @@ mod test {
Ok(())
}
+ #[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(())
+ }
+
+ #[test]
+ fn test_floor_decimal128_i64_boundary_array() -> Result<()> {
+ // decimal_floor_pow10 switches from the i64 fast path to
decimal_floor_wide exactly at
+ // i64::MAX / i64::MIN. Straddling both edges pins that the two paths
agree: each pair
+ // below differs by one unscaled unit but floors to the same value, so
a divergence
+ // between the 64-bit and 128-bit divisions would show up as a
mismatch here.
+ let array = Decimal128Array::from(vec![
+ Some(i64::MAX as i128), // 9223372036854.775807 -> fast path
+ Some(i64::MAX as i128 + 1), // 9223372036854.775808 -> wide path
+ Some(i64::MIN as i128), // -9223372036854.775808 -> fast path
+ Some(i64::MIN as i128 - 1), // -9223372036854.775809 -> wide path
+ ])
+ .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(9_223_372_036_854_i128),
+ Some(9_223_372_036_854_i128),
+ Some(-9_223_372_036_855_i128),
+ Some(-9_223_372_036_855_i128),
+ ])
+ .with_precision_and_scale(38, 0)?;
+ let actual =
result.as_any().downcast_ref::<Decimal128Array>().unwrap();
+ assert_eq!(actual, &expected);
+ Ok(())
+ }
+
+ #[test]
+ fn test_floor_decimal128_large_scale_array() -> Result<()> {
+ // dispatch_pow10! only specializes scales whose divisor fits in an
i64, so scale 19 takes
+ // the runtime `decimal_floor_f` fallback. Untested on the array path
otherwise.
+ let array = Decimal128Array::from(vec![
+ Some(20_000_000_000_000_000_000_000_i128), // 2000.0
+ Some(20_000_000_000_000_000_000_001_i128), //
2000.0000000000000000001
+ Some(-20_000_000_000_000_000_000_001_i128), //
-2000.0000000000000000001
+ None,
+ ])
+ .with_precision_and_scale(38, 19)?;
+ 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(2000_i128),
+ Some(2000_i128),
+ Some(-2001_i128),
+ None,
+ ])
+ .with_precision_and_scale(38, 0)?;
+ let actual =
result.as_any().downcast_ref::<Decimal128Array>().unwrap();
+ assert_eq!(actual, &expected);
+ Ok(())
+ }
+
#[test]
fn test_floor_f32_scalar() -> Result<()> {
let args =
vec![ColumnarValue::Scalar(ScalarValue::Float32(Some(125.9345)))];
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
