-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Refactor power() signature away from user defined
#18968
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
Conversation
| // Null propagation | ||
| if base_type.is_null() || exponent_type.is_null() { | ||
| let return_type = self.return_type(&[base_type, exponent_type])?; | ||
| return Ok(ExprSimplifyResult::Simplified(lit( | ||
| ScalarValue::Null.cast_to(&return_type)? | ||
| ))); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to what we do for log:
datafusion/datafusion/functions/src/math/log.rs
Lines 278 to 283 in c6f7363
| // Null propagation | |
| if arg_types.iter().any(|dt| dt.is_null()) { | |
| return Ok(ExprSimplifyResult::Simplified(lit( | |
| ScalarValue::Null.cast_to(&return_type)? | |
| ))); | |
| } |
| } | ||
|
|
||
| #[test] | ||
| fn test_power_f64() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved these to SLTs
alamb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @Jefffrey -- this looks great to me
| fn invoke_with_args(&self, args: ScalarFunctionArgs) -> Result<ColumnarValue> { | ||
| let base = &args.args[0].to_array(args.number_rows)?; | ||
| let exponent = &args.args[1]; | ||
| let [base, exponent] = take_function_args(self.name(), &args.args)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Which issue does this PR close?
Signature::user_definedfor normal functions #12725Rationale for this change
Prefer to avoid user_defined for consistency in function definitions.
What changes are included in this PR?
Refactor signature of power away from user_defined.
Are these changes tested?
Existing tests.
Are there any user-facing changes?
No.