dhruvdavest07 opened a new pull request, #24643:
URL: https://github.com/apache/datafusion/pull/24643
## 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-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`:
```scala
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
