perf: Optimize signum scalar performance with fast path#19871
perf: Optimize signum scalar performance with fast path#19871Jefffrey merged 4 commits intoapache:mainfrom
Conversation
| } | ||
|
|
||
| // Array path | ||
| make_scalar_function(signum, vec![])(&args.args) |
There was a problem hiding this comment.
this should have handled that optimization, no?
There was a problem hiding this comment.
If my interpretation is correct, you are asking: To add scalar optimization inside make_scalar_function? To do that we would need to change the signature to also accept a scalar function, which would be a larger refactor. If you meant that Doesn't make_scalar_function already handle scalar optimization? Then no we still need to convert scalars to arrays first. We have used the inline path in other parts of the optimization too.
There was a problem hiding this comment.
I think we can technically use make_scalar_function with the correct hints, but we might be trying to move away from that function, see:
| // Scalar fast path for float types - avoid array conversion overhead | ||
| if let ColumnarValue::Scalar(scalar) = arg { |
There was a problem hiding this comment.
We don't need to repeat this comment about fast paths each time (not to mention specifying it for "float types" is confusing considering the function signature already limits the inputs to float types). So it can actually be a bit misleading as it might imply we omit fast path for non-float types. We're better off removing the comment.
There was a problem hiding this comment.
Thanks for pointing that out.
| } | ||
| } | ||
|
|
||
| // Array path |
There was a problem hiding this comment.
We might as well change the if let to a match statement, and inline the contents of signum here to avoid use of make_scalar_function to simplify the code
Co-authored-by: Jeffrey Vo <jeffrey.vo.australia@gmail.com>
| }, | ||
| ), | ||
| ))), | ||
| other => exec_err!("Unsupported data type {other:?} for function signum"), |
There was a problem hiding this comment.
nit: this should be internal error to be consistent with scalar path above
There was a problem hiding this comment.
Can you provide a basic mental model for when I should use exec_err and when internal_err? Is there any documentation for this?
There was a problem hiding this comment.
exec err -> things that can happen in normal execution, such as invalid value to a function (e.g. trying to get ascii character from an integer input, and we input a value that doesnt have a corresponding character like 99999)
internal err -> things that shouldn't normally happen, aka occur if some other bug in datafusion allowed this code path to occur
in this case, the signature should already guard us to only have f32/f64 inputs; therefore if at this point we find an array not of that type, then something went wrong in type coercion/signature code and its an internal bug
|
Thanks @kumarUjjawal & @rluvaton |
## Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes apache#123` indicates that this PR will close issue apache#123. --> - Part of apache/datafusion-comet#2986 ## Rationale for this change The signum function currently converts scalar inputs to arrays before processing, even for single scalar values. This adds unnecessary overhead from array allocation and conversion. Adding a scalar fast path avoids this overhead and improves performance for constant folding and scalar expression evaluation. <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. --> ## What changes are included in this PR? - Added scalar fast path for `float32` and `float64` | Type | Before | After | Speedup | |------|--------|-------|---------| | **signum_f64_scalar** | 266 ns | 54 ns | **4.9x** | | **signum_f32_scalar** | 263 ns | 55 ns | **4.8x** | <!-- There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR. --> ## Are these changes tested? Yes <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code If tests are not included in your PR, please explain why (for example, are they covered by existing tests)? --> ## Are there any user-facing changes? No <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. --> <!-- If there are any breaking changes to public APIs, please add the `api change` label. --> --------- Co-authored-by: Jeffrey Vo <jeffrey.vo.australia@gmail.com>
3 participants
Which issue does this PR close?
Rationale for this change
The signum function currently converts scalar inputs to arrays before processing, even for single scalar values. This adds unnecessary overhead from array allocation and conversion. Adding a scalar fast path avoids this overhead and improves performance for constant folding and scalar expression evaluation.
What changes are included in this PR?
float32andfloat64Are these changes tested?
Yes
Are there any user-facing changes?
No