Omega359 commented on code in PR #9304:
URL: https://github.com/apache/arrow-datafusion/pull/9304#discussion_r1513764598
##########
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:
Sorry, I'm a bit late to the party on this one but I keep rereading this and
I can't help but think the logic is inverted - Original means unchanged which
should result in Transformed::no, shouldn't it? And Simplified mean changed
which should result in Transformed::yes?
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 🤔
Or am I completely missing something (very likely)?
--
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]
