refactor: deduplicate spark_ceil and spark_floor by FreakingMind · Pull Request #7534 · apache/datafusion-comet

FreakingMind · 2026-11-06T12:30:58Z

Which issue does this PR close?

Closes #6974.

Rationale for this change

ceil.rs and floor.rs were the same code with ceil and floor swapped. The tests had already drifted: floor tested the switch from i64 to i128 and scale 19, ceil didn't.

What changes are included in this PR?

Adds floor's two decimal tests to ceil. The expected values are different because ceil rounds the other way: 9223372036854.775807 becomes 9223372036855 instead of 9223372036854.
Replaces ceil.rs and floor.rs with one module, math_funcs/ceil_floor.rs, which exports spark_ceil and spark_floor. The direction is a const CEIL: bool parameter, so each direction is still monomorphized into the loop. dispatch_pow10! moved there too, since only ceil and floor use it. utils.rs keeps only the decimal helpers shared with div, round and make_decimal.
Merges the two test modules into one, where each test checks both functions. The input values are the same, and 22 tests become 11. The decimal tests now pass the result precision Spark plans, (33, 0) for a (38, 6) input and (20, 0) for (38, 19).
Adds a bigint column and DECIMAL(38, 6) and DECIMAL(38, 19) columns to ceil.sql and floor.sql, with values on both sides of i64::MAX and i64::MIN, close to the maximum precision, and at scale 19, so these paths are now compared with Spark.

This removes the #[macro_export] macro downcast_compute_op. Nothing in the repo uses it after this change, but it was exported from the published datafusion-comet-spark-expr crate, so anyone calling it from outside will need to inline it.

How are these changes tested?

To check that the tests catch real bugs, I temporarily swapped the rounding direction, broke the i64 fast path and the wide path one at a time, and made the scale 19 fallback ignore the direction. The Rust tests failed each time, and ceil.sql also failed when the wide path rounded the wrong way.

Benchmarks compared to the code before the refactor:

Bench	Change
ceil_decimal_18_4	-0.1%
ceil_decimal_38_6	-0.4%
spark_floor decimal128(38,2)	+2.0%
spark_floor decimal128(38,18)	-5.2%
spark_floor decimal128(38,6) wide values	-2.2%

Switching from the marker types to the const bool made no measurable difference: running the two builds alternately, the decimal benches were within 2% of each other, which is about the run-to-run noise on my machine.

sam-1112

LGTM! Could we add run-spark-4.1-tests before queueing? Spark's SQL suites were skipped in the current fork run.

Part of apache#6974.

FreakingMind · 2026-11-09T17:46:34Z

Thanks for the review! Done, all tests passed

comphead

I compared the generic kernel with the old ceil.rs and floor.rs path by path and found no behavior change, and all 22 original test cases are still there. The PR also removes the #[macro_export] macro downcast_compute_op from the crate root. Nothing in native/ uses it any more, but the crate is published, so that is a public API removal worth a line in the description.

comphead · 2026-11-11T01:52:58Z

    (*precision, *scale)
 }
+
+/// The rounding direction that distinguishes `ceil` from `floor`.


utils.rs holds the decimal helpers that div.rs, round.rs, make_decimal.rs and wide_decimal_binary_expr.rs share. The trait, the markers, the kernel and its tests (about 480 lines) are used only by ceil and floor, and so is dispatch_pow10!. Since ceil.rs and floor.rs are now thin wrappers, could the shared code live in one ceil_floor.rs module that exports spark_ceil and spark_floor, and keep utils.rs for the cross-function helpers?

Thanks, fixed, dispatch_pow10! and the tests are in ceil_floor.rs now, and utils.rs only differs from main by the removed macro

comphead · 2026-11-11T01:52:59Z

