Issue #674: Fix issues with semantics of parenthesis removal (#675)

* Issue #674: Fix issues with semantics of parenthesis removal

* Fix comment

* #674 Revamp how conditions are written to SQL - now uniformly handled as SimpleExpr:Binary

* #674 Fix clippy

* Issue #674 Move methods for deciding associativity and precedence to trait

* Added extra testcase from Issue #674

* Issue #674: Follow up on comments in code review

src/backend/mod.rs

@@ -85,3 +85,112 @@ pub trait EscapeBuilder {
         output
     }
 }
+
+pub trait PrecedenceDecider {
+    // This method decides which precedence relations should lead to dropped parentheses.
+    // There will be more fine grained precedence relations than the ones represented here,
+    // but dropping parentheses due to these relations can be confusing for readers.
+    fn inner_expr_well_known_greater_precedence(
+        &self,
+        inner: &SimpleExpr,
+        outer_oper: &Oper,
+    ) -> bool;
+}
+
+pub trait OperLeftAssocDecider {
+    // This method decides if the left associativity of an operator should lead to dropped parentheses.
+    // Not all known left associative operators are necessarily included here,
+    // as dropping them may in some cases be confusing to readers.
+    fn well_known_left_associative(&self, op: &BinOper) -> bool;
+}
+
+#[derive(Debug, PartialEq)]
+pub enum Oper {
+    UnOper(UnOper),
+    BinOper(BinOper),
+}
+
+impl From<UnOper> for Oper {
+    fn from(value: UnOper) -> Self {
+        Oper::UnOper(value)
+    }
+}
+
+impl From<BinOper> for Oper {
+    fn from(value: BinOper) -> Self {
+        Oper::BinOper(value)
+    }
+}
+
+impl Oper {
+    pub(crate) fn is_logical(&self) -> bool {
+        matches!(
+            self,
+            Oper::UnOper(UnOper::Not) | Oper::BinOper(BinOper::And) | Oper::BinOper(BinOper::Or)
+        )
+    }
+
+    pub(crate) fn is_between(&self) -> bool {
+        matches!(
+            self,
+            Oper::BinOper(BinOper::Between) | Oper::BinOper(BinOper::NotBetween)
+        )
+    }
+
+    pub(crate) fn is_like(&self) -> bool {
+        matches!(
+            self,
+            Oper::BinOper(BinOper::Like) | Oper::BinOper(BinOper::NotLike)
+        )
+    }
+
+    pub(crate) fn is_in(&self) -> bool {
+        matches!(
+            self,
+            Oper::BinOper(BinOper::In) | Oper::BinOper(BinOper::NotIn)
+        )
+    }
+
+    pub(crate) fn is_is(&self) -> bool {
+        matches!(
+            self,
+            Oper::BinOper(BinOper::Is) | Oper::BinOper(BinOper::IsNot)
+        )
+    }
+
+    pub(crate) fn is_shift(&self) -> bool {
+        matches!(
+            self,
+            Oper::BinOper(BinOper::LShift) | Oper::BinOper(BinOper::RShift)
+        )
+    }
+
+    pub(crate) fn is_arithmetic(&self) -> bool {
+        match self {
+            Oper::BinOper(b) => {
+                matches!(
+                    b,
+                    BinOper::Mul | BinOper::Div | BinOper::Mod | BinOper::Add | BinOper::Sub
+                )
+            }
+            _ => false,
+        }
+    }
+
+    pub(crate) fn is_comparison(&self) -> bool {
+        match self {
+            Oper::BinOper(b) => {
+                matches!(
+                    b,
+                    BinOper::SmallerThan
+                        | BinOper::SmallerThanOrEqual
+                        | BinOper::Equal
+                        | BinOper::GreaterThanOrEqual
+                        | BinOper::GreaterThan
+                        | BinOper::NotEqual
+                )
+            }
+            _ => false,
+        }
+    }
+}

src/backend/mysql/mod.rs

@@ -26,3 +26,19 @@ impl QuotedBuilder for MysqlQueryBuilder {
 impl EscapeBuilder for MysqlQueryBuilder {}
 
 impl TableRefBuilder for MysqlQueryBuilder {}
+
+impl PrecedenceDecider for MysqlQueryBuilder {
+    fn inner_expr_well_known_greater_precedence(
+        &self,
+        inner: &SimpleExpr,
+        outer_oper: &Oper,
+    ) -> bool {
+        common_inner_expr_well_known_greater_precedence(inner, outer_oper)
+    }
+}
+
+impl OperLeftAssocDecider for MysqlQueryBuilder {
+    fn well_known_left_associative(&self, op: &BinOper) -> bool {
+        common_well_known_left_associative(op)
+    }
+}

src/backend/postgres/query.rs

@@ -1,6 +1,38 @@
 use super::*;
 use crate::extension::postgres::*;
 
+impl OperLeftAssocDecider for PostgresQueryBuilder {
+    fn well_known_left_associative(&self, op: &BinOper) -> bool {
+        let common_answer = common_well_known_left_associative(op);
+        let pg_specific_answer = matches!(op, BinOper::PgOperator(PgBinOper::Concatenate));
+        common_answer || pg_specific_answer
+    }
+}
+
+impl PrecedenceDecider for PostgresQueryBuilder {
+    fn inner_expr_well_known_greater_precedence(
+        &self,
+        inner: &SimpleExpr,
+        outer_oper: &Oper,
+    ) -> bool {
+        let common_answer = common_inner_expr_well_known_greater_precedence(inner, outer_oper);
+        let pg_specific_answer = match inner {
+            SimpleExpr::Binary(_, inner_bin_oper, _) => {
+                let inner_oper: Oper = (*inner_bin_oper).into();
+                if inner_oper.is_arithmetic() || inner_oper.is_shift() {
+                    is_ilike(inner_bin_oper)
+                } else if is_pg_comparison(inner_bin_oper) {
+                    outer_oper.is_logical()
+                } else {
+                    false
+                }
+            }
+            _ => false,
+        };
+        common_answer || pg_specific_answer
+    }
+}
+
 impl QueryBuilder for PostgresQueryBuilder {
     fn placeholder(&self) -> (&str, bool) {
         ("$", true)
@@ -136,3 +168,22 @@ impl QueryBuilder for PostgresQueryBuilder {
         "COALESCE"
     }
 }
+
+fn is_pg_comparison(b: &BinOper) -> bool {
+    matches!(
+        b,
+        BinOper::PgOperator(PgBinOper::Contained)
+            | BinOper::PgOperator(PgBinOper::Contains)
+            | BinOper::PgOperator(PgBinOper::Similarity)
+            | BinOper::PgOperator(PgBinOper::WordSimilarity)
+            | BinOper::PgOperator(PgBinOper::StrictWordSimilarity)
+            | BinOper::PgOperator(PgBinOper::Matches)
+    )
+}
+
+fn is_ilike(b: &BinOper) -> bool {
+    matches!(
+        b,
+        BinOper::PgOperator(PgBinOper::ILike) | BinOper::PgOperator(PgBinOper::NotILike)
+    )
+}