feat(spark): support the scale argument in Spark ceil by dhruvdavest07 · Pull Request #24643 · apache/datafusion

dhruvdavest07 · 2026-08-24T23:48:57Z

Which issue does this PR close?

Rationale for this change

ceil(value, scale) rounds up to a given number of decimal places. Only the one-argument
form was implemented, and ceil.rs carried a TODO pointing at this issue, with the two
matching cases in ceil.slt commented out.

Those commented-out cases already carried their PySpark 3.5.5 results, so the expected values
here are recorded Spark output rather than anything I decided.

What changes are included in this PR?

SparkCeil gains a two-argument form.

Spark builds it as RoundCeil, which declares inputTypes = Seq(DecimalType, IntegerType)
and extends RoundBase with RoundingMode.CEILING. So the value is a decimal and the scale
has to be a foldable constant. The result type is RoundBase::dataType:

val integralLeastNumDigits = p - s + 1
if (_scale < 0) {
  val newPrecision = math.max(integralLeastNumDigits, -_scale + 1)
  DecimalType(math.min(newPrecision, DecimalType.MAX_PRECISION), 0)
} else {
  val newScale = math.min(s, _scale)
  DecimalType(math.min(integralLeastNumDigits + newScale, 38), newScale)
}

ceil_scaled_precision and ceil_scaled_scale are that, transcribed. The p - s + 1 term is
the integral part gaining a digit, because rounding up can carry: ceil(9.9, 0) is 10.

Checked against the two recorded PySpark results:

Query	Spark type	Spark value	This PR
`ceil(3.1411::decimal(5,4), 3)`	`decimal(5,3)`	`3.142`	`Decimal128(5, 3)`, `3.142`
`ceil(3.1411::decimal(5,4), -3)`	`decimal(4,0)`	`1000`	`Decimal128(4, 0)`, `1000`

The result type depends on the scale's value, not just its type, so it is read through
return_field_from_args via scalar_arguments. A non-constant or null scale is a planning
error, which is what Spark reports as NON_FOLDABLE_INPUT.

Only decimal values accept a scale, matching RoundCeil's declared input types. A scale on
any other type is rejected rather than guessed at. Spark casts those to decimal first, and
picking a target precision for that cast is a separate decision I did not want to make
silently.

The one-argument path is untouched.

Are these changes tested?

Yes.

The two previously commented-out ceil.slt cases are enabled and pass with the recorded
PySpark values. Six unit tests in ceil.rs cover the two reference cases, rounding toward
positive infinity for negatives, a scale at or beyond the input scale being a no-op, the carry
case, precision capped at 38, and precision never dropping below 1.

cargo test -p datafusion-spark --lib
cargo test -p datafusion-sqllogictest --test sqllogictests -- spark/math

285 tests pass in datafusion-spark, and all 60 files under spark/math pass. cargo fmt --check and cargo clippy --all-targets are clean on the crate.

I ran the enabled .slt cases against the unmodified ceil.rs first: both fail with
Function 'ceil' expects 1 arguments but received 2, so they are exercising the new path.

Something a reviewer should decide

One case in the .slt is spelled ceiling rather than ceil, and that is not cosmetic.

sqlparser recognises CEIL(<expr>, <unsigned literal>) as the SQL-standard scale form and
hands it to the planner as CeilFloorKind::Scale. datafusion/sql/src/expr/mod.rs:619 rejects
that with CEIL with scale is not supported, before any function is resolved. A negative
scale does not match that production, so ceil(x, -3) reaches the function while ceil(x, 3)
does not. The ceiling alias parses as an ordinary function call either way.

So after this PR, in a Spark context:

ceil(x, -3) works
ceiling(x, 3) works
ceil(x, 3) still errors in the planner

Routing CeilFloorKind::Scale to a two-argument ceil call would fix the asymmetry, but it
changes core SQL planning for every session, and in plain DataFusion ceil takes one argument,
so the error would become a signature mismatch rather than today's clearer message. That is a
call for maintainers rather than something to slip into this PR, and FLOOR has the same
shape. Happy to send it as a follow-up if you want it, or to fold it in here if you would
rather it land together.

