andygrove commented on code in PR #4937:
URL: https://github.com/apache/datafusion-comet/pull/4937#discussion_r3591294863
##########
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:
Done in 65bfa3383. Dropped `validate_decimal_precision`, the `"too large"`
string-match, and the unreachable `Internal` error; the ANSI branch now builds
the Spark error from a single `find` over `is_valid_decimal_precision`.
Confirmed the negative-overflow bug against arrow-data 58.3.0:
`validate_decimal_precision` emits `"too small to store"` for a value below
`MIN_DECIMAL128_FOR_EACH_PRECISION`, so the old outer match on `"too large"`
fell through to `DataFusionError::ArrowError` and never produced the Spark
error (and the inner `find` reported `0`). `is_valid_decimal_precision` checks
both bounds, so this is now correct for underflow too, covered by a new test.
##########
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:
Added in 65bfa3383: `test_array_negative_overflow_nulled_legacy` (nulls
`-1000` at precision 3, legacy) and `test_array_negative_overflow_ansi_errors`
(raises on a negative overflow in ANSI). The second one guards the underflow
branch that the old `"too large"` match missed.
##########
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:
Added in 65bfa3383:
- `test_array_all_null_reuses_input_and_preserves_mask` (pins that
`flatten().all(...)` returns true on all-null and the null mask survives),
- `test_array_all_overflow_nulled_legacy` and
`test_array_all_overflow_ansi_errors`,
- `test_array_boundary_precision_max_passes_and_over_by_one_overflows`
(`999` passes, `9999` overflows at precision 3).
--
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]
