You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "Kevin-Li-2025 (via GitHub)" <gi...@apache.org> on 2026/06/19 02:57:59 UTC
[PR] fix: round large UInt64 values without narrowing [datafusion]
Kevin-Li-2025 opened a new pull request, #23033:
URL: https://github.com/apache/datafusion/pull/23033
## Which issue does this PR close?
- Closes #22696.
## Rationale for this change
Spark `round` currently narrows `UInt64` inputs through `i64`. Values above `i64::MAX` therefore fail even when a non-negative scale makes rounding a no-op. The narrowing also prevents correct negative-scale rounding for the upper half of the `UInt64` domain.
## What changes are included in this PR?
- Add a native `u64` HALF_UP rounding path with the same ANSI overflow and non-ANSI wrapping behavior as the signed implementation.
- Use it for both scalar and array `UInt64` inputs.
- Add helper-level coverage for large values and overflow behavior.
- Add SQL regression coverage for `u64::MAX` with default/positive scales and a large negative-scale case.
## Are these changes tested?
Yes:
- `cargo test -p datafusion-spark function::math::round::tests --lib`
- `cargo test -p datafusion-sqllogictest --test sqllogictests -- spark/math/round --test-threads 1`
- `cargo clippy -p datafusion-spark --features core --all-targets -- -D warnings`
- `cargo fmt --all -- --check`
## Are there any user-facing changes?
Yes. Spark `round` now accepts `UInt64` values above `i64::MAX` and returns correct results instead of reporting a narrowing error. There are no public API changes.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: round large UInt64 values without narrowing [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #23033:
URL: https://github.com/apache/datafusion/pull/23033#issuecomment-5350265523
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: round large UInt64 values without narrowing [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey closed pull request #23033: fix: round large UInt64 values without narrowing
URL: https://github.com/apache/datafusion/pull/23033
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: round large UInt64 values without narrowing [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #23033:
URL: https://github.com/apache/datafusion/pull/23033#issuecomment-4757077083
Fyi we have an existing PR:
- https://github.com/apache/datafusion/pull/22697
This PR seems to fix only spark, while the original issue seems to describe for datafusion function. Also worth noting Spark doesn't support unsigned types in the first place (I made a comment to this affect on the linked PR)
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: round large UInt64 values without narrowing [datafusion]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #23033:
URL: https://github.com/apache/datafusion/pull/23033#issuecomment-4759948505
Some archaeology on the "Spark doesn't have a u64 type" point, in case it helps decide the direction here.
You're right that Spark can't produce a `UInt64`. Spark SQL's integer types are `ByteType`/`ShortType`/`IntegerType`/`LongType` — all signed (JVM has no unsigned integers). This PR's own doc comment on `round_integer` reflects that: it says it matches *"Spark's `RoundBase` behaviour for `ByteType`, `ShortType`, `IntegerType`, and `LongType`"* — only the signed types.
So why does the code accept `UInt64` at all? It looks **incidental, not intentional**. The signature has used `Coercion::new_exact(TypeSignatureClass::Integer)` since the function was first added in #21062, and `TypeSignatureClass::Integer` matches on `NativeType::is_integer()`, which includes the unsigned types:
```rust
// datafusion/common/src/types/native.rs
pub fn is_integer(&self) -> bool {
matches!(self, UInt8 | UInt16 | UInt32 | UInt64 | Int8 | Int16 | Int32 | Int64)
}
```
So `UInt8..UInt64` came in "for free" via that coercion class rather than by deliberate design, and the `UInt64` dispatch arm exists only because the signature lets those values through (e.g. via `arrow_cast(x, 'UInt64')`) — Spark itself never generates them.
That seems to leave two coherent directions:
1. **Keep the codepath and make it correct** — what this PR (and #22697's Spark portion) does.
2. **Narrow the signature to signed-only** and drop the `UInt*` dispatch arms, so Spark `round` matches Spark's actual type domain — which would make both this fix and #22697's Spark change unnecessary.
No opinion from me on which is preferable — just flagging that the unsigned support appears to be a side effect of the coercion class rather than a requirement, in case that informs the call.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: round large UInt64 values without narrowing [datafusion]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org