andygrove commented on code in PR #4941:
URL: https://github.com/apache/datafusion-comet/pull/4941#discussion_r3693294796
##########
native/spark-expr/src/conversion_funcs/numeric.rs:
##########
@@ -430,69 +411,50 @@ macro_rules! cast_decimal_to_int16_down {
$rust_dest_type:ty,
$dest_type_str:expr,
$precision:expr,
- $scale:expr
+ $scale:expr,
+ $dest_arrow_type:ty
) => {{
let cast_array = $array
.as_any()
.downcast_ref::<Decimal128Array>()
.expect("Expected a Decimal128ArrayType");
- let output_array = match $eval_mode {
- EvalMode::Ansi => cast_array
- .iter()
- .map(|value| match value {
- Some(value) => {
- let divisor = 10_i128.pow($scale as u32);
- let truncated = value / divisor;
- let is_overflow = truncated.abs() > i32::MAX.into();
- if is_overflow {
- return Err(cast_overflow(
- &format!(
- "{}BD",
- format_decimal_str(
- &value.to_string(),
- $precision as usize,
- $scale
- )
- ),
- &format!("DECIMAL({},{})", $precision, $scale),
- $dest_type_str,
- ));
- }
- let i32_value = truncated as i32;
- <$rust_dest_type>::try_from(i32_value)
- .map_err(|_| {
- cast_overflow(
- &format!(
- "{}BD",
- format_decimal_str(
- &value.to_string(),
- $precision as usize,
- $scale
- )
- ),
- &format!("DECIMAL({},{})", $precision,
$scale),
- $dest_type_str,
- )
- })
- .map(Some)
- }
- None => Ok(None),
- })
- .collect::<Result<$dest_array_type, _>>()?,
- _ => cast_array
- .iter()
- .map(|value| match value {
- Some(value) => {
- let divisor = 10_i128.pow($scale as u32);
- let i32_value = (value / divisor) as i32;
- Ok::<Option<$rust_dest_type>, SparkError>(Some(
- i32_value as $rust_dest_type,
- ))
- }
- None => Ok(None),
+ // The scale divisor is constant across the batch, so hoist it out of
the per-element
+ // loop. `unary`/`try_unary` then map the values buffer in one pass,
carrying the null
+ // buffer over, instead of the per-element iterator-collect.
+ let divisor = 10_i128.pow($scale as u32);
+ let output_array: $dest_array_type = match $eval_mode {
+ EvalMode::Ansi => cast_array.try_unary::<_, $dest_arrow_type,
SparkError>(|value| {
+ let truncated = value / divisor;
Review Comment:
Confirmed and fixed in 6838967c6 — thanks, this was a real panic and I
reproduced every step of your reasoning.
`-1i8 as u32` is `4294967295`, and `10_i128.wrapping_pow(4294967295)` is
exactly `0` (10^k has a factor of 2^k, so for k >= 128 the wrapped value is 0).
So release wraps to a zero divisor and then divides by zero, and debug panics
in the `pow` itself.
End-to-end reproduction took a bit of setting up, which is worth recording:
a negative-scale decimal cannot be round-tripped through Parquet (`Invalid
DECIMAL scale: -4`) and the SQL parser rejects `DECIMAL(10,-4)` regardless of
`allowNegativeScaleOfDecimal`. So the reachable path is a negative-scale value
produced *mid-plan* by one native cast and consumed by the next:
```scala
reread.select(col("a").cast(DataTypes.createDecimalType(10,
-4)).cast(DataTypes.ByteType))
```
which gives:
```
org.apache.comet.CometNativeException: native panic: attempt to multiply
with overflow
(core/src/num/mod.rs:475 -> pow)
```
I went with your second option, falling back rather than implementing
scale-aware handling: `CometCast.canCastFromDecimal` now takes the source
`DecimalType` and reports negative-scale integral casts `Unsupported`,
following the negative-scale precedent already in `canCastToString`.
Multiplying correctly would need its own ANSI-overflow and wrap semantics
worked out against Spark, which does not belong in a vectorization PR. I gated
only the integral targets, since those are the ones with a reproduced panic.
Both `pow` sites now carry a comment pointing at the Scala guard so they do not
drift apart.
The regression test covers both shapes you asked about, including all-null.
One clarification on scope, since it affects how you read the fix: on `main`
the same `10_i128.pow(scale as u32)` sits *inside* the per-element closure, so
the panic already exists there for any non-null negative-scale value. What this
PR changes is specifically the **all-null** case — `unary` applies the op to
null slots, where the old iterator-collect ran it only for `Some` values — so
an all-null negative-scale column newly reaches the divisor. Your instinct to
ask for an all-null test was pointing straight at the part this PR actually
introduced.
--
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]
