Jefffrey commented on code in PR #23215:
URL: https://github.com/apache/datafusion/pull/23215#discussion_r3596419386
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -2355,6 +2360,60 @@ fn simplify_right_is_one_case(
}
}
+fn reassociate_literals(expr: Expr, info: &SimplifyContext) -> Expr {
+ fn flatten(
+ op: Operator,
+ datatype: &DataType,
+ info: &SimplifyContext,
+ expr: Expr,
+ out: &mut Vec<Expr>,
+ ) {
+ match &expr {
+ Expr::BinaryExpr(binary)
+ if binary.op == op
+ && matches!(info.get_data_type(&expr), Ok(dt) if &dt ==
datatype) =>
+ {
+ let Expr::BinaryExpr(binary) = expr else {
+ unreachable!()
+ };
+ flatten(op, datatype, info, *binary.left, out);
+ flatten(op, datatype, info, *binary.right, out);
+ }
+ _ => out.push(expr),
+ }
Review Comment:
```suggestion
match expr {
Expr::BinaryExpr(binary)
if binary.op == op
&& matches!(info.get_data_type(&expr), Ok(dt) if &dt ==
datatype) =>
{
flatten(op, datatype, info, *binary.left, out);
flatten(op, datatype, info, *binary.right, out);
}
_ => out.push(expr),
}
```
minor refactor to avoid the duplicate unstructuring
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -1095,6 +1095,11 @@ impl TreeNodeRewriter for Simplifier<'_> {
Transformed::yes(*right)
}
+ // (A + 1) + 2 -> A + (1 + 2)
+ expr if is_associative_with_adjacent_literals(&expr, self.info) =>
{
Review Comment:
i do worry about this guard; it seems in the worst case, with Simplifier
already doing a traversal (up) of the expr tree, this guard then does another
traversal (down) of the tree, repeatedly as the simplifier goes up 🤔
ill see if benchmarks show this as an actual impact
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