ceil(value, scale) rounds up to a given number of decimal places. Only the one-argument form was implemented, and the file carried a TODO pointing at this issue. Spark builds the two-argument form as RoundCeil, declared over (DecimalType, IntegerType) with a foldable scale, so the value is a decimal and the scale has to be a constant. The result type comes straight from Spark's RoundBase::dataType: scale < 0 Decimal128(min(max(p - s + 1, -scale + 1), 38), 0) scale >= 0 Decimal128(min(p - s + 1 + min(s, scale), 38), min(s, scale)) The p - s + 1 term is the integral part gaining a digit, since rounding up can carry: ceil(9.9, 0) is 10. Both formulas reproduce the PySpark 3.5.5 results already recorded in ceil.slt, which is where the expected values in the enabled tests come from. Those two cases were commented out with a TODO and are now live. The scale is read through return_field_from_args, since the result type depends on its value rather than just its type. A non-constant or null scale is a planning error, which is what Spark does too. Only decimal values take a scale, matching RoundCeil's declared input types. A scale on any other type is rejected rather than guessed at; Spark casts those to decimal first and that coercion is a separate piece of work. Fixes apache#21560

amitvijapur · 2026-09-12T18:54:36Z

Nice to see this one picked up @dhruvdavest07. I have been working in datafusion/spark recently so I had a read; three things, one of which is probably worth acting on.

1. The scale parser diverges from round's.

round.rs already has a get_scale that accepts Int8, Int16, Int32 and Int64 scale arguments. This PR parses the scale inline in return_field_from_args and matches only ScalarValue::Int32:

Some(ScalarValue::Int32(Some(scale))) => *scale,
...
_ => return plan_err!("Function ceil requires a constant integer scale argument"),

So round(x, 2::bigint) would work while ceil(x, 2::bigint) is a plan error, for two functions in the same module that take the same argument. Worth widening the match, or reusing round's parser.

2. There is approved prior art you may not have seen.

#21710 implements the same issue (#21560) and was approved after 11 rounds of review. Its author commented "untake" on 30 August and it has since gone conflicting, so this PR is the live one, but the review feedback on it is worth harvesting rather than rediscovering.

Relevant to point 1: #21710 lifted get_scale out of round.rs into a shared datafusion/spark/src/function/math/scale.rs as pub(crate) fn get_scale(fn_name, args), so ceil and round could not drift apart. That shape already passed review. It also means the duplication concern above was raised and settled before.

3. The scale path is decimal-only.

return_field_from_args errors for anything that is not Decimal128:

other => return plan_err!("Function ceil does not support a scale argument for {other:?}"),

#21710 had ceil_integer(value: i64, scale: i32) and ceil_float<T: Float>(value: T, scale: i32) for the same argument. If narrowing to decimal is deliberate, saying so in the description would save a reviewer asking. If it is not, that is a gap against the approved implementation.

For what it is worth, I like the return_field_from_args approach here — deriving the result type from the declared arguments rather than the values is the same thing we ended up doing for pmod in #24409, and ceil_scaled_precision capping at the Decimal128 maximum matches what that needed too.

Disclosure: written with AI assistance, reviewed by me before posting.

ShayanGho · 2026-09-19T05:23:19Z

@dhruvdavest07 Thanks for picking this up, and for the thorough description. I built this branch locally (head 410eaf4) and compared it against PySpark 4.2.0 with ANSI on and off. The normal decimal cases I tested match Spark, values and result types, including negative scales, a scale larger than the input scale, column input and NULLs. The ceil unit tests and ceil.slt pass locally. Note that CI is still awaiting workflow approval.

One correctness issue: overflow produces silent wrong results.

ceil(3.1411::decimal(5,4), -40) prints 13181135929278455956213638447872186777. Casting the result to Decimal256(76, 0) shows the stored value is 131811359292784559562136384478721867776, which is 10^40 wrapped at 2^128.
ceil(3.1411::decimal(5,4), -38) and ceil(CAST('99999999999999999999999999999999999999' AS decimal(38,0)), -1) both store 10^38 in a Decimal128(38, 0). That exceeds the declared precision, and it prints as a plausible-looking 38-digit number.

Spark raises NUMERIC_VALUE_OUT_OF_RANGE for all three, in both ANSI modes. The cause is the wrapping arithmetic (pow_wrapping in the decimal128_ceil helper, pow_wrapping and mul_wrapping in decimal128_ceil_to_scale) plus no check of the result against the output precision. Could these use checked arithmetic, validate the result, and return an error? Tests for these cases would be great.

A smaller gap on the scale argument: it has to be a bare literal. ceiling(x, CAST(3 AS INT)) and ceiling(x, 1+2) fail with "Function ceil requires a constant integer scale argument", but both are foldable constants that Spark accepts. On point 1 of the earlier comment: Spark rejects ceil(x, CAST(2 AS BIGINT)), and TINYINT and NULL scales too, so I would not widen the accepted types. In this PR that example fails because it is a cast, and 3::int fails the same way.

