perf: Optimize round scalar performance #19831
Conversation
| return Ok(ColumnarValue::Scalar(ScalarValue::Float64(None))); | ||
| } | ||
| _ => { | ||
| // Fall through to array path for non-Int32 decimal places |
There was a problem hiding this comment.
Is this arm possible in normal execution?
There was a problem hiding this comment.
No, it's unreachable because decimal_places is coerced to Int32 by the signature.
There was a problem hiding this comment.
If it is unreachable in normal execution then it should return an internal error to indicate so
| ScalarValue::Float32(None) => { | ||
| return Ok(ColumnarValue::Scalar(ScalarValue::Float32(None))); | ||
| } | ||
| // For decimals and other types: fall through to array path |
There was a problem hiding this comment.
Is there anything stopping us from supporting decimals as well?
There was a problem hiding this comment.
The round_decimal function requires the scale parameter from the decimal type (e.g., Decimal128(precision, scale)), which makes extracting and processing scalars more involved than for floats.
For float scalars, we just call round_float(*v, dp) directly. For decimals, we'd need to:
- Extract the native value from ScalarValue::Decimal128 (or other decimal variants)
- Get the scale from the type
- Call round_decimal(value, scale, dp)
- Reconstruct the ScalarValue with precision/scale
If you're comfortable with these changes in this PR for the decimal then I can introduce these as well. What do you think?
There was a problem hiding this comment.
We should do it in this PR
| 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 |
There was a problem hiding this comment.
This fails to take into account decimals
| ScalarValue::Float32(None) => { | ||
| return Ok(ColumnarValue::Scalar(ScalarValue::Float32(None))); | ||
| } | ||
| // For decimals and other types: fall through to array path |
There was a problem hiding this comment.
We should do it in this PR
| return Ok(ColumnarValue::Scalar(ScalarValue::Float64(None))); | ||
| } | ||
| _ => { | ||
| // Fall through to array path for non-Int32 decimal places |
There was a problem hiding this comment.
If it is unreachable in normal execution then it should return an internal error to indicate so
2 participants
Which issue does this PR close?
Rationale for this change
The round function currently converts scalar inputs to arrays before processing, even when both value and decimal_places are scalar values. This adds unnecessary overhead for constant folding scenarios like
What changes are included in this PR?
Are these changes tested?
Yes
Are there any user-facing changes?
No