alamb commented on code in PR #9304:
URL: https://github.com/apache/arrow-datafusion/pull/9304#discussion_r1515004035
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -1264,6 +1303,19 @@ impl<'a, S: SimplifyInfo> TreeNodeRewriter for
Simplifier<'a, S> {
out_expr.rewrite(self)?
}
+ Expr::ScalarFunction(ScalarFunction {
+ func_def: ScalarFunctionDefinition::UDF(udf),
+ args,
+ }) => match udf.simplify(args, info)? {
+ ExprSimplifyResult::Original(args) => {
+ Transformed::yes(Expr::ScalarFunction(ScalarFunction {
+ func_def: ScalarFunctionDefinition::UDF(udf),
+ args,
+ }))
+ }
+ ExprSimplifyResult::Simplified(expr) => Transformed::no(expr),
+ },
Review Comment:
I think you are correct that the logic is inverted. I wil make a PR to
correct it.
> I did a test where I switched the yes -> no, no -> yes and cargo test
passes which makes me wonder if this code is indeed getting tested properly 🤔
It is a good question about why no tests fail if Transformed::no
I think it is because the expr simplifer code doens't do anything different
if the expression was transformed or not:
https://github.com/apache/arrow-datafusion/blob/f3836a53122e86f2b73c25557deaa5b800e488e9/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs#L188-L189
It just looks at `data`:
https://github.com/apache/arrow-datafusion/blob/f3836a53122e86f2b73c25557deaa5b800e488e9/datafusion/common/src/tree_node.rs#L456
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
