peterxcli commented on code in PR #4941:
URL: https://github.com/apache/datafusion-comet/pull/4941#discussion_r3657679429
##########
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:
The hoisted divisor assumes `scale >= 0`, but Arrow and Comet permit
negative-scale `Decimal128` values. Casting a negative `i8` scale to `u32`
makes `pow` overflow; in release it can produce a zero divisor, after which
Legacy `unary` divides null-slot backing values by zero. Could we either
implement scale-aware negative-scale handling or fall back these casts to
Spark, and add an all-null negative-scale regression test?
--
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]
