From 61af132b6dfff1ca063c8e15c2cd4e94d521583c Mon Sep 17 00:00:00 2001 From: Noel Kwan Date: Wed, 1 Feb 2023 10:25:57 +0800 Subject: [PATCH 1/5] remove extra parenthesis from display --- src/sqlparser/src/ast/mod.rs | 49 +++++++++++++++++------------------- 1 file changed, 23 insertions(+), 26 deletions(-) diff --git a/src/sqlparser/src/ast/mod.rs b/src/sqlparser/src/ast/mod.rs index 5457a77531975..5ccf80eb20cb7 100644 --- a/src/sqlparser/src/ast/mod.rs +++ b/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)); + } } From 1f0ee665238e23481a16cd8839b14a471116a1bb Mon Sep 17 00:00:00 2001 From: Noel Kwan Date: Thu, 2 Feb 2023 17:54:25 +0800 Subject: [PATCH 2/5] fix tests --- src/sqlparser/tests/sqlparser_common.rs | 8 ++++---- src/sqlparser/tests/testdata/select.yaml | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/sqlparser/tests/sqlparser_common.rs b/src/sqlparser/tests/sqlparser_common.rs index d5f1f08bc013a..5a2bbbd36a2c8 100644 --- a/src/sqlparser/tests/sqlparser_common.rs +++ b/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 { diff --git a/src/sqlparser/tests/testdata/select.yaml b/src/sqlparser/tests/testdata/select.yaml index 78efa0e532a25..342a2f86e9e41 100644 --- a/src/sqlparser/tests/testdata/select.yaml +++ b/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" From e7fdd0b0592eeccd641da5e8fa47c14b645d1c4b Mon Sep 17 00:00:00 2001 From: Noel Kwan Date: Thu, 2 Feb 2023 18:43:31 +0800 Subject: [PATCH 3/5] fix --- src/sqlparser/tests/sqlparser_common.rs | 2 +- src/tests/sqlsmith/tests/frontend/mod.rs | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/sqlparser/tests/sqlparser_common.rs b/src/sqlparser/tests/sqlparser_common.rs index 5a2bbbd36a2c8..2836572c820ac 100644 --- a/src/sqlparser/tests/sqlparser_common.rs +++ b/src/sqlparser/tests/sqlparser_common.rs @@ -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 { diff --git a/src/tests/sqlsmith/tests/frontend/mod.rs b/src/tests/sqlsmith/tests/frontend/mod.rs index 7997ae22fe2af..fd2f90b074804 100644 --- a/src/tests/sqlsmith/tests/frontend/mod.rs +++ b/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(); From 753fec99a9c33a8809ea06e20a3706b5ef5483d0 Mon Sep 17 00:00:00 2001 From: Noel Kwan Date: Fri, 3 Feb 2023 10:45:18 +0800 Subject: [PATCH 4/5] fix --- src/sqlparser/tests/sqlparser_common.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sqlparser/tests/sqlparser_common.rs b/src/sqlparser/tests/sqlparser_common.rs index 2836572c820ac..5bd2fdfa56e87 100644 --- a/src/sqlparser/tests/sqlparser_common.rs +++ b/src/sqlparser/tests/sqlparser_common.rs @@ -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 { From ceeafd0bb7f0f2f87070a3cd115dc32ea4215dae Mon Sep 17 00:00:00 2001 From: Noel Kwan Date: Fri, 3 Feb 2023 14:33:20 +0800 Subject: [PATCH 5/5] fix --- src/tests/sqlsmith/src/sql_gen/expr.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/tests/sqlsmith/src/sql_gen/expr.rs b/src/tests/sqlsmith/src/sql_gen/expr.rs index a9a85cf68fbf7..7bc02e23b4bd9 100644 --- a/src/tests/sqlsmith/src/sql_gen/expr.rs +++ b/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),