+pub(crate) trait RoundingMode {
+    const NAME: &'static str;
+
+    fn round_f32(x: f32) -> f32;
+    fn round_f64(x: f64) -> f64;
+    fn div_i64(x: i64, div: i64) -> i64;
+    fn div_i128(x: i128, div: i128) -> i128;


div_ceil and div_floor from num::integer are already generic over Integer, and f32/f64 both have ceil/floor through num::Float. So div_i64/div_i128 and round_f32/round_f64 could each be one generic method, which halves the trait and both impls. A const CEIL: bool parameter would also fold away at compile time and remove the marker types. I have not compiled either variant.

Thanks, done with const CEIL: bool, I ran the old and new builds alternately and the benches are almost the same

comphead · 2026-11-11T01:52:59Z

+    #[test]
+    fn test_decimal128_i64_boundary_array() -> Result<()> {
+        // decimal_round_pow10 switches from the i64 fast path to decimal_round_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 rounds to the same value, so a divergence
+        // between the 64-bit and 128-bit divisions would show up as a mismatch here.
+        let input = [
+            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
+        ];
+        check_decimal_array(
+            spark_ceil,
+            &input,
+            (38, 6),
+            38,
+            vec![
+                Some(9_223_372_036_855_i128),
+                Some(9_223_372_036_855_i128),
+                Some(-9_223_372_036_854_i128),
+                Some(-9_223_372_036_854_i128),
+            ],
+        )?;
+        check_decimal_array(
+            spark_floor,
+            &input,
+            (38, 6),
+            38,
+            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),
+            ],
+        )
+    }
+
+    #[test]
+    fn test_decimal128_large_scale_array() -> Result<()> {


ceil.sql and floor.sql only cover FLOAT, DOUBLE and DECIMAL(5, 2), so a BIGINT column and a wide decimal column (DECIMAL(38, 6), DECIMAL(38, 19)) with values past i64::MAX are never compared with Spark. These Rust tests also pass Decimal128(38, 0) as the output type for a (38, 6) input, where Spark plans (33, 0) (p - s + 1). Could a few such rows be added to the two SQL files, read from a table so constant folding does not hide the native path? The i64 boundary and scale 19 cases could then be checked there against Spark too.

Fixed the precisions to (33, 0) and (20, 0), and added bigint, DECIMAL(38, 6) and DECIMAL(38, 19) rows to both files, read from a table. I also checked that ceil.sql fails if the wide path rounds the wrong way

utils.rs goes back to the decimal helpers that several math functions share. The rounding kernel, dispatch_pow10! and the tests are only used by ceil and floor, so they now live in one module that exports both.

num's div_ceil and div_floor are already generic over Integer, and Float covers ceil and floor for f32 and f64, so two generic helpers replace the four trait methods. A const CEIL parameter still folds the direction away at compile time, which removes the trait and the Ceil and Floor marker types.

Spark plans ceil and floor of DECIMAL(p, s) as DECIMAL(p - s + 1, 0), so a (38, 6) input produces (33, 0) and a (38, 19) input produces (20, 0). The tests passed 38 for both.

The SQL tests only covered float, double and DECIMAL(5, 2), so the i64 boundary, the 128-bit path and the scale 19 fallback were never compared with Spark. The new rows are read from a table so that the native kernel runs on them.

FreakingMind · 2026-11-11T08:37:25Z

I compared the generic kernel with the old ceil.rs and floor.rs path by path and found no behavior change, and all 22 original test cases are still there. The PR also removes the #[macro_export] macro downcast_compute_op from the crate root. Nothing in native/ uses it any more, but the crate is published, so that is a public API removal worth a line in the description.

Thanks for checking. Added a line about downcast_compute_op to the description

github-actions Bot added enhancement New feature or request test Testing related area:expressions Expression evaluation labels Nov 6, 2026

FreakingMind changed the title ~~test: add floor's decimal boundary tests to ceil~~ refactor: deduplicate spark_ceil and spark_floor Nov 6, 2026

sam-1112 approved these changes Nov 9, 2026

View reviewed changes

FreakingMind added 2 commits November 9, 2026 15:35

test: add floor's decimal boundary tests to ceil

6f098a4

Part of apache#6974.

refactor: share the ceil and floor implementation

ca5454d

Part of apache#6974.

FreakingMind force-pushed the ceil-decimal-boundary-tests branch 2 times, most recently from d406171 to b02254d Compare November 9, 2026 12:46

test: run the ceil and floor tests from one module

cab6ac9

Part of apache#6974.

FreakingMind force-pushed the ceil-decimal-boundary-tests branch from b02254d to cab6ac9 Compare November 9, 2026 13:00

comphead reviewed Nov 11, 2026

View reviewed changes

FreakingMind added 4 commits November 11, 2026 11:03

refactor: move the ceil and floor kernel into ceil_floor.rs

878908f

utils.rs goes back to the decimal helpers that several math functions share. The rounding kernel, dispatch_pow10! and the tests are only used by ceil and floor, so they now live in one module that exports both.

test: use Spark's result precision in the ceil and floor decimal tests

25a7da5

Spark plans ceil and floor of DECIMAL(p, s) as DECIMAL(p - s + 1, 0), so a (38, 6) input produces (33, 0) and a (38, 19) input produces (20, 0). The tests passed 38 for both.

Navigation Menu

Repository navigation

refactor: deduplicate spark_ceil and spark_floor - #7534

refactor: deduplicate spark_ceil and spark_floor#7534
FreakingMind wants to merge 7 commits into
apache:mainfrom
FreakingMind:ceil-decimal-boundary-tests

FreakingMind commented Nov 6, 2026 •

edited

Loading

Uh oh!

sam-1112 left a comment

Uh oh!

FreakingMind commented Nov 9, 2026

Uh oh!

comphead left a comment

Uh oh!

comphead Nov 11, 2026

Uh oh!

FreakingMind Nov 11, 2026 •

edited

Loading

Uh oh!

comphead Nov 11, 2026

Uh oh!

FreakingMind Nov 11, 2026

Uh oh!

comphead Nov 11, 2026

Uh oh!

FreakingMind Nov 11, 2026

Uh oh!

FreakingMind commented Nov 11, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

FreakingMind commented Nov 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

How are these changes tested?

Uh oh!

sam-1112 left a comment

Choose a reason for hiding this comment

Uh oh!

FreakingMind commented Nov 9, 2026

Uh oh!

comphead left a comment

Choose a reason for hiding this comment

Uh oh!

comphead Nov 11, 2026

Choose a reason for hiding this comment

Uh oh!

FreakingMind Nov 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

comphead Nov 11, 2026

Choose a reason for hiding this comment

Uh oh!

FreakingMind Nov 11, 2026

Choose a reason for hiding this comment

Uh oh!

comphead Nov 11, 2026

Choose a reason for hiding this comment

Uh oh!

FreakingMind Nov 11, 2026

Choose a reason for hiding this comment

Uh oh!

FreakingMind commented Nov 11, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

FreakingMind commented Nov 6, 2026 •

edited

Loading

FreakingMind Nov 11, 2026 •

edited

Loading