mbutrovich commented on code in PR #4941:
URL: https://github.com/apache/datafusion-comet/pull/4941#discussion_r3591287265
##########
native/spark-expr/src/conversion_funcs/numeric.rs:
##########
@@ -301,52 +301,37 @@ macro_rules! cast_float_to_int16_down {
$rust_dest_type:ty,
$src_type_str:expr,
$dest_type_str:expr,
+ $dest_arrow_type:ty,
$format_str:expr
) => {{
let cast_array = $array
.as_any()
.downcast_ref::<$src_array_type>()
.expect(concat!("Expected a ", stringify!($src_array_type)));
- let output_array = match $eval_mode {
- EvalMode::Ansi => cast_array
- .iter()
- .map(|value| match value {
- Some(value) => {
- let is_overflow = value.is_nan() || value.abs() as i32
== i32::MAX;
- if is_overflow {
- return Err(cast_overflow(
- &format!($format_str, value).replace("e", "E"),
- $src_type_str,
- $dest_type_str,
- ));
- }
- let i32_value = value as i32;
- <$rust_dest_type>::try_from(i32_value)
- .map_err(|_| {
- cast_overflow(
- &format!($format_str, value).replace("e",
"E"),
- $src_type_str,
- $dest_type_str,
- )
- })
- .map(Some)
- }
- None => Ok(None),
- })
- .collect::<Result<$dest_array_type, _>>()?,
- _ => cast_array
- .iter()
- .map(|value| match value {
- Some(value) => {
- let i32_value = value as i32;
- Ok::<Option<$rust_dest_type>, SparkError>(Some(
- i32_value as $rust_dest_type,
- ))
- }
- None => Ok(None),
+ // Spark casts float -> Byte/Short by going through Int first (with
its own overflow),
+ // then narrowing. `unary`/`try_unary` map the values buffer in one
pass and carry the
+ // null buffer over, replacing the per-element iterator-collect.
+ let output_array: $dest_array_type = match $eval_mode {
+ EvalMode::Ansi => cast_array.try_unary::<_, $dest_arrow_type,
SparkError>(|value| {
+ let is_overflow = value.is_nan() || value.abs() as i32 ==
i32::MAX;
+ if is_overflow {
+ return Err(cast_overflow(
+ &format!($format_str, value).replace("e", "E"),
+ $src_type_str,
+ $dest_type_str,
+ ));
+ }
+ let i32_value = value as i32;
+ <$rust_dest_type>::try_from(i32_value).map_err(|_| {
+ cast_overflow(
+ &format!($format_str, value).replace("e", "E"),
+ $src_type_str,
+ $dest_type_str,
+ )
})
- .collect::<Result<$dest_array_type, _>>()?,
+ })?,
+ _ => cast_array.unary::<_, $dest_arrow_type>(|value| (value as
i32) as $rust_dest_type),
Review Comment:
`native/spark-expr/src/conversion_funcs/numeric.rs:334` (float legacy arm
`cast_array.unary::<_, $dest_arrow_type>(|value| (value as i32) as
$rust_dest_type)`), `:373` (float32_up legacy), `:428` and `:473` (decimal
legacy arms with `value / divisor`).
`PrimitiveArray::unary` applies the op to every slot including nulls
(`arrow-array/src/array/primitive_array.rs:880-903`: "Applies the function for
all values, including those on null slots ... requires that the operation must
be infallible (not error/panic) for any value"). The old iterator-collect only
ran the body for `Some` values, so this PR widens the set of inputs the closure
sees to include whatever garbage sits in null slots.
I verified this is safe here. Float `value as i32` and `as $rust_dest_type`
are saturating `as` casts that never panic on any bit pattern including NaN.
Decimal `value / divisor` cannot panic because `divisor = 10^scale` is always
positive (the only `i128` division panic is `i128::MIN / -1`). So there is no
regression. The risk is a future edit adding a fallible op (a `checked_*`, an
index, an `unwrap`) into one of these closures without realizing it now runs on
null slots. The existing comments explain the Spark cast-through-Int semantics
but not this invariant. Suggested change: add one line to the legacy-arm
comment in each macro, e.g. `// unary runs op on null slots too; the as-cast /
positive-divisor division here is infallible for any bit pattern.`
##########
native/spark-expr/src/conversion_funcs/numeric.rs:
##########
@@ -1160,6 +1104,111 @@ mod tests {
assert_eq!(decimal_array.value(1), -10000); // -100 * 10^2
assert!(decimal_array.is_null(2));
}
+
Review Comment:
(`test_cast_float64_to_int8_legacy_wraps`) covers `300.7 -> 300 -> 44`,
which exercises the Byte narrowing wrap but keeps the intermediate inside i32
range. The distinctive Spark behavior these macros exist to replicate is the
double narrowing: float overflows i32 first, then that already-wrapped Int
narrows to Byte/Short. The legacy float path is `(value as i32) as
$rust_dest_type`, and `value as i32` saturates on overflow in Rust (`3e9_f64 as
i32 == i32::MAX`), so a value like `3e9` produces `i32::MAX` then narrows to
`-1` for i8. That saturate-then-narrow is exactly the fragile path. Suggested
change: add a case with `Some(3e9)` (or `Some(f64::INFINITY)`) to
`test_cast_float64_to_int8_legacy_wraps` and assert the wrapped byte, so the
legacy overflow-then-narrow behavior is pinned and cannot silently drift if the
`as i32` step is ever refactored.
--
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]
