HaoYang670 commented on code in PR #4234:
URL: https://github.com/apache/arrow-datafusion/pull/4234#discussion_r1024818973
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -760,6 +766,29 @@ impl<'a, S: SimplifyInfo> ExprRewriter for Simplifier<'a,
S> {
},
},
+ // unfold the expression round(source, n)
+ Expr::ScalarFunction {
+ fun: BuiltinScalarFunction::Round,
+ args,
Review Comment:
> This makes the logical plan harder to use for other query engines
Why is that? Could you please give an example?
> We can implement the functionality in the physical plan in a separate PR.
The reason that I don't implement in physical plan is that the
implementation isn't straightforward.
Rust only support the `round` function with one argument.
If we want to implement in physical plan, we have to assemble some arrow
compute kernel together, which the logic is same as what I've done in the
physical plan. However, the drawback is that, we can't optimize the expression
if the second function is a constant, which we have to add additional optimize
rules.
The disadvantages of implementing in logical plan are:
1. Put the implementation in the logical optimizer isn't a good solution, we
should unfold the expression before doing any optimization.
1. the query plan isn't user-friendly to read because `round` is unfolded.
But I think this is not a big deal, as the optimized code is always ugly to
read. Users can also use `explain verbose` to find what has happened.
--
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]
