amitvijapur commented on PR #24643:
URL: https://github.com/apache/datafusion/pull/24643#issuecomment-5647987543
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`:
```rust
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`:
```rust
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.
--
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]
