move the Log, Power functions to datafusion-functions #9983
Conversation
github-actions
bot
added
logical-expr
datafusion/functions/src/math/log.rs
Outdated
| Expr::ScalarFunction(ScalarFunction { | ||
| func_def: ScalarFunctionDefinition::UDF(fun), | ||
| args, | ||
| }) if base == &args[0] && fun.name() == "power" => { |
There was a problem hiding this comment.
[Need help]
I wasn't able to figure out how to match the exact PowerFunc type here. Same for a rule in power.rs.
There was a problem hiding this comment.
I think you can do something like
let is_power = fun.as_any().downcast_ref::<PowerUDF>().is_some();If that doesn't work I can try and look more into it on Monday
There was a problem hiding this comment.
This worked with a slight modification. Thank you!
fun.as_ref().inner().as_any().downcast_ref::<PowerFunc>().is_some()
There was a problem hiding this comment.
I took a look over the normal things that catch me (monotonicity, aliases, tests) and nothing stood out - LGTM
tinfoil-knight
force-pushed
the
9875-power-log
branch
from
864245a to
fec86a1
Compare
tinfoil-knight
force-pushed
the
9875-power-log
branch
from
fec86a1 to
1b2a852
Compare
tinfoil-knight
force-pushed
the
9875-power-log
branch
from
1b2a852 to
7d7b28b
Compare
|
I noticed a few things while reviewing the code for merge conflicts and force pushed the commit a couple of times. Sorry for the partial CI re-runs. |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you so much @tinfoil-knight and @Omega359 for the review. This is a really nice piece of work and your porting of the simplify logic is quite nice 👌 .
I have a suggestion on how to avoid clone'ing in the simplify logic for your consideration: tinfoil-knight#1 (targets this PR).
However, I think we can also merge this PR as is and then rework the cloning as a follow on.
| .downcast_ref::<PowerFunc>() | ||
| .is_some() => | ||
| { | ||
| Ok(ExprSimplifyResult::Simplified(args[1].clone())) |
There was a problem hiding this comment.
It would be really nice to avoid this clone (I am working to reduce the number of times we need to clone Exprs during planning). see tinfoil-knight#1 for a suggestion
No worries -- thanks for your efforts |
3 participants
Which issue does this PR close?
Closes #9875.
Rationale for this change
As part of #9285,
log&powerfunctions should be migrated to the datafusion-functions crate.What changes are included in this PR?
Log&Powerfrom built-in functions & changes to scalar UDFs.log&powerwere inter-dependent & therefore were migrated together along with tests.Are these changes tested?
Tested by existing tests which were migrated (and slightly modified).
Are there any user-facing changes?
No.