fix(parser): remove extra parenthesis when displaying nested operations

src/sqlparser/src/ast/mod.rs

@@ -396,20 +396,14 @@ impl fmt::Display for Expr {
                 low,
                 high
             ),
-            Expr::BinaryOp { left, op, right } => write!(
-                f,
-                "{} {} {}",
-                fmt_expr_with_paren(left),
-                op,
-                fmt_expr_with_paren(right)
-            ),
+            Expr::BinaryOp { left, op, right } => write!(f, "{} {} {}", left, op, right),
             Expr::SomeOp(expr) => write!(f, "SOME({})", expr),
             Expr::AllOp(expr) => write!(f, "ALL({})", expr),
             Expr::UnaryOp { op, expr } => {
                 if op == &UnaryOperator::PGPostfixFactorial {
                     write!(f, "{}{}", expr, op)
                 } else {
-                    write!(f, "{} {}", op, fmt_expr_with_paren(expr))
+                    write!(f, "{} {}", op, expr)
                 }
             }
             Expr::Cast { expr, data_type } => write!(f, "CAST({} AS {})", expr, data_type),
@@ -557,24 +551,6 @@ impl fmt::Display for Expr {
     }
 }
 
-/// Wrap complex expressions with necessary parentheses.
-/// For example, `a > b LIKE c` becomes `a > (b LIKE c)`.
-fn fmt_expr_with_paren(e: &Expr) -> String {
-    use Expr as E;
-    match e {
-        E::BinaryOp { .. }
-        | E::UnaryOp { .. }
-        | E::IsNull(_)
-        | E::IsNotNull(_)
-        | E::IsFalse(_)
-        | E::IsTrue(_)
-        | E::IsNotTrue(_)
-        | E::IsNotFalse(_) => return format!("({})", e),
-        _ => {}
-    };
-    format!("{}", e)
-}
-
 /// A window specification (i.e. `OVER (PARTITION BY .. ORDER BY .. etc.)`)
 #[derive(Debug, Clone, PartialEq, Eq, Hash)]
 #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
@@ -2298,4 +2274,25 @@ mod tests {
         };
         assert_eq!("v1[1][1]", format!("{}", array_index2));
     }
+
+    #[test]
+    /// issue: https://github.com/risingwavelabs/risingwave/issues/7635
+    fn test_nested_op_display() {
+        let binary_op = Expr::BinaryOp {
+            left: Box::new(Expr::Value(Value::Boolean(true))),
+            op: BinaryOperator::Or,
+            right: Box::new(Expr::IsNotFalse(Box::new(Expr::Value(Value::Boolean(
+                true,
+            ))))),
+        };
+        assert_eq!("true OR true IS NOT FALSE", format!("{}", binary_op));
+
+        let unary_op = Expr::UnaryOp {
+            op: UnaryOperator::Not,
+            expr: Box::new(Expr::IsNotFalse(Box::new(Expr::Value(Value::Boolean(
+                true,
+            ))))),
+        };
+        assert_eq!("NOT true IS NOT FALSE", format!("{}", unary_op));
+    }
 }

src/sqlparser/tests/sqlparser_common.rs

