Jefffrey commented on code in PR #19831:
URL: https://github.com/apache/datafusion/pull/19831#discussion_r2697247852
##########
datafusion/functions/src/math/round.rs:
##########
@@ -141,6 +141,50 @@ impl ScalarUDFImpl for RoundFunc {
&default_decimal_places
};
+ // Scalar fast path for float types - avoid array conversion overhead
+ if let (ColumnarValue::Scalar(value_scalar),
ColumnarValue::Scalar(dp_scalar)) =
+ (&args.args[0], decimal_places)
+ {
+ // Extract decimal places as i32
+ let dp = match dp_scalar {
+ ScalarValue::Int32(Some(dp)) => *dp,
+ ScalarValue::Int32(None) => {
+ return
Ok(ColumnarValue::Scalar(ScalarValue::Float64(None)));
+ }
+ _ => {
+ // Fall through to array path for non-Int32 decimal places
+ return round_columnar(
+ &args.args[0],
+ decimal_places,
+ args.number_rows,
+ );
+ }
+ };
+
+ match value_scalar {
+ ScalarValue::Float64(Some(v)) => {
+ let factor = 10_f64.powi(dp);
+ return Ok(ColumnarValue::Scalar(ScalarValue::Float64(Some(
+ (v * factor).round() / factor,
+ ))));
+ }
+ ScalarValue::Float64(None) => {
+ return
Ok(ColumnarValue::Scalar(ScalarValue::Float64(None)));
+ }
+ ScalarValue::Float32(Some(v)) => {
+ let factor = 10_f32.powi(dp);
+ return Ok(ColumnarValue::Scalar(ScalarValue::Float32(Some(
+ (v * factor).round() / factor,
+ ))));
+ }
+ ScalarValue::Float32(None) => {
+ return
Ok(ColumnarValue::Scalar(ScalarValue::Float32(None)));
+ }
+ // For decimals and other types: fall through to array path
Review Comment:
We should do it in this PR
##########
datafusion/functions/src/math/round.rs:
##########
@@ -141,6 +141,58 @@ impl ScalarUDFImpl for RoundFunc {
&default_decimal_places
};
+ // Scalar fast path for float types - avoid array conversion overhead
+ if let (ColumnarValue::Scalar(value_scalar),
ColumnarValue::Scalar(dp_scalar)) =
+ (&args.args[0], decimal_places)
+ {
+ // Extract decimal places as i32
+ // Note: decimal_places is coerced to Int32 by the signature, so
the non-Int32
+ // arm should be unreachable in normal execution. We fall through
to the array
+ // path as a safety measure.
+ let dp = match dp_scalar {
+ ScalarValue::Int32(Some(dp)) => *dp,
+ ScalarValue::Int32(None) => {
+ // Return type depends on input type, but for null dp we
return null
Review Comment:
This fails to take into account decimals
##########
datafusion/functions/src/math/round.rs:
##########
@@ -141,6 +141,50 @@ impl ScalarUDFImpl for RoundFunc {
&default_decimal_places
};
+ // Scalar fast path for float types - avoid array conversion overhead
+ if let (ColumnarValue::Scalar(value_scalar),
ColumnarValue::Scalar(dp_scalar)) =
+ (&args.args[0], decimal_places)
+ {
+ // Extract decimal places as i32
+ let dp = match dp_scalar {
+ ScalarValue::Int32(Some(dp)) => *dp,
+ ScalarValue::Int32(None) => {
+ return
Ok(ColumnarValue::Scalar(ScalarValue::Float64(None)));
+ }
+ _ => {
+ // Fall through to array path for non-Int32 decimal places
Review Comment:
If it is unreachable in normal execution then it should return an internal
error to indicate so
--
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]
