andygrove commented on PR #5082: URL: https://github.com/apache/datafusion-comet/pull/5082#issuecomment-5146124038
@mbutrovich thanks for the review — all nine inline comments plus the `CometExpressionSuite` one are addressed in 285a211bd, with replies inline on each thread. Summary: - **Null scalar at `scale <= -39`** returned `Some(0)` instead of `None`. Real (pre-existing) bug, fixed — but by hoisting the null check out in front of the band dispatch rather than patching the third branch, so all three bands preserve null by construction. This also collapsed the duplicated unwrap boilerplate you flagged separately. - **`integer_round_widened!` sign arms** collapsed to one; **`(-point) as u32`** hoisted into `point_abs`. - **Unifying `integer_round!` on i128** — I implemented it and am pushing back with benchmarks: it costs **2.3-3.3x** on these kernels, because the native band gains a 128-bit division libcall and the widened band, which today does no division at all, gains a 128-bit modulo by ~1e19. Details and the table are on that thread. I did take the substance of the comment: the load-bearing invariant is now stated precisely *and* enforced with a `debug_assert!`, and I added `native/spark-expr/benches/round.rs` so the tradeoff is measurable rather than asserted. Happy to be overruled if you still prefer the single path. - **`WRAPPED_POS_1E19`** renamed to `WRAPPED_1E19`, with a comment noting the wrapped value is negative. - **Test gaps** closed: ANSI overflow for -5e18, a `scale = -40` test for the doesn't-fit-i128 band (array + scalar, both modes), and a null-scalar test across all three bands. On the `CometExpressionSuite` comment — you were right, and the root cause is worse than the scale list. That test builds an 8-column table but only ever queries `_1`, which is `Int.MaxValue`, so it never rounds a **long** at any scale; the long columns `_3`/`_4` are dead. No scale added to that list could have caught #5070. So I added a dedicated test that rounds longs (`Long.MaxValue`, `Long.MinValue`, ±5e18, 4999999999999999999, 0) at scales -39, -38, -20, -19 and -18 under both ANSI settings, which brackets both band boundaries — note -18 overflows too, since `Long.MaxValue` rounds up to 1e19 there, so this covers the native path as well as the widened one. I also added -19 and -20 to the existing test's scale sequence as you suggested. Verified: 11 Rust unit tests pass; both Scala tests pass against real Spark on **Spark 3.5** and **Spark 4.1**. `cargo clippy --all-targets` clean, `cargo fmt` and `spotless:apply` applied. -- 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]