@@ -457,7 +457,7 @@ fn parse_compound_expr_1() {
     use self::Expr::*;
     let sql = "a + b * c";
     let ast = run_parser_method(sql, |parser| parser.parse_expr()).unwrap();
-    assert_eq!("a + (b * c)", &ast.to_string());
+    assert_eq!("a + b * c", &ast.to_string());
     assert_eq!(
         BinaryOp {
             left: Box::new(Identifier(Ident::new("a"))),
@@ -478,7 +478,7 @@ fn parse_compound_expr_2() {
     use self::Expr::*;
     let sql = "a * b + c";
     let ast = run_parser_method(sql, |parser| parser.parse_expr()).unwrap();
-    assert_eq!("(a * b) + c", &ast.to_string());
+    assert_eq!("a * b + c", &ast.to_string());
     assert_eq!(
         BinaryOp {
             left: Box::new(BinaryOp {
@@ -498,7 +498,7 @@ fn parse_unary_math() {
     use self::Expr::*;
     let sql = "- a + - b";
     let ast = run_parser_method(sql, |parser| parser.parse_expr()).unwrap();
-    assert_eq!("(- a) + (- b)", &ast.to_string());
+    assert_eq!("- a + - b", &ast.to_string());
     assert_eq!(
         BinaryOp {
             left: Box::new(UnaryOp {
@@ -565,7 +565,7 @@ fn parse_not_precedence() {
     // NOT has higher precedence than OR/AND, so the following must parse as (NOT true) OR true
     let sql = "NOT true OR true";
     let ast = run_parser_method(sql, |parser| parser.parse_expr()).unwrap();
-    assert_eq!("(NOT true) OR true", &ast.to_string());
+    assert_eq!("NOT true OR true", &ast.to_string());
     assert_matches!(
         ast,
         Expr::BinaryOp {
@@ -578,7 +578,7 @@ fn parse_not_precedence() {
     // NULL)
     let sql = "NOT a IS NULL";
     let ast = run_parser_method(sql, |parser| parser.parse_expr()).unwrap();
-    assert_eq!("NOT (a IS NULL)", &ast.to_string());
+    assert_eq!("NOT a IS NULL", &ast.to_string());
     assert_matches!(
         ast,
         Expr::UnaryOp {
@@ -605,7 +605,7 @@ fn parse_not_precedence() {
     // NOT has lower precedence than LIKE, so the following parses as NOT ('a' NOT LIKE 'b')
     let sql = "NOT 'a' NOT LIKE 'b'";
     let ast = run_parser_method(sql, |parser| parser.parse_expr()).unwrap();
-    assert_eq!("NOT ('a' NOT LIKE 'b')", &ast.to_string());
+    assert_eq!("NOT 'a' NOT LIKE 'b'", &ast.to_string());
     assert_eq!(
         ast,
         Expr::UnaryOp {

src/sqlparser/tests/testdata/select.yaml

@@ -53,10 +53,10 @@
     Query(Query { with: None, body: Select(Select { distinct: All, projection: [Wildcard], from: [TableWithJoins { relation: TableFunction { name: ObjectName([Ident { value: "unnest", quote_style: None }]), alias: None, args: [Unnamed(Expr(Array([Value(Number("1")), Value(Number("2")), Value(Number("3"))])))] }, joins: [] }], lateral_views: [], selection: None, group_by: [], having: None }), order_by: [], limit: None, offset: None, fetch: None })
 
 - input: SELECT id, fname, lname FROM customer WHERE salary <> 'Not Provided' AND salary <> ''
-  formatted_sql: SELECT id, fname, lname FROM customer WHERE (salary <> 'Not Provided') AND (salary <> '')
+  formatted_sql: SELECT id, fname, lname FROM customer WHERE salary <> 'Not Provided' AND salary <> ''
 
 - input: SELECT id FROM customer WHERE NOT salary = ''
-  formatted_sql: SELECT id FROM customer WHERE NOT (salary = '')
+  formatted_sql: SELECT id FROM customer WHERE NOT salary = ''
 
 - input: SELECT * FROM t LIMIT 1 FETCH FIRST ROWS ONLY
   error_msg: "sql parser error: Cannot specify both LIMIT and FETCH"

src/tests/sqlsmith/src/sql_gen/expr.rs

@@ -86,9 +86,20 @@ impl<'a, R: Rng> SqlGenerator<'a, R> {
             };
         }
 
+        // NOTE:
+        // We generate AST first, then use its `Display` trait
+        // to generate an sql string.
+        // That may erase nesting context.
+        // For instance `IN(a, b)` is `a IN b`.
+        // this can lead to ambiguity, if `a` is an
+        // INFIX/POSTFIX compound expression too:
+        // - `a1 IN a2 IN b`
+        // - `a1 >= a2 IN b`
+        // ...
+        // We just nest compound expressions to avoid this.
         let range = if context.can_gen_agg() { 99 } else { 90 };
         match self.rng.gen_range(0..=range) {
-            0..=70 => self.gen_func(typ, context),
+            0..=70 => Expr::Nested(Box::new(self.gen_func(typ, context))),
             71..=80 => self.gen_exists(typ, context),
             81..=90 => self.gen_explicit_cast(typ, context),
             91..=99 => self.gen_agg(typ),

src/tests/sqlsmith/tests/frontend/mod.rs

@@ -109,6 +109,7 @@ async fn create_tables(
     // Generate some mviews
     for i in 0..10 {
         let (sql, table) = mview_sql_gen(rng, tables.clone(), &format!("m{}", i));
+        reproduce_failing_queries(&setup_sql, &sql);
         setup_sql.push_str(&format!("{};", &sql));
         let stmts = parse_sql(&sql);
         let stmt = stmts[0].clone();