andygrove commented on code in PR #6953:
URL: https://github.com/apache/datafusion-comet/pull/6953#discussion_r4272184108
##########
native/spark-expr/src/conversion_funcs/numeric.rs:
##########
@@ -540,62 +513,37 @@ macro_rules! cast_decimal_to_int16_down {
.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),
+ let divisor = 10_i128.pow($scale as u32);
+ // The legacy conversion cannot fail, so `unary` maps the values
buffer in one pass and
+ // keeps the null buffer. `try_unary` only visits valid slots, so an
out-of-range value
+ // under a null cannot raise a spurious overflow error.
+ let output_array: $dest_array_type = match $eval_mode {
+ EvalMode::Ansi => cast_array.try_unary(|value| {
+ 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,
+ )
})
- .collect::<Result<$dest_array_type, _>>()?,
+ })?,
+ _ => cast_array.unary(|value| (value / divisor) as i32 as
$rust_dest_type),
Review Comment:
The legacy decimal arms divide every slot here, including the ones under a
null, and the i128 division is most of the cost. I copied the old and new macro
bodies into a small harness (arrow 59.3, release, 8192 rows, `DECIMAL(38,2)` to
INT). This PR runs at about 18 µs a batch at every null density, against 44 to
70 µs before, so it's a clear win already. `try_unary` with a closure that
always returns `Ok` only visits valid slots, and it came in at 16.4 µs with no
nulls, 10.1 µs at 50% nulls and 0.4 µs for an all-null batch. Would it be worth
using it for the two legacy decimal arms? The float arms are fine with `unary`,
since the `as` cast is nearly free and vectorizes (1.9 µs at any density, down
from 17 to 31 µs).
##########
native/spark-expr/src/conversion_funcs/numeric.rs:
##########
@@ -617,46 +565,27 @@ macro_rules! cast_decimal_to_int32_up {
.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() >
$max_dest_val.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,
- ));
- }
- Ok(Some(truncated as $rust_dest_type))
- }
- 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 truncated = value / divisor;
- Ok::<Option<$rust_dest_type>, SparkError>(Some(
- truncated as $rust_dest_type,
- ))
- }
- None => Ok(None),
- })
- .collect::<Result<$dest_array_type, _>>()?,
+ let divisor = 10_i128.pow($scale as u32);
+ // The legacy conversion cannot fail, so `unary` maps the values
buffer in one pass and
+ // keeps the null buffer. `try_unary` only visits valid slots, so an
out-of-range value
+ // under a null cannot raise a spurious overflow error.
+ let output_array: $dest_array_type = match $eval_mode {
+ EvalMode::Ansi => cast_array.try_unary(|value| {
+ let truncated = value / divisor;
+ let is_overflow = truncated.abs() > $max_dest_val.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,
+ ));
+ }
+ Ok(truncated as $rust_dest_type)
+ })?,
+ _ => cast_array.unary(|value| (value / divisor) as
$rust_dest_type),
Review Comment:
I checked this end to end and it goes a bit further than the all-null case.
On main, a negative scale already panics as soon as a batch has a non-null
value. `col("_1").cast(DecimalType(10, -2)).cast(IntegerType)` over a Parquet
double column runs natively as a `CometProject` over `CometNativeScan` and
fails on a debug build with `attempt to multiply with overflow`, while Spark
returns 12300. That is the same family as #5013. What this PR adds is the
all-null batch. Main returns nulls there, like Spark, and with the PR it panics
too. On a debug build that happens in both eval modes. On a release build the
divisor wraps to zero, so LEGACY panics because `unary` divides the null slots
by it.
So a null-only fast path would only hide half of it. Could we keep negative
scales out of these macros instead? If `canCastFromDecimal` in
`CometCast.scala` returned `Unsupported` for a negative scale with an integral
target, the codegen dispatcher would run Spark's own cast, and the hoisted
divisor could never wrap. A test in `CometNativeCastSuite` with a
negative-scale column, one table with values and one that is all null, would
cover both.
--
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]
