andygrove commented on PR #5044: URL: https://github.com/apache/datafusion-comet/pull/5044#issuecomment-5441467975
> **Note on this review:** this was generated by an LLM (Claude Code) at my request while I worked through a review backlog. I have not verified the individual findings myself. Please treat everything below as suggestions to evaluate rather than as authoritative review feedback, and push back on anything that is wrong or already handled. The idea is right: the expression already knows whether anything overflowed, so scanning the output afterwards is redundant work. Recording it in a `Cell` during evaluation and skipping `null_if_overflow_precision` entirely on clean batches is a clean way to get there, and I convinced myself it is safe because `check_overflow_and_convert` bounds by exactly the same `max_for_precision(p_out)` that the masking pass uses, so an unmasked value can never need masking. The new test with a null in the middle of an overflowing batch is a good one to have. Three things. **The description does not match the diff** "Apply the same approach to `DecimalRescaleCheckOverflow` unary evaluation" is listed under what changes are included, but `native/spark-expr/src/math_funcs/internal/decimal_rescale_check.rs` is not in the PR. The only two files changed are the benchmark and `wide_decimal_binary_expr.rs`. Did that part get lost in a rebase, or was it intentionally dropped? Either is fine, but the description should say what actually landed, and if `DecimalRescaleCheckOverflow` still rescans the output buffer then the same win is still on the table. **The overflow-path regression** Sparse overflow is 1.6% slower and dense overflow 5.0% slower. That is a fine trade against 10 to 13% on the clean path, and I would take it. But it is worth saying explicitly in the description that the regression is accepted and why, rather than leaving the table to speak for itself. Is #5309, writing the null bit directly, expected to recover both, or just the masking pass? **Where the extra cost comes from** The `overflowed.set(true)` store happens on every overflowing row, not just the first. For the dense case that is 4096 redundant stores. I would not add a `if !overflowed.get()` guard, since the branch is probably worse than the store. But is the 5% actually coming from the stores, or from the `Cell` inhibiting some optimization inside the `try_binary` closure? If you happen to know, a one-line comment would save the next person from guessing. If #5309 removes the sentinel entirely then it does not matter much. **One benchmark question** `make_overflow_batch` builds `right` as all zeros, so `Add` reduces to passing `left` through and the overflow is determined entirely by whether `left` exceeds the output precision. That is fine for measuring the masking pass, but it means the arithmetic itself is the cheapest possible case. Would a benchmark with two non-trivial operands, where the addition itself is doing real i256 work, change the ratio? If the arithmetic dominates, the percentages above would all shrink, which would be worth knowing. -- 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]
