-
Notifications
You must be signed in to change notification settings - Fork 2.1k
perf: Optimize signum scalar performance with fast path #19871
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,7 +22,7 @@ use arrow::array::{ArrayRef, AsArray}; | |
| use arrow::datatypes::DataType::{Float32, Float64}; | ||
| use arrow::datatypes::{DataType, Float32Type, Float64Type}; | ||
|
|
||
| use datafusion_common::{Result, exec_err}; | ||
| use datafusion_common::{Result, ScalarValue, exec_err, internal_err}; | ||
| use datafusion_expr::sort_properties::{ExprProperties, SortProperties}; | ||
| use datafusion_expr::{ | ||
| ColumnarValue, Documentation, ScalarFunctionArgs, ScalarUDFImpl, Signature, | ||
|
|
@@ -98,6 +98,34 @@ impl ScalarUDFImpl for SignumFunc { | |
| } | ||
|
|
||
| fn invoke_with_args(&self, args: ScalarFunctionArgs) -> Result<ColumnarValue> { | ||
| let arg = &args.args[0]; | ||
|
|
||
| // Scalar fast path for float types - avoid array conversion overhead | ||
| if let ColumnarValue::Scalar(scalar) = arg { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for pointing that out. |
||
| if scalar.is_null() { | ||
| return ColumnarValue::Scalar(ScalarValue::Null) | ||
| .cast_to(args.return_type(), None); | ||
| } | ||
|
|
||
| match scalar { | ||
| ScalarValue::Float64(Some(v)) => { | ||
| let result = if *v == 0.0 { 0.0 } else { v.signum() }; | ||
| return Ok(ColumnarValue::Scalar(ScalarValue::Float64(Some(result)))); | ||
| } | ||
| ScalarValue::Float32(Some(v)) => { | ||
| let result = if *v == 0.0 { 0.0 } else { v.signum() }; | ||
| return Ok(ColumnarValue::Scalar(ScalarValue::Float32(Some(result)))); | ||
| } | ||
| _ => { | ||
| return internal_err!( | ||
| "Unexpected scalar type for signum: {:?}", | ||
| scalar.data_type() | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Array path | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We might as well change the |
||
| make_scalar_function(signum, vec![])(&args.args) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this should have handled that optimization, no?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can technically use |
||
| } | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.