On the decimal-only scope, which the description explains: keeping it out of this PR seems right to me. In case it helps the follow-up, Spark's numeric-to-decimal mappings are in DecimalType.forType: Int is decimal(10,0), Long is decimal(20,0), Float is decimal(14,7), Double is decimal(30,15). The results I measured are consistent with that: ceil(5, -1) = 10 decimal(11,0), ceil(CAST(5 AS BIGINT), 1) = 5 decimal(21,0), ceil(3.5D, 0) = 4 decimal(16,0), ceil(CAST(3.14159 AS FLOAT), 2) = 3.15 decimal(10,2).

On the "something a reviewer should decide" question: I confirmed ceil(x, 3) is still rejected in the planner on current main, for both CEIL and FLOOR. A follow-up issue covering both seems cleaner than folding planner changes into this PR. Not my call, just one reviewer's view.

@comphead this PR targets your #21560 . Could you approve the CI workflows when you get a chance?

comphead · 2026-09-19T05:31:31Z

Thanks @dhruvdavest07

github-actions · 2026-09-19T05:36:58Z

Thank you for opening this pull request!

Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).

Details

     Cloning apache/main
    Building datafusion-spark v55.0.0 (current)
       Built [  42.094s] (current)
     Parsing datafusion-spark v55.0.0 (current)
      Parsed [   0.056s] (current)
    Building datafusion-spark v55.1.0 (baseline)
       Built [  40.278s] (baseline)
     Parsing datafusion-spark v55.1.0 (baseline)
      Parsed [   0.055s] (baseline)
    Checking datafusion-spark v55.1.0 -> v55.0.0 (minor change)
     Checked [   0.379s] 196 checks: 192 pass, 4 fail, 0 warn, 58 skip

--- failure function_missing: pub fn removed or renamed ---

Description:
A publicly-visible function cannot be imported by its prior path. A `pub use` may have been removed, or the function itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/function_missing.ron

Failed in:
  function datafusion_spark::function::string::expr_fn::encode, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/56972f9d1535dd158aba2e654cd4e99e2b2e4577/datafusion/spark/src/function/string/mod.rs:97
  function datafusion_spark::expr_fn::encode, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/56972f9d1535dd158aba2e654cd4e99e2b2e4577/datafusion/spark/src/function/string/mod.rs:97
  function datafusion_spark::function::string::encode, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/56972f9d1535dd158aba2e654cd4e99e2b2e4577/datafusion/spark/src/function/string/mod.rs:50

--- failure function_parameter_count_changed: pub fn parameter count changed ---

Description:
A publicly-visible function now takes a different number of parameters.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#fn-change-arity
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/function_parameter_count_changed.ron

Failed in:
  datafusion_spark::function::math::modulus::spark_pmod now takes 2 parameters instead of 3, in /home/runner/work/datafusion/datafusion/datafusion/spark/src/function/math/modulus.rs:107

--- failure module_missing: pub module removed or renamed ---

Description:
A publicly-visible module cannot be imported by its prior path. A `pub use` may have been removed, or the module may have been renamed, removed, or made non-public.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/module_missing.ron

Failed in:
  mod datafusion_spark::function::string::encode, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/56972f9d1535dd158aba2e654cd4e99e2b2e4577/datafusion/spark/src/function/string/encode.rs:18

--- failure struct_missing: pub struct removed or renamed ---

Description:
A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/struct_missing.ron

Failed in:
  struct datafusion_spark::function::string::encode::SparkEncode, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/56972f9d1535dd158aba2e654cd4e99e2b2e4577/datafusion/spark/src/function/string/encode.rs:59

     Summary semver requires new major version: 4 major and 0 minor checks failed
    Finished [  84.961s] datafusion-spark
    Building datafusion-sqllogictest v55.0.0 (current)
       Built [  87.920s] (current)
     Parsing datafusion-sqllogictest v55.0.0 (current)
      Parsed [   0.018s] (current)
    Building datafusion-sqllogictest v55.1.0 (baseline)
       Built [  89.932s] (baseline)
     Parsing datafusion-sqllogictest v55.1.0 (baseline)
      Parsed [   0.021s] (baseline)
    Checking datafusion-sqllogictest v55.1.0 -> v55.0.0 (minor change)
     Checked [   0.080s] 196 checks: 193 pass, 3 fail, 0 warn, 58 skip

