mbutrovich commented on code in PR #4937:
URL: https://github.com/apache/datafusion-comet/pull/4937#discussion_r3591201082
##########
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
Review Comment:
On an ANSI batch that contains an overflow, the code now runs:
1. the fast-path `all(is_valid_decimal_precision)` scan (`:128-131`), which
returns `false`,
2. `validate_decimal_precision(*precision)` (`:140-141`), a full second scan
that finds the overflow and produces the error, and
3. inside the `map_err`, a third scan `decimal_array.iter().find(...)`
(`:146-158`) to locate the first offending value for the Spark error.
Scans 2 and 3 are redundant. The fast-path scan at `:128` already knows an
overflow exists, and `is_valid_decimal_precision` already tells you which value
is the first offender. You can drop `validate_decimal_precision` entirely and
build the error from a single `find`:
```rust
} else if self.fail_on_error {
// ANSI: fast path already proved an overflow exists. Find the first
offending
// value and raise the precise Spark error. Only runs on the aborting
error path.
let overflow_value = decimal_array
.iter()
.flatten()
.find(|v| !Decimal128Type::is_valid_decimal_precision(*v,
*precision))
.unwrap_or(0);
let spark_error =
crate::error::decimal_overflow_error(overflow_value, *precision,
*scale);
return Err(match &self.query_context {
Some(ctx) => DataFusionError::External(Box::new(
crate::SparkErrorWithContext::with_context(spark_error,
Arc::clone(ctx)),
)),
None => DataFusionError::External(Box::new(spark_error)),
});
}
```
This removes the whole `validate_decimal_precision` call, the
string-matching on `"too large to store in a Decimal128"` (which is brittle
against arrow-rs wording changes, and also fails to catch the `"too small"`
underflow branch at `arrow-data/src/decimal.rs:1160`), and the unreachable
`Internal` error at `:177-179`. The PR says the error path "aborts the query
anyway" so cost does not matter, but the simpler version is also more correct:
the current `find` at `:146-158` calls the three-arg
`Decimal128Type::validate_decimal_precision(val, precision, scale)` and matches
only the "too large" string, so a value that overflows on the negative side
reaches the `.unwrap_or(0)` fallback and reports value `0` in the error. Using
`is_valid_decimal_precision` in the `find` fixes that.
##########
native/spark-expr/src/math_funcs/internal/checkoverflow.rs:
##########
@@ -381,4 +402,79 @@ mod tests {
other => panic!("unexpected: {other:?}"),
}
}
+
+ // --- array path ---
+
+ fn array_batch(values: Vec<Option<i128>>, in_precision: u8, scale: i8) ->
RecordBatch {
Review Comment:
Every new array test uses a positive overflow value (`1000` against
precision 3). The `MIN_DECIMAL128_FOR_EACH_PRECISION` lower bound is never
exercised. This matters because the existing ANSI error-formatting path only
string-matches `"too large"` (see finding 1), so a negative overflow is a real
untested branch. Add a legacy case that nulls a large-negative value and an
ANSI case that errors on one:
```rust
#[test]
fn test_array_negative_overflow_nulled_legacy() {
let batch = array_batch(vec![Some(-1000), Some(5)], 38, 0);
let out = eval_array(&array_check_overflow(3, 0, false), &batch);
assert_eq!(out.iter().collect::<Vec<_>>(), vec![None, Some(5)]);
}
```
##########
native/spark-expr/src/math_funcs/internal/checkoverflow.rs:
##########
@@ -381,4 +402,79 @@ mod tests {
other => panic!("unexpected: {other:?}"),
}
}
+
+ // --- array path ---
+
+ fn array_batch(values: Vec<Option<i128>>, in_precision: u8, scale: i8) ->
RecordBatch {
+ let arr = values
+ .into_iter()
+ .collect::<Decimal128Array>()
+ .with_precision_and_scale(in_precision, scale)
+ .unwrap();
+ let schema = Schema::new(vec![Field::new("d", arr.data_type().clone(),
true)]);
+ RecordBatch::try_new(Arc::new(schema), vec![Arc::new(arr)]).unwrap()
+ }
+
+ fn array_check_overflow(target_precision: u8, scale: i8, fail_on_error:
bool) -> CheckOverflow {
Review Comment:
The review brief asks for all-overflow, all-null, and boundary-precision
coverage. Current tests cover no-overflow, single-overflow, and mixed-null, but
not:
- an all-null batch (the fast path takes `flatten().all(...)` which returns
`true` on an empty iterator, so all-null must reuse the input and preserve the
null mask - worth pinning),
- an all-overflow batch in legacy mode (every slot nulled) and ANSI mode
(errors),
- a boundary value that exactly equals `MAX_FOR_EACH_PRECISION[precision]`
(for example `Some(999)` at precision 3 is already the max and passes; add
`Some(9999)` at precision 3 to confirm it is treated as overflow, since
off-by-one on the bound is the classic failure).
These are cheap to add and directly guard the fast-path condition.
##########
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:
#4937 detects overflow with `all(is_valid_decimal_precision)` over the raw
input values. #4938 detects it with `result.values().contains(&i128::MAX)` over
a sentinel that `try_unary` already wrote. The two do not and cannot share a
helper, because in `DecimalRescaleCheckOverflow` the rescale pass has already
folded the overflow decision into the sentinel, so re-deriving it from the
original values would mean redoing the rescale. That is a defensible split, not
a flaw. What is missing is the shared decision both PRs encode: "run the
null-masking / error pass only when an overflow is actually present." If you
want reuse, extract the non-ANSI tail both files share:
```rust
// returns the input array unchanged when nothing overflows, else the
null-masked array
fn null_if_any_overflow(arr: &Decimal128Array, precision: u8, any_overflow:
bool) -> Decimal128Array {
if any_overflow { arr.null_if_overflow_precision(precision) } else {
arr.clone() }
}
```
This is optional given the different detection inputs, but I would rather
see one named concept than two ad hoc `if` shapes drifting apart over time. At
minimum, land both PRs together so a reviewer can see the pattern is
intentional.
--
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]
