andygrove commented on code in PR #4941:
URL: https://github.com/apache/datafusion-comet/pull/4941#discussion_r3603739076
##########
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:
Extended `test_cast_float64_to_int8_legacy_wraps` with `3e9`,
`f64::INFINITY`, and `f64::NEG_INFINITY`. `3e9`/`+inf` saturate to `i32::MAX`
(0x7FFFFFFF) and narrow to `-1` as i8; `-inf` saturates to `i32::MIN`
(0x80000000) and narrows to `0` as i8. Those asserts pin the
saturate-then-narrow chain end to end.
##########
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:
Added a one-line comment on each of the four legacy `unary` arms (`:334`,
`:373`, `:428`, `:473`) noting that `unary` runs the op on null slots too and
stating why the as-cast or positive-divisor division is infallible for any bit
pattern.
--
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]
