andygrove commented on code in PR #4937:
URL: https://github.com/apache/datafusion-comet/pull/4937#discussion_r3591296105
##########
native/spark-expr/src/math_funcs/internal/checkoverflow.rs:
##########
@@ -119,8 +119,24 @@ impl PhysicalExpr for CheckOverflow {
let decimal_array =
as_primitive_array::<Decimal128Type>(&array);
- let casted_array = if self.fail_on_error {
- // Returning error if overflow - convert decimal overflow
to SparkError
+ // Fast path shared by both ANSI and non-ANSI:
`is_valid_decimal_precision` is a
+ // small, inlined bounds check and `all` short-circuits at the
first overflow. When
+ // nothing overflows (the common shape for decimal arithmetic
in TPC-DS) we reuse the
+ // input buffers via `to_data()`, which only clones cheap Arc
metadata. This avoids
+ // the heavier per-value `validate_decimal_precision` scan
(ANSI) or the allocating
+ // `null_if_overflow_precision` (non-ANSI) below.
+ let no_overflow = decimal_array
+ .iter()
+ .flatten()
+ .all(|v| Decimal128Type::is_valid_decimal_precision(v,
*precision));
Review Comment:
Leaving this one as-is. As you note, the detection inputs differ (this PR
scans the raw input values; #4938 reads a post-rescale sentinel), so a shared
helper would only wrap the tail `if any_overflow { ... }`, which is a single
line in each file. I would rather not couple the two PRs around a one-line
helper. Happy to land them together so the pattern is visible in review, and if
#4938 goes in first I can rebase on top and extract the shared tail then if it
still reads as one concept.
--
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]
