Jefffrey commented on code in PR #8780:
URL: https://github.com/apache/arrow-datafusion/pull/8780#discussion_r1448501687
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -224,6 +226,52 @@ impl<S: SimplifyInfo> ExprSimplifier<S> {
}
}
+/// Canonicalize any BinaryExprs that are not in canonical form
+///
+/// `<literal> <op> <col>` is rewritten to `<col> <op> <literal>`
+///
+/// `<col1> <op> <col2>` is rewritten so that the name of `col1` sorts higher
+/// than `col2` (`b > a` would be canonicalized to `a < b`)
+struct Canonicalizer {}
+
+impl Canonicalizer {
+ fn new() -> Self {
+ Self {}
+ }
+}
+
+impl TreeNodeRewriter for Canonicalizer {
+ type N = Expr;
+
+ fn mutate(&mut self, expr: Expr) -> Result<Expr> {
+ let Expr::BinaryExpr(BinaryExpr { left, op, right }) = expr else {
+ return Ok(expr);
+ };
+ match (left.as_ref(), right.as_ref(), op.swap()) {
+ // <col1> <op> <col2>
+ (Expr::Column(left_col), Expr::Column(right_col),
Some(swapped_op)) => {
+ if right_col > left_col {
+ return Ok(Expr::BinaryExpr(BinaryExpr {
+ left: right,
+ op: swapped_op,
+ right: left,
+ }));
+ }
+ }
+ // <literal> <op> <col>
+ (Expr::Literal(_a), Expr::Column(_b), Some(swapped_op)) => {
+ return Ok(Expr::BinaryExpr(BinaryExpr {
+ left: right,
+ op: swapped_op,
+ right: left,
+ }));
+ }
+ _ => {}
+ }
+ Ok(Expr::BinaryExpr(BinaryExpr { left, op, right }))
Review Comment:
```suggestion
match (left.as_ref(), right.as_ref(), op.swap()) {
// <col1> <op> <col2>
(Expr::Column(left_col), Expr::Column(right_col),
Some(swapped_op)) if right_col > left_col => {
Ok(Expr::BinaryExpr(BinaryExpr {
left: right,
op: swapped_op,
right: left,
}))
}
// <literal> <op> <col>
(Expr::Literal(_a), Expr::Column(_b), Some(swapped_op)) => {
Ok(Expr::BinaryExpr(BinaryExpr {
left: right,
op: swapped_op,
right: left,
}))
}
_ => Ok(Expr::BinaryExpr(BinaryExpr { left, op, right })),
}
```
One last suggestion for code structure, to take better advantage of how Rust
enables expressions to simplify things
Essentially pushing the if condition into that first match arm, and then
removing the need to have a `return` keyword as all the match arms will have
implicit return
--
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]
