[datafusion-spark] Support 2-argument ceil(value, scale) by diegoQuinas · Pull Request #21710 · apache/datafusion

diegoQuinas · 2026-04-17T23:11:33Z

Which issue does this PR close?

Rationale for this change

The Spark ceil function supports an optional scale parameter that controls
the decimal position to round up to. This was not yet implemented in
datafusion-spark.

What changes are included in this PR?

Updated Signature to accept 1 or 2 arguments, following the same pattern as SparkRound
Updated return_type: floats preserve their type when a scale is provided (instead of returning Int64); scale=0 preserves the original behavior
Added get_scale() helper to extract the optional scale argument, returning None for NULL scale (which produces a NULL result)
Added ceil_float() helper for ceiling floats at arbitrary decimal positions
Updated spark_ceil_scalar and spark_ceil_array to apply the scale

Are these changes tested?

Yes, unit tests covering:

Float64/Float32 scalar with positive scale, negative scale, zero scale, and NULL scale
Float64 array with scale
Existing 1-argument tests continue to pass

Are there any user-facing changes?

Yes — ceil(expr, scale) is now supported in addition to the existing ceil(expr).

kosiew

@diegoQuinas
Thanks for the work here.
There are a few inconsistencies between the declared return types and the actual execution paths that could lead to planner/runtime mismatches. I’ve called those out below. Once those are addressed and we have a couple of type-contract tests in place, this should be in a much safer spot.

- Fix type contract: 2-arg ceil now preserves input type for any scale (incl. scale=0 on floats); decimal 2-arg surfaces NotImplemented - Implement integer ceil at negative scale (ceiling toward +inf, distinct from HALF_UP round); positive scale on integer is a no-op - Extract shared get_scale helper into math/scale.rs and reuse from round - Simplify Signature using TypeSignature::Numeric + Coercible([Numeric, Int32]) instead of hand-enumerated type lists - Move Rust unit tests into the SLT, including type-contract assertions via arrow_typeof for every 2-arg overload

kosiew

@diegoQuinas
Nice follow-up overall. This addresses all the original comments and the refactor looks clean. The type handling is much more consistent now and the SLT coverage is solid. I left a few small suggestions, mostly around the decimal 2-arg behavior and some minor readability and edge-case clarifications.

