peterxcli commented on PR #5044: URL: https://github.com/apache/datafusion-comet/pull/5044#issuecomment-5443588878
@andygrove Thanks for the pass — responses point by point. **Description vs. diff:** you're right, and the history is: the `DecimalRescaleCheckOverflow` half of this PR was superseded on main. #5440 (closing #5094) replaced that expression's custom kernel with a plain Arrow `cast_to`, so the sentinel, the scan, and the masking pass no longer exist there — the win isn't still on the table, it was collected a different way. When I merged upstream/main, this PR's rescale changes correctly dropped out of the diff. I've updated the description to reflect what actually remains and moved the old rescale benchmark table under a note explaining it measured code that #5440 has since removed. **Regression wording:** the description already states the 3–5% overflow-path regression is an accepted trade-off (added after your Aug 6 review). On your question: #5309 is expected to recover both costs — writing validity bits directly during evaluation removes the masking pass *and* the `Cell`, so no observable side effect remains in the closure. **Cost attribution:** your Aug 6 measurement already answered this — `overflow at end of batch` has exactly one overflowing row yet regresses 3.7%, so it can't be the stores; it's the `Cell` write being an observable side effect that inhibits optimization of the kernel loop. I've added a comment saying exactly that at the `Cell` creation site, with a pointer to #5309. **Zero right operand:** the arithmetic isn't actually cheaper for zeros — `i256::wrapping_mul`/`wrapping_add` do identical work regardless of operand values, and the scale factors are runtime values the compiler can't fold, so every row pays the full add-shape cost. What is true is that Add-with-equal-scales is the lightest op (no rescale division), so these shapes deliberately maximize the guard's relative visibility — the reported 3–5% is an upper bound, and heavier arithmetic (multiply + scale-down) would only shrink it. Happy to add a multiply-based overflow shape if you'd like that quantified, though #5309 should make the question moot. I also restored the safety-invariant comment from your Aug 6 review at the surviving guard site — it had only been added to `decimal_rescale_check.rs`, whose version was deleted by the #5440 merge. -- 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]
