viirya commented on PR #23033:
URL: https://github.com/apache/datafusion/pull/23033#issuecomment-4758860161
Following up on @Jefffrey's note about the overlap with #22697 — I dug into
both and wanted to lay out the relationship so the maintainers can decide which
path to take.
**Scope:** @Jefffrey is right that this PR addresses only the Spark side.
Issue #22696 has two halves: (1) core `datafusion-functions` `round()` losing
precision on `Int64` above `2^53` by routing through `Float64`, and (2) Spark
`round()` failing on `UInt64` above `i64::MAX`. This PR fixes only (2); #22697
covers both.
**But the two PRs fix the Spark `UInt64` path differently, and this one is
more complete there:**
- #22697 (current state) only short-circuits when `scale >= 0` (the no-op
case). For **negative** scale with a value above `i64::MAX`, it still falls
back to `i64::try_from(...)` and errors:
```rust
if scale >= 0 { Ok(...v...) } else {
let v_i64 = i64::try_from(*v).map_err(...)?; // still narrows →
errors for u64 > i64::MAX
...
}
```
- This PR's `round_unsigned_integer` handles negative scale natively in
`u64`, so e.g. `round(u64::MAX, -1)` rounds correctly instead of erroring.
That's the case the unit test `round_unsigned_integer_overflow` and the `-1`
SLT case exercise.
**On whether the Spark `UInt64` codepath should exist at all:** @Jefffrey
raised this on #22697 too ("spark doesn't have a u64 type"). Worth noting the
`SparkRound` signature coerces via `TypeSignatureClass::Integer`, which
includes unsigned types, so `arrow_cast(x, 'UInt64')` does reach this arm today
— it's live, not dead code. Whether it *should* be supported is a separate
design question; if the answer is "drop unsigned support," that would supersede
both this fix and #22697's Spark portion.
No strong opinion from me on which PR lands — just flagging that if the
Spark `UInt64` path stays, this PR's negative-scale handling is the more
correct of the two. Deferring to the maintainers on scope/direction.
--
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]
