Re: [PR] perf: track decimal overflow without rescanning results [datafusion-comet]

["andygrove (via GitHub)" <gi...@apache.org>]

andygrove commented on code in PR #5044:
URL: https://github.com/apache/datafusion-comet/pull/5044#discussion_r3728643269


##########
native/spark-expr/src/math_funcs/wide_decimal_binary_expr.rs:
##########
@@ -329,20 +331,22 @@ impl PhysicalExpr for WideDecimalBinaryExpr {
 }
 
 /// Check if the i256 result fits in the output precision. In Ansi mode, return an error
-/// on overflow. In Legacy/Try mode, return i128::MAX as a sentinel value that will be
-/// nullified by `null_if_overflow_precision`.
+/// on overflow. In Legacy/Try mode, record the overflow and return i128::MAX as a sentinel
+/// value that will be nullified by `null_if_overflow_precision`.
 #[inline]
 fn check_overflow_and_convert(
     result: i256,
     bound: i256,
     neg_bound: i256,
     eval_mode: EvalMode,
+    overflowed: &Cell<bool>,
 ) -> Result<i128, ArrowError> {
     if result > bound || result < neg_bound {
         if eval_mode == EvalMode::Ansi {
             return Err(ArrowError::ComputeError("Arithmetic overflow".to_string()));
         }
         // Sentinel value — will be nullified by null_if_overflow_precision
+        overflowed.set(true);

Review Comment:
   Now that the flag records the overflow, the `i128::MAX` sentinel and the masking pass it feeds are both redundant. Would you be up for filing a follow-up issue to drop them and write the null bit directly at the point the overflow is detected? That would remove the extra pass and the `Cell` side effect, which is what is costing the 3% to 5% on the overflow shapes.
   
   Clearly out of scope here. I would just like it tracked rather than left implicit, and a link to the issue in this thread would be enough.



##########
native/spark-expr/src/math_funcs/wide_decimal_binary_expr.rs:
##########
@@ -276,12 +278,12 @@ impl PhysicalExpr for WideDecimalBinaryExpr {
                     } else {
                         raw
                     };
-                    check_overflow_and_convert(result, bound, neg_bound, eval_mode)
+                    check_overflow_and_convert(result, bound, neg_bound, eval_mode, &overflowed)
                 })?
             }
         };
 
-        let result = if eval_mode != EvalMode::Ansi {
+        let result = if overflowed.get() {

Review Comment:
   This dropped the only comment in the codebase explaining why skipping the masking pass is safe. The six lines removed from `decimal_rescale_check.rs` are gone, and neither guard has a replacement, so both call sites are now a bare `if overflowed.get()`.
   
   The invariant is not obvious from reading the guard. It rests on the fact that every non-sentinel value the closure returns is already clamped inside `±(10^p_out - 1)`, which is exactly the set `null_if_overflow_precision` leaves untouched, so the pass can only ever null sentinels. It is also worth saying that ANSI mode returns `Err` before setting the flag, so ANSI still skips the pass for the same reason it did before.
   
   Could we add that back at both sites, here and at `decimal_rescale_check.rs:212`? We worked the argument out in this thread, and it would be good for it to live in the code rather than only in the PR conversation.



-- 
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