--- failure function_missing: pub fn removed or renamed ---

Description:
A publicly-visible function cannot be imported by its prior path. A `pub use` may have been removed, or the function itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/function_missing.ron

Failed in:
  function datafusion_sqllogictest::run_each_configuration, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/56972f9d1535dd158aba2e654cd4e99e2b2e4577/datafusion/sqllogictest/src/config_matrix.rs:101
  function datafusion_sqllogictest::test_configurations, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/56972f9d1535dd158aba2e654cd4e99e2b2e4577/datafusion/sqllogictest/src/config_matrix.rs:81

--- failure inherent_method_missing: pub method removed or renamed ---

Description:
A publicly-visible method or associated fn is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/inherent_method_missing.ron

Failed in:
  TestContext::apply_config_overrides, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/56972f9d1535dd158aba2e654cd4e99e2b2e4577/datafusion/sqllogictest/src/test_context.rs:258

--- failure struct_missing: pub struct removed or renamed ---

Description:
A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/struct_missing.ron

Failed in:
  struct datafusion_sqllogictest::TestConfiguration, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/56972f9d1535dd158aba2e654cd4e99e2b2e4577/datafusion/sqllogictest/src/config_matrix.rs:45

     Summary semver requires new major version: 3 major and 0 minor checks failed
    Finished [ 181.128s] datafusion-sqllogictest

codecov-commenter · 2026-09-19T05:57:14Z

Codecov Report

❌ Patch coverage is 80.80000% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.43%. Comparing base (c699996) to head (410eaf4).
⚠️ Report is 358 commits behind head on main.

Files with missing lines	Patch %	Lines
datafusion/spark/src/function/math/ceil.rs	80.80%	23 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #24643      +/-   ##
==========================================
- Coverage   81.43%   81.43%   -0.01%     
==========================================
  Files        1118     1118              
  Lines      399414   399535     +121     
  Branches   399414   399535     +121     
==========================================
+ Hits       325281   325364      +83     
- Misses      55142    55176      +34     
- Partials    18991    18995       +4

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

github-actions · 2026-10-26T01:27:29Z

Marked as draft after 14 days without activity: failing CI checks.

Please address these items and mark Ready for review when ready.

alamb · 2026-10-29T19:12:45Z

Thanks for the contribution! I am closing this under DataFusion's AI Dump policy.

https://datafusion.apache.org/contributor-guide/ai-policy.html#what-is-an-ai-dump-and-why-it-is-not-helpful

This has the hallmarks of AI code that was not reviewed by the author: a very long description with test pass counts, sentence-style test names, and no reply to the review comments left in September.

If you would like to continue, please open a focused PR in your own words that you have reviewed and understand, and we are happy to help with review.

github-actions Bot added sqllogictest SQL Logic Tests (.slt) spark Changes to datafusion-spark crate labels Aug 24, 2026

github-actions Bot added the auto detected api change Auto detected API change label Sep 19, 2026

github-actions Bot marked this pull request as draft October 26, 2026 01:27

alamb closed this Oct 29, 2026

ump45nose mentioned this pull request Oct 30, 2026

feat(spark): support ceil(decimal, scale) #26914

Closed

Navigation Menu

Repository navigation

feat(spark): support the scale argument in Spark ceil - #24643

feat(spark): support the scale argument in Spark ceil#24643
dhruvdavest07 wants to merge 1 commit into
apache:mainfrom
dhruvdavest07:feat/spark-ceil-scale-argument

dhruvdavest07 commented Aug 24, 2026

Uh oh!

amitvijapur commented Sep 12, 2026

Uh oh!

ShayanGho commented Sep 19, 2026

Uh oh!

comphead commented Sep 19, 2026

Uh oh!

github-actions Bot commented Sep 19, 2026

Uh oh!

codecov-commenter commented Sep 19, 2026

Uh oh!

github-actions Bot commented Oct 26, 2026

Uh oh!

alamb commented Oct 29, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

dhruvdavest07 commented Aug 24, 2026

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Something a reviewer should decide

Uh oh!

amitvijapur commented Sep 12, 2026

Uh oh!

ShayanGho commented Sep 19, 2026

Uh oh!

comphead commented Sep 19, 2026

Uh oh!

github-actions Bot commented Sep 19, 2026

Uh oh!

codecov-commenter commented Sep 19, 2026

Codecov Report

Uh oh!

github-actions Bot commented Oct 26, 2026

Uh oh!

alamb commented Oct 29, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants