andygrove commented on code in PR #5082:
URL: https://github.com/apache/datafusion-comet/pull/5082#discussion_r3692485403
##########
native/spark-expr/src/math_funcs/round.rs:
##########
@@ -59,6 +59,29 @@ macro_rules! integer_round {
}};
}
+// Round a single native integer when `10^(-point)` does not fit in the native
+// integer type but still fits in i128. The caller has already excluded the
+// case where `div` fits the native type, so `|x| < div` and the result is
+// either `0` (when `|x| < half`) or `sign(x) * div` — the latter always
+// overflows the native type. Under ANSI we throw, under legacy we wrap by
+// truncation (matches `BigDecimal.longValue`'s low-64-bit semantics).
+macro_rules! integer_round_widened {
+ ($X:expr, $DIV:expr, $HALF:expr, $NATIVE:ty, $FAIL_ON_ERROR:expr) => {{
+ let x128 = $X as i128;
+ if x128 > -$HALF && x128 < $HALF {
+ Ok(0 as $NATIVE)
+ } else if $FAIL_ON_ERROR {
+ Err(ArrowError::ComputeError(
+ arithmetic_overflow_error("integer").to_string(),
+ ))
+ } else if x128 >= $HALF {
+ Ok($DIV as $NATIVE)
+ } else {
+ Ok((-$DIV) as $NATIVE)
+ }
+ }};
+}
Review Comment:
I tried this and I would like to push back, with numbers.
I implemented it exactly as described — one `integer_round!` that always
computes `rem`/`half` in i128 and does a single checked-or-wrapping downcast at
the end, deleting `integer_round_widened!` and reducing the dispatch to two
branches (still need the `10^(-point)` doesn't-fit-i128 fallback, since
`checked_pow` returns `None` there and we have no `div` to divide by). It is
genuinely nicer to read, and it dissolved your other three comments on this
macro for free. But it costs 2.3-3.3x on these kernels:
| Benchmark | native + widened | unified i128 | Change |
| --- | --- | --- | --- |
| int64 scale=-1 legacy | 16.0 µs | 42.9 µs | **+168%** |
| int64 scale=-1 ansi | 15.8 µs | 42.8 µs | **+171%** |
| int64 scale=-9 legacy | 18.6 µs | 43.3 µs | **+132%** |
| int64 scale=-19 legacy | 14.7 µs | 48.0 µs | **+226%** |
| int32 scale=-1 legacy | 15.2 µs | 40.5 µs | **+166%** |
| int32 scale=-1 ansi | 15.2 µs | 43.3 µs | **+185%** |
(criterion, 8192 rows, same machine, baseline re-measured back-to-back and
reproducing within 3%, and the regression reproduced on a second run.)
Two causes. On the native band, `x % div` becomes a 128-bit division, which
on arm64 and x86-64 is an `__umodti3`/`__udivti3` libcall rather than a single
instruction. On the widened band it is worse: today that band does **no
division at all** — the `|x| <= NATIVE::MAX < div` invariant makes `x % div ==
x`, so it is just two comparisons — whereas the unified version pays a 128-bit
modulo by a divisor near 1e19, the most expensive shape there is.
So I have kept the two macros, but taken the substance of your comment,
which I think is the real point: the invariant was load-bearing and only stated
in prose. It is now (a) spelled out as `|x| <= NATIVE::MAX < div`, with the `x
% div == x` consequence and the fact that no division is needed made explicit,
and (b) enforced by a `debug_assert!` inside `integer_round_widened!`, so any
future caller that violates it fails loudly in tests rather than silently
computing the wrong thing. Tests run in debug, so this is live coverage.
The comparison to `decimal_round_f` I think cuts the other way,
incidentally: it computes in i128 because its input *is* an i128 unscaled
value, so there is no narrower type to fall back to. Here the input is
i8/i16/i32/i64 and the narrow path is the common case, so paying i128
unconditionally is a real cost rather than a free simplification.
I have added `native/spark-expr/benches/round.rs` (alongside the existing
`floor`/`ceil` benches) in 285a211bd so this is measurable rather than asserted
— happy to be overruled if you still prefer the single path, but I did not want
to take a 2-3x hit on the kernel silently.
--
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]
