phillipleblanc commented on code in PR #11186:
URL: https://github.com/apache/datafusion/pull/11186#discussion_r1663380022
##########
datafusion/expr/src/operator.rs:
##########
@@ -218,37 +218,33 @@ impl Operator {
}
/// Get the operator precedence
- /// use <https://www.postgresql.org/docs/7.0/operators.htm#AEN2026> as a
reference
+ /// use <https://www.postgresql.org/docs/7.2/sql-precedence.html> as a
reference
pub fn precedence(&self) -> u8 {
Review Comment:
It looks like this is only used in a few places for formatting the
parenthesis in a `Display` trait on `BinaryExpr`. I don't think that will have
any major impact on library users.
##########
datafusion/sql/src/unparser/expr.rs:
##########
@@ -603,6 +612,60 @@ impl Unparser<'_> {
}
}
+ /// Given an expression of the form `((a + b) * (c * d))`,
+ /// the parenthesing is redundant if the precedence of the nested
expression is already higher
+ /// than the surrounding operators' precedence. The above expression would
become
+ /// `(a + b) * c * d`.
+ ///
+ /// Also note that when fetching the precedence of a nested expression, we
ignore other nested
+ /// expressions, so precedence of expr `(a * (b + c))` equals `*` and not
`+`.
+ fn pretty(
Review Comment:
If we just have a single `expr_to_sql` method, then it might make sense to
rename this method as well to something like `remove_unnecessary_nesting` to
more accurately describe what it does. Then if we added more "rewrite passes"
to make it prettier or achieve some other functional goal, then they would just
be added as separate functions after this one.
##########
datafusion/sql/src/unparser/expr.rs:
##########
@@ -101,7 +101,16 @@ pub fn expr_to_unparsed(expr: &Expr) -> Result<Unparsed> {
unparser.expr_to_unparsed(expr)
}
+const LOWEST: &BinaryOperator = &BinaryOperator::Or;
+
impl Unparser<'_> {
+ /// Try to unparse the expression into a more human-readable format
+ /// by removing unnecessary parentheses.
+ pub fn pretty_expr_to_sql(&self, expr: &Expr) -> Result<ast::Expr> {
+ let root_expr = self.expr_to_sql(expr)?;
+ Ok(self.pretty(root_expr, LOWEST, LOWEST))
+ }
Review Comment:
I wouldn't have an extra method here and would combine it with
`expr_to_sql`. The `ast::Expr` it produces is logically the same as the input
one, just with unnecessary nesting removed. In fact, you could even think about
this as serving the same purpose as an optimizer rewrite pass for LogicalPlan -
it should produce logically the same thing as the input, just more efficient.
##########
datafusion/sql/src/unparser/expr.rs:
##########
@@ -618,6 +681,50 @@ impl Unparser<'_> {
}
}
+ // TODO: operator precedence should be defined in sqlparser
+ // to avoid the need for sql_to_op and sql_op_precedence
+ fn sql_op_precedence(&self, op: &BinaryOperator) -> u8 {
+ match self.sql_to_op(op) {
+ Ok(op) => op.precedence(),
+ Err(_) => 0,
+ }
+ }
+
+ fn sql_to_op(&self, op: &BinaryOperator) -> Result<Operator> {
+ match op {
+ ast::BinaryOperator::Eq => Ok(Operator::Eq),
+ ast::BinaryOperator::NotEq => Ok(Operator::NotEq),
+ ast::BinaryOperator::Lt => Ok(Operator::Lt),
+ ast::BinaryOperator::LtEq => Ok(Operator::LtEq),
+ ast::BinaryOperator::Gt => Ok(Operator::Gt),
+ ast::BinaryOperator::GtEq => Ok(Operator::GtEq),
+ ast::BinaryOperator::Plus => Ok(Operator::Plus),
+ ast::BinaryOperator::Minus => Ok(Operator::Minus),
+ ast::BinaryOperator::Multiply => Ok(Operator::Multiply),
+ ast::BinaryOperator::Divide => Ok(Operator::Divide),
+ ast::BinaryOperator::Modulo => Ok(Operator::Modulo),
+ ast::BinaryOperator::And => Ok(Operator::And),
+ ast::BinaryOperator::Or => Ok(Operator::Or),
+ ast::BinaryOperator::PGRegexMatch => Ok(Operator::RegexMatch),
+ ast::BinaryOperator::PGRegexIMatch => Ok(Operator::RegexIMatch),
+ ast::BinaryOperator::PGRegexNotMatch =>
Ok(Operator::RegexNotMatch),
+ ast::BinaryOperator::PGRegexNotIMatch =>
Ok(Operator::RegexNotIMatch),
+ ast::BinaryOperator::PGILikeMatch => Ok(Operator::ILikeMatch),
+ ast::BinaryOperator::PGNotLikeMatch => Ok(Operator::NotLikeMatch),
+ ast::BinaryOperator::PGLikeMatch => Ok(Operator::LikeMatch),
+ ast::BinaryOperator::PGNotILikeMatch =>
Ok(Operator::NotILikeMatch),
+ ast::BinaryOperator::BitwiseAnd => Ok(Operator::BitwiseAnd),
+ ast::BinaryOperator::BitwiseOr => Ok(Operator::BitwiseOr),
+ ast::BinaryOperator::BitwiseXor => Ok(Operator::BitwiseXor),
+ ast::BinaryOperator::PGBitwiseShiftRight =>
Ok(Operator::BitwiseShiftRight),
+ ast::BinaryOperator::PGBitwiseShiftLeft =>
Ok(Operator::BitwiseShiftLeft),
+ ast::BinaryOperator::StringConcat => Ok(Operator::StringConcat),
+ ast::BinaryOperator::AtArrow => Ok(Operator::AtArrow),
+ ast::BinaryOperator::ArrowAt => Ok(Operator::ArrowAt),
+ _ => not_impl_err!("unsupported operation: {op:?}"),
Review Comment:
I counted 49 operators for the `ast::BinaryOperator` enum, and there are 29
here. The other 20 don't appear to be things we would support in DataFusion and
thus need to unparse, so this looks good to me.
--
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]
