Add Decimal128 support to Ceil and Floor #18979

kumarUjjawal · 2025-11-28T09:59:19Z

Which issue does this PR close?

Closes Add Decimal128 support to Ceil and Floor #7689.

Rationale for this change

What changes are included in this PR?

Added dedicated ceil/floor UDF implementations that keep existing float/int behavior but operate directly on Decimal128 arrays, including overflow checks and metadata preservation.
Updated the math module wiring plus sqllogictest coverage so decimal cases are executed and validated end to end.

Are these changes tested?

All existing tests pass
Added new tests for the changes

Are there any user-facing changes?

Jefffrey

Thanks for picking this up, left some comments (applicable to floor too)

datafusion/functions/src/math/ceil.rs

Jefffrey · 2025-11-28T10:06:29Z

datafusion/functions/src/math/ceil.rs

+    fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
+        match arg_types[0] {
+            DataType::Float32 => Ok(DataType::Float32),
+            DataType::Decimal128(precision, scale) => {


We should include support for all decimal types, not just 128

I'm thinking of creating a shared module for this, would that be okay.

Actually I think this signature can be simplified to:

if arg_types[0].is_null() { Ok(DataType::Float64) else { Ok(arg_types[0].clone()) }

datafusion/functions/src/math/ceil.rs

Jefffrey · 2025-11-28T10:12:42Z

datafusion/sqllogictest/test_files/scalar.slt

+# ceil with decimal argument
+query RRRR
+select
+  ceil(arrow_cast(1.23,'Decimal128(10,2)')),
+  ceil(arrow_cast(-1.23,'Decimal128(10,2)')),
+  ceil(arrow_cast(123.00,'Decimal128(10,2)')),
+  ceil(arrow_cast(-123.00,'Decimal128(10,2)'));
+----
+2 -1 123 -123


Should check the output type using arrow_typeof to ensure we're actually getting decimals and not just floats

Assuming it returns decimals: Why the numbers after the decimal point (.00) are not printed ?
I'd expect them to be printed for decimals but maybe it has been decided in the past to not do it for .0

datafusion/functions/src/math/ceil.rs

martin-g · 2025-11-28T11:17:23Z

datafusion/sqllogictest/test_files/scalar.slt

+# ceil with decimal argument
+query RRRR
+select
+  ceil(arrow_cast(1.23,'Decimal128(10,2)')),


Would the following will be valid:

ceil(arrow_cast(9.23,'Decimal128(3,2)')),

?
It would return 10.00 which is above the requested precision of 3

Yes, it should raise a compute error. I will handle this case.

martin-g · 2025-11-28T11:20:43Z

datafusion/sqllogictest/test_files/scalar.slt

+# ceil with decimal argument
+query RRRR
+select
+  ceil(arrow_cast(1.23,'Decimal128(10,2)')),
+  ceil(arrow_cast(-1.23,'Decimal128(10,2)')),
+  ceil(arrow_cast(123.00,'Decimal128(10,2)')),
+  ceil(arrow_cast(-123.00,'Decimal128(10,2)'));
+----
+2 -1 123 -123


Assuming it returns decimals: Why the numbers after the decimal point (.00) are not printed ?
I'd expect them to be printed for decimals but maybe it has been decided in the past to not do it for .0

kumarUjjawal · 2025-11-28T19:11:27Z

datafusion/sqllogictest/test_files/scalar.slt

+  arrow_typeof(ceil(arrow_cast('-9999999999999999999999999999999999.01','Decimal256(38,2)'))),
+  arrow_cast(ceil(arrow_cast('-9999999999999999999999999999999999.01','Decimal256(38,2)')), 'Utf8');
+----
+Decimal256(38, 2) 10000000000000000000000000000000000.00 Decimal256(38, 2) -9999999999999999999999999999999999.00


@martin-g is this the right way to handle this test?

Jefffrey · 2025-11-29T01:25:55Z

datafusion/functions/src/math/ceil.rs

+    fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
+        match arg_types[0] {
+            DataType::Float32 => Ok(DataType::Float32),
+            DataType::Decimal128(precision, scale) => {


Actually I think this signature can be simplified to:

if arg_types[0].is_null() { Ok(DataType::Float64) else { Ok(arg_types[0].clone()) }

Jefffrey · 2025-11-29T01:27:21Z

datafusion/functions/src/math/decimal.rs

+use datafusion_common::{exec_err, Result};
+
+/// Operations required to manipulate the native representation of Arrow decimal arrays.
+pub(super) trait DecimalNative:


Can't help but feel this trait is unnecessary; is there some other existing code (traits) we could use instead of implementing this? 🤔

I think we can use ArrowNativeTypeOp.

datafusion/functions/src/math/ceil.rs

Jefffrey · 2025-11-29T01:29:29Z

datafusion/functions/src/math/decimal.rs

+    Ok(Arc::new(result))
+}
+
+fn decimal_scale_factor<T>(scale: i8, fn_name: &str) -> Result<T::Native>


I feel we should just inline this function; doing so would make it clear that scale was already checked for being non-negative

Jefffrey · 2025-11-29T01:32:50Z

datafusion/functions/src/math/floor.rs

+    fn output_ordering(&self, input: &[ExprProperties]) -> Result<SortProperties> {
+        Ok(input[0].sort_properties)
+    }
+
+    fn evaluate_bounds(&self, inputs: &[&Interval]) -> Result<Interval> {
+        let data_type = inputs[0].data_type();
+        Interval::make_unbounded(&data_type)
+    }


Ah maybe I was wrong about inlining for ordering & bounds actually 🤔

Because bounds was generic to all the other functions, and for ordering we have tests against it so inlining here leaves that ceil_order function dangling 🤔

Jefffrey · 2025-11-29T01:33:26Z

datafusion/sqllogictest/test_files/scalar.slt

+query TTTTTTTT
+select
+  arrow_typeof(ceil(arrow_cast(9.01,'Decimal32(7,2)'))),
+  arrow_cast(ceil(arrow_cast(9.01,'Decimal32(7,2)')), 'Utf8'),


Don't think this cast to utf8 is necessary

Co-authored-by: Jeffrey Vo <jeffrey.vo.australia@gmail.com>

Add Decimal128 support to Ceil and Floor

08dbd6e

github-actions bot added sqllogictest SQL Logic Tests (.slt) functions Changes to functions implementation labels Nov 28, 2025

Jefffrey reviewed Nov 28, 2025

View reviewed changes

martin-g reviewed Nov 28, 2025

View reviewed changes

created new module for decimal shared generic code

47b8f26

kumarUjjawal commented Nov 28, 2025

View reviewed changes

Jefffrey reviewed Nov 29, 2025

View reviewed changes

kumarUjjawal and others added 2 commits November 29, 2025 12:15

Update datafusion/functions/src/math/ceil.rs

7ac92ff

Co-authored-by: Jeffrey Vo <jeffrey.vo.australia@gmail.com>

used alreay present traits for decimal

ae2fbd0

github-actions bot added the execution Related to the execution crate label Nov 29, 2025

fixed tests and updated docs

df2be81

github-actions bot added the documentation Improvements or additions to documentation label Nov 29, 2025

Add Decimal128 support to Ceil and Floor #18979

Are you sure you want to change the base?

Add Decimal128 support to Ceil and Floor #18979

Conversation

kumarUjjawal commented Nov 28, 2025

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Jefffrey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants