comphead commented on code in PR #6953:
URL: https://github.com/apache/datafusion-comet/pull/6953#discussion_r4274983783
##########
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:
Thanks, this is fixed in 0ab5db9138. I followed andygrove's suggestion below
instead of a null-only fast path. A negative scale already panics on main
(debug build) as soon as a batch has a non-null value, so the fast path would
only have covered the all-null half.
`CometCast.canCastFromDecimal` now returns `Unsupported` for a
negative-scale decimal with a `ByteType`, `ShortType`, `IntegerType` or
`LongType` target in every eval mode. Spark's own cast runs through the codegen
dispatcher, so the hoisted divisor is never computed for such a column. The
cast docs have a short section on it.
The new test in `CometNativeCastSuite` casts a negative-scale column twice,
once with values and once all null, to the four integral targets. It also
asserts the `Unsupported` result for the three eval modes. One thing I ran into
while writing it: Spark's `ConvertToLocalRelation` folds
`Project(LocalRelation)` on the driver, so by default the query never reaches
the Comet cast and the answer check cannot fail. The test excludes that rule.
With the rule excluded and the guard removed, it fails with the native panic
`attempt to multiply with overflow`.
Locally I ran clippy, the 116 `conversion_funcs` unit tests, and this test
together with the existing negative-scale cast test. I only reproduced the
debug build panic. I did not run a release build, so the divide by zero in your
report is not re-checked here. The guard keeps the cast out of the native
kernels, so it should not depend on the build profile.
##########
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:
Done in 0ab5db9138. The two legacy decimal arms now use `try_unary` with a
closure that always returns `Ok`, and a comment says why. The float arms stay
on `unary`.
My numbers for `decimal(18,2)` to `i32` at 8192 rows (criterion, M3 Max,
bench profile): with no nulls it is 47.5 µs on main, 18.6 µs with `unary` and
16.2 µs with `try_unary`. At 10% nulls it is 58.5 µs on main and 18.4 µs with
either, so I see no difference there. I did not measure 50% or all-null
batches, so I cannot confirm your 10.1 µs and 0.4 µs figures.
--
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]
