Jefffrey commented on code in PR #19831:
URL: https://github.com/apache/datafusion/pull/19831#discussion_r2698649935
##########
datafusion/functions/src/math/round.rs:
##########
@@ -141,6 +141,135 @@ impl ScalarUDFImpl for RoundFunc {
&default_decimal_places
};
+ // Scalar fast path for float and decimal types - avoid array
conversion overhead
Review Comment:
```rust
if let (ColumnarValue::Scalar(value_scalar),
ColumnarValue::Scalar(dp_scalar)) =
(&args.args[0], decimal_places)
{
if value_scalar.is_null() || dp_scalar.is_null() {
return ColumnarValue::Scalar(ScalarValue::Null)
.cast_to(args.return_type(), None);
}
let dp = if let ScalarValue::Int32(Some(dp)) = dp_scalar {
*dp
} else {
return internal_err!(
"Unexpected datatype for decimal_places: {}",
dp_scalar.data_type()
);
};
match value_scalar {
ScalarValue::Float32(Some(v)) => {
let rounded = round_float(*v, dp)?;
Ok(ColumnarValue::Scalar(ScalarValue::from(rounded)))
}
ScalarValue::Float64(Some(v)) => {
let rounded = round_float(*v, dp)?;
Ok(ColumnarValue::Scalar(ScalarValue::from(rounded)))
}
ScalarValue::Decimal128(Some(v), precision, scale) => {
let rounded = round_decimal(*v, *scale, dp)?;
let scalar =
ScalarValue::Decimal128(Some(rounded), *precision,
*scale);
Ok(ColumnarValue::Scalar(scalar))
}
ScalarValue::Decimal256(Some(v), precision, scale) => {
let rounded = round_decimal(*v, *scale, dp)?;
let scalar =
ScalarValue::Decimal256(Some(rounded), *precision,
*scale);
Ok(ColumnarValue::Scalar(scalar))
}
ScalarValue::Decimal64(Some(v), precision, scale) => {
let rounded = round_decimal(*v, *scale, dp)?;
let scalar =
ScalarValue::Decimal64(Some(rounded), *precision,
*scale);
Ok(ColumnarValue::Scalar(scalar))
}
ScalarValue::Decimal32(Some(v), precision, scale) => {
let rounded = round_decimal(*v, *scale, dp)?;
let scalar =
ScalarValue::Decimal32(Some(rounded), *precision,
*scale);
Ok(ColumnarValue::Scalar(scalar))
}
_ => {
internal_err!(
"Unexpected datatype for value: {}",
value_scalar.data_type()
)
}
}
} else {
round_columnar(&args.args[0], decimal_places, args.number_rows)
}
```
Cleaner way of doing this
- Using `internal_err` which are more appropriate here than `exec_err`
- Collapse null handling using
[`ScalarValue::is_null`](https://docs.rs/datafusion/latest/datafusion/scalar/enum.ScalarValue.html#method.is_null)
and
[`ColumnarValue::cast_to`](https://docs.rs/datafusion/latest/datafusion/logical_expr/enum.ColumnarValue.html#method.cast_to)
- Don't need to map the error of `round_float` and `round_decimal` because
using `?` does that for us
--
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]