kosiew · 2026-04-29T05:27:04Z

        match &arg_types[0] {
+            // 2-arg decimal is not yet supported; report input type so the planner
+            // does not reject the call before we surface a proper error at execution.
+            DataType::Decimal128(p, s) if has_scale => Ok(DataType::Decimal128(*p, *s)),


Good progress here, but there is still a mismatch between planning and execution for the 2-arg decimal case.

Right now return_type allows Decimal128(p, s), so the planner accepts the query. Then execution always throws not_impl_err. That means users only see the failure at runtime, which can be confusing.

It would be cleaner to fail early in return_type instead, so the planner rejects it with a clear message. Something like:

DataType::Decimal128(_, _) if has_scale => {
exec_err!("2-argument ceil is not yet supported for decimal inputs")
}

This keeps the schema contract honest and improves the developer experience.

kosiew · 2026-04-29T05:27:04Z

+        }
+        (value * factor).ceil() / factor
+    } else {
+        let factor = T::from(10.0f64.powi(-scale)).unwrap_or_else(T::infinity);


This edge case caught my eye. When scale is very negative and the factor overflows, we silently return 0.0.

I see this matches the integer path, but for floats it is not obvious why that is correct. It might help to add a short comment explaining the intent here, especially that extremely coarse rounding collapses everything to zero and that this is consistent with the integer behavior.

A tiny note would save future readers some head scratching.

kosiew · 2026-04-29T05:27:04Z

+            impl_integer_array_ceil!(input, UInt32Type, scale)
+        }
+        DataType::UInt64 if has_scale => {
+            let array = input.as_primitive::<UInt64Type>();


The error construction here works, but it is a bit hard to read.

The (exec_err!(...) as Result<(), _>).unwrap_err() pattern feels a little roundabout. You could simplify this by mapping directly to a DataFusionError, similar to how other paths handle it.

Something like:

let v = i64::try_from(x).map_err(|_| {
datafusion_common::exec_datafusion_err!(
"ceil: UInt64 value {x} exceeds i64::MAX and cannot be processed"
)
})?;

Same behavior, but easier on the eyes.

Jefffrey · 2026-04-29T16:28:19Z

-/// <https://github.com/apache/datafusion/issues/21560>
+/// Two-argument form `ceil(expr, scale)` returns the same type as `expr` for
+/// integer and floating-point inputs (regardless of `scale`).  Decimal inputs
+/// are not yet supported in the 2-arg form (see TODO in execution path).


I'd prefer not having this detail of decimals not being supported on the 2-arg path in the docstring; it should just be a TODO item for clear visibility

Jefffrey · 2026-04-29T16:28:39Z

+
        match &arg_types[0] {
+            // 2-arg decimal is not yet supported; report input type so the planner
+            // does not reject the call before we surface a proper error at execution.


We should error at planning time instead of execution time if it is not supported

Jefffrey · 2026-04-29T16:31:43Z

+        // Decimal128 with positive scale (1-arg only).
+        ScalarValue::Decimal128(_, _, _) if has_scale => {
+            return not_impl_err!(
+                "2-argument ceil is not yet supported for decimal inputs"


If we aren't supporting this yet we should keep the original issue open instead of marking this PR as closing it

Jefffrey · 2026-04-29T16:35:57Z

+    }
+}
+
+fn out_of_range_err<T: std::fmt::Display>(


We can use exec_datafusion_err like so

datafusion/datafusion-examples/examples/execution_monitoring/memory_pool_execution_plan.rs

Lines 289 to 291 in 5fda216

.map_err(|e| {

exec_datafusion_err!("Failed to create final RecordBatch: {e}")

})

Jefffrey · 2026-04-29T16:37:04Z

+    if scale >= 0 {
+        let factor = T::from(10.0f64.powi(scale)).unwrap_or_else(T::infinity);
+        if factor.is_infinite() {
+            return value;


Does this match with Spark behaviour?

diegoQuinas added 3 commits April 17, 2026 20:05

feat(spark): support 2-argument ceil(value, scale) (apache#21560)

31fc32a

test(spark): add tests for 2-argument ceil(value, scale) (apache#21560)

399d300

feat(spark): export ceil with optional scale argument (apache#21560)

fa29e5f

github-actions Bot added the spark label Apr 17, 2026

diegoQuinas mentioned this pull request Apr 18, 2026

Support 2-argument ceil(value, scale) for Spark #21560

Open

kosiew requested changes Apr 23, 2026

View reviewed changes

Merge branch 'main' into feat/ceil-two-args

b7faa1a

Jefffrey mentioned this pull request Apr 26, 2026

feat: support 2-argument ceil(value, scale) for Spark compatibility #21562

Closed

Jefffrey reviewed Apr 26, 2026

View reviewed changes

Comment thread datafusion/spark/src/function/math/ceil.rs Outdated

Comment thread datafusion/spark/src/function/math/ceil.rs Outdated

github-actions Bot added the sqllogictest SQL Logic Tests (.slt) label Apr 28, 2026

kosiew approved these changes Apr 29, 2026

View reviewed changes

Jefffrey reviewed Apr 29, 2026

View reviewed changes

Merge branch 'main' into feat/ceil-two-args

3b5cf95

	.map_err(\|e\| {
	exec_datafusion_err!("Failed to create final RecordBatch: {e}")
	})

Conversation

diegoQuinas commented Apr 17, 2026

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!

kosiew left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kosiew left a comment • 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!

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kosiew left a comment •

edited

Loading

kosiew left a comment •

edited

Loading