feat(spark): support the scale argument in Spark ceil - #24643
Open
dhruvdavest07 wants to merge 1 commit into
Open
feat(spark): support the scale argument in Spark ceil#24643dhruvdavest07 wants to merge 1 commit into
dhruvdavest07 wants to merge 1 commit into
Conversation
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Closes #21560.
Rationale for this change
ceil(value, scale)rounds up to a given number of decimal places. Only the one-argumentform was implemented, and
ceil.rscarried a TODO pointing at this issue, with the twomatching cases in
ceil.sltcommented 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?
SparkCeilgains a two-argument form.Spark builds it as
RoundCeil, which declaresinputTypes = Seq(DecimalType, IntegerType)and extends
RoundBasewithRoundingMode.CEILING. So the value is a decimal and the scalehas to be a foldable constant. The result type is
RoundBase::dataType:ceil_scaled_precisionandceil_scaled_scaleare that, transcribed. Thep - s + 1term isthe integral part gaining a digit, because rounding up can carry:
ceil(9.9, 0)is10.Checked against the two recorded PySpark results:
ceil(3.1411::decimal(5,4), 3)decimal(5,3)3.142Decimal128(5, 3),3.142ceil(3.1411::decimal(5,4), -3)decimal(4,0)1000Decimal128(4, 0),1000The result type depends on the scale's value, not just its type, so it is read through
return_field_from_argsviascalar_arguments. A non-constant or null scale is a planningerror, which is what Spark reports as
NON_FOLDABLE_INPUT.Only decimal values accept a scale, matching
RoundCeil's declared input types. A scale onany 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.sltcases are enabled and pass with the recordedPySpark values. Six unit tests in
ceil.rscover the two reference cases, rounding towardpositive 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.
285 tests pass in
datafusion-spark, and all 60 files underspark/mathpass.cargo fmt --checkandcargo clippy --all-targetsare clean on the crate.I ran the enabled
.sltcases against the unmodifiedceil.rsfirst: both fail withFunction 'ceil' expects 1 arguments but received 2, so they are exercising the new path.Something a reviewer should decide
One case in the
.sltis spelledceilingrather thanceil, and that is not cosmetic.sqlparser recognises
CEIL(<expr>, <unsigned literal>)as the SQL-standard scale form andhands it to the planner as
CeilFloorKind::Scale.datafusion/sql/src/expr/mod.rs:619rejectsthat with
CEIL with scale is not supported, before any function is resolved. A negativescale does not match that production, so
ceil(x, -3)reaches the function whileceil(x, 3)does not. The
ceilingalias parses as an ordinary function call either way.So after this PR, in a Spark context:
ceil(x, -3)worksceiling(x, 3)worksceil(x, 3)still errors in the plannerRouting
CeilFloorKind::Scaleto a two-argumentceilcall would fix the asymmetry, but itchanges core SQL planning for every session, and in plain DataFusion
ceiltakes 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
FLOORhas the sameshape. 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.