fix: parenthesize nested expression display names

datafusion/expr/src/expr.rs

@@ -2940,6 +2940,32 @@ macro_rules! expr_vec_fmt {
 }
 
 struct SchemaDisplay<'a>(&'a Expr);
+
+fn write_schema_display_binary_child(
+    f: &mut Formatter<'_>,
+    expr: &Expr,
+    precedence: u8,
+) -> fmt::Result {
+    match expr {
+        Expr::BinaryExpr(child) => {
+            let child_precedence = child.op.precedence();
+            if child_precedence == 0 || child_precedence < precedence {
+                write!(f, "({})", SchemaDisplay(expr))
+            } else {
+                write!(f, "{}", SchemaDisplay(expr))
+            }
+        }
+        _ => write!(f, "{}", SchemaDisplay(expr)),
+    }
+}
+
+fn write_schema_display_unary_child(f: &mut Formatter<'_>, expr: &Expr) -> fmt::Result {
+    match expr {
+        Expr::BinaryExpr(_) => write!(f, "({})", SchemaDisplay(expr)),
+        _ => write!(f, "{}", SchemaDisplay(expr)),
+    }
+}
+
 impl Display for SchemaDisplay<'_> {
     fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
         match self.0 {
@@ -2994,7 +3020,10 @@ impl Display for SchemaDisplay<'_> {
                 }
             }
             Expr::BinaryExpr(BinaryExpr { left, op, right }) => {
-                write!(f, "{} {op} {}", SchemaDisplay(left), SchemaDisplay(right),)
+                let precedence = op.precedence();
+                write_schema_display_binary_child(f, left, precedence)?;
+                write!(f, " {op} ")?;
+                write_schema_display_binary_child(f, right, precedence)
             }
             Expr::Case(Case {
                 expr,
@@ -3104,8 +3133,15 @@ impl Display for SchemaDisplay<'_> {
 
                 Ok(())
             }
-            Expr::Negative(expr) => write!(f, "(- {})", SchemaDisplay(expr)),
-            Expr::Not(expr) => write!(f, "NOT {}", SchemaDisplay(expr)),
+            Expr::Negative(expr) => {
+                write!(f, "(- ")?;
+                write_schema_display_unary_child(f, expr)?;
+                write!(f, ")")
+            }
+            Expr::Not(expr) => {
+                write!(f, "NOT ")?;
+                write_schema_display_unary_child(f, expr)
+            }
             Expr::Unnest(Unnest { expr }) => {
                 write!(f, "UNNEST({})", SchemaDisplay(expr))
             }
@@ -3245,6 +3281,31 @@ impl Display for SchemaDisplay<'_> {
 /// A helper struct for displaying an `Expr` as an SQL-like string.
 struct SqlDisplay<'a>(&'a Expr);
 
+fn write_sql_display_binary_child(
+    f: &mut Formatter<'_>,
+    expr: &Expr,
+    precedence: u8,
+) -> fmt::Result {
+    match expr {
+        Expr::BinaryExpr(child) => {
+            let child_precedence = child.op.precedence();
+            if child_precedence == 0 || child_precedence < precedence {
+                write!(f, "({})", SqlDisplay(expr))
+            } else {
+                write!(f, "{}", SqlDisplay(expr))
+            }
+        }
+        _ => write!(f, "{}", SqlDisplay(expr)),
+    }
+}
+
+fn write_sql_display_unary_child(f: &mut Formatter<'_>, expr: &Expr) -> fmt::Result {
+    match expr {
+        Expr::BinaryExpr(_) => write!(f, "({})", SqlDisplay(expr)),
+        _ => write!(f, "{}", SqlDisplay(expr)),
+    }
+}
+
 impl Display for SqlDisplay<'_> {
     fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
         match self.0 {
@@ -3275,7 +3336,10 @@ impl Display for SqlDisplay<'_> {
                 }
             }
             Expr::BinaryExpr(BinaryExpr { left, op, right }) => {
-                write!(f, "{} {op} {}", SqlDisplay(left), SqlDisplay(right),)
+                let precedence = op.precedence();
+                write_sql_display_binary_child(f, left, precedence)?;
+                write!(f, " {op} ")?;
+                write_sql_display_binary_child(f, right, precedence)
             }
             Expr::Case(Case {
                 expr,
@@ -3379,8 +3443,15 @@ impl Display for SqlDisplay<'_> {
 
                 Ok(())
             }
-            Expr::Negative(expr) => write!(f, "(- {})", SqlDisplay(expr)),
-            Expr::Not(expr) => write!(f, "NOT {}", SqlDisplay(expr)),
+            Expr::Negative(expr) => {
+                write!(f, "(- ")?;
+                write_sql_display_unary_child(f, expr)?;
+                write!(f, ")")
+            }
+            Expr::Not(expr) => {
+                write!(f, "NOT ")?;
+                write_sql_display_unary_child(f, expr)
+            }
             Expr::Unnest(Unnest { expr }) => {
                 write!(f, "UNNEST({})", SqlDisplay(expr))
             }
@@ -3507,6 +3578,13 @@ pub fn schema_name_from_sorts(sorts: &[Sort]) -> Result<String, fmt::Error> {
 pub const OUTER_REFERENCE_COLUMN_PREFIX: &str = "outer_ref";
 pub const UNNEST_COLUMN_PREFIX: &str = "UNNEST";
 
+fn write_expr_display_unary_child(f: &mut Formatter<'_>, expr: &Expr) -> fmt::Result {
+    match expr {
+        Expr::BinaryExpr(_) => write!(f, "({expr})"),
+        _ => write!(f, "{expr}"),
+    }
+}
+
 /// Format expressions for display as part of a logical plan. In many cases, this will produce
 /// similar output to `Expr.name()` except that column names will be prefixed with '#'.
 impl Display for Expr {
@@ -3547,8 +3625,15 @@ impl Display for Expr {
                     format_type_and_metadata(field.data_type(), Some(field.metadata()));
                 write!(f, "TRY_CAST({expr} AS {formatted})")
             }
-            Expr::Not(expr) => write!(f, "NOT {expr}"),
-            Expr::Negative(expr) => write!(f, "(- {expr})"),
+            Expr::Not(expr) => {
+                write!(f, "NOT ")?;
+                write_expr_display_unary_child(f, expr)
+            }
+            Expr::Negative(expr) => {
+                write!(f, "(- ")?;
+                write_expr_display_unary_child(f, expr)?;
+                write!(f, ")")
+            }
             Expr::IsNull(expr) => write!(f, "{expr} IS NULL"),
             Expr::IsNotNull(expr) => write!(f, "{expr} IS NOT NULL"),
             Expr::IsTrue(expr) => write!(f, "{expr} IS TRUE"),
@@ -4073,6 +4158,35 @@ mod test {
         assert_eq!("NULL", null_expr.human_display().to_string());
     }
 
+    #[test]
+    fn format_nested_binary_exprs_with_parentheses() {
+        let one_plus_two = binary_expr(lit(1i64), Operator::Plus, lit(2i64));
+        let expr = binary_expr(one_plus_two.clone(), Operator::Multiply, lit(3i64));
+
+        assert_eq!(
+            "(Int64(1) + Int64(2)) * Int64(3)",
+            expr.schema_name().to_string()
+        );
+        assert_eq!("(1 + 2) * 3", expr.human_display().to_string());
+
+        let negative_expr = Expr::Negative(Box::new(one_plus_two.clone()));
+        assert_eq!(
+            "(- (Int64(1) + Int64(2)))",
+            negative_expr.schema_name().to_string()
+        );
+        assert_eq!("(- (1 + 2))", negative_expr.human_display().to_string());
+        assert_eq!("(- (Int64(1) + Int64(2)))", negative_expr.to_string());
+
+        let not_expr =
+            Expr::Not(Box::new(binary_expr(lit(1i64), Operator::Eq, lit(2i64))));
+        assert_eq!(
+            "NOT (Int64(1) = Int64(2))",
+            not_expr.schema_name().to_string()
+        );
+        assert_eq!("NOT (1 = 2)", not_expr.human_display().to_string());
+        assert_eq!("NOT (Int64(1) = Int64(2))", not_expr.to_string());
+    }
+
     #[test]
     fn test_partial_ord() {
         // Test validates that partial ord is defined for Expr, not

datafusion/optimizer/src/common_subexpr_eliminate.rs

@@ -909,7 +909,7 @@ mod test {
         assert_optimized_plan_equal!(
             plan,
             @ r"
-        Aggregate: groupBy=[[]], aggr=[[sum(__common_expr_1 AS test.a * Int32(1) - test.b), sum(__common_expr_1 AS test.a * Int32(1) - test.b * (Int32(1) + test.c))]]
+        Aggregate: groupBy=[[]], aggr=[[sum(__common_expr_1 AS test.a * (Int32(1) - test.b)), sum(__common_expr_1 AS test.a * (Int32(1) - test.b) * (Int32(1) + test.c))]]
           Projection: test.a * (Int32(1) - test.b) AS __common_expr_1, test.a, test.b, test.c
             TableScan: test
         "
@@ -1735,8 +1735,8 @@ mod test {
         assert_optimized_plan_equal!(
             plan,
             @ r"
-        Projection: __common_expr_1 AS NOT test.a = test.b, __common_expr_1 AS NOT test.b = test.a
-          Projection: NOT test.a = test.b AS __common_expr_1, test.a, test.b, test.c
+        Projection: __common_expr_1 AS NOT (test.a = test.b), __common_expr_1 AS NOT (test.b = test.a)
+          Projection: NOT (test.a = test.b) AS __common_expr_1, test.a, test.b, test.c
             TableScan: test
         "
         )?;

datafusion/optimizer/src/simplify_expressions/simplify_exprs.rs

@@ -634,7 +634,7 @@ mod tests {
             .build()?;
 
         let actual = get_optimized_plan_formatted(plan, &time);
-        let expected = "Projection: NOT test.a AS Boolean(true) OR Boolean(false) != test.a\
+        let expected = "Projection: NOT test.a AS (Boolean(true) OR Boolean(false)) != test.a\
                         \n  TableScan: test";
 
         assert_eq!(expected, actual);

datafusion/sqllogictest/test_files/tpch/plans/q1.slt.part

@@ -41,18 +41,18 @@ explain select
 ----
 logical_plan
 01)Sort: lineitem.l_returnflag ASC NULLS LAST, lineitem.l_linestatus ASC NULLS LAST
-02)--Projection: lineitem.l_returnflag, lineitem.l_linestatus, sum(lineitem.l_quantity) AS sum_qty, sum(lineitem.l_extendedprice) AS sum_base_price, sum(lineitem.l_extendedprice * Int64(1) - lineitem.l_discount) AS sum_disc_price, sum(lineitem.l_extendedprice * Int64(1) - lineitem.l_discount * Int64(1) + lineitem.l_tax) AS sum_charge, avg(lineitem.l_quantity) AS avg_qty, avg(lineitem.l_extendedprice) AS avg_price, avg(lineitem.l_discount) AS avg_disc, count(Int64(1)) AS count(*) AS count_order
-03)----Aggregate: groupBy=[[lineitem.l_returnflag, lineitem.l_linestatus]], aggr=[[sum(lineitem.l_quantity), sum(lineitem.l_extendedprice), sum(__common_expr_1) AS sum(lineitem.l_extendedprice * Int64(1) - lineitem.l_discount), sum(__common_expr_1 * (Decimal128(1,20,0) + lineitem.l_tax)) AS sum(lineitem.l_extendedprice * Int64(1) - lineitem.l_discount * Int64(1) + lineitem.l_tax), avg(lineitem.l_quantity), avg(lineitem.l_extendedprice), avg(lineitem.l_discount), count(Int64(1))]]
+02)--Projection: lineitem.l_returnflag, lineitem.l_linestatus, sum(lineitem.l_quantity) AS sum_qty, sum(lineitem.l_extendedprice) AS sum_base_price, sum(lineitem.l_extendedprice * (Int64(1) - lineitem.l_discount)) AS sum_disc_price, sum(lineitem.l_extendedprice * (Int64(1) - lineitem.l_discount) * (Int64(1) + lineitem.l_tax)) AS sum_charge, avg(lineitem.l_quantity) AS avg_qty, avg(lineitem.l_extendedprice) AS avg_price, avg(lineitem.l_discount) AS avg_disc, count(Int64(1)) AS count(*) AS count_order
+03)----Aggregate: groupBy=[[lineitem.l_returnflag, lineitem.l_linestatus]], aggr=[[sum(lineitem.l_quantity), sum(lineitem.l_extendedprice), sum(__common_expr_1) AS sum(lineitem.l_extendedprice * (Int64(1) - lineitem.l_discount)), sum(__common_expr_1 * (Decimal128(1,20,0) + lineitem.l_tax)) AS sum(lineitem.l_extendedprice * (Int64(1) - lineitem.l_discount) * (Int64(1) + lineitem.l_tax)), avg(lineitem.l_quantity), avg(lineitem.l_extendedprice), avg(lineitem.l_discount), count(Int64(1))]]
 04)------Projection: lineitem.l_extendedprice * (Decimal128(1,20,0) - lineitem.l_discount) AS __common_expr_1, lineitem.l_quantity, lineitem.l_extendedprice, lineitem.l_discount, lineitem.l_tax, lineitem.l_returnflag, lineitem.l_linestatus
 05)--------Filter: lineitem.l_shipdate <= Date32("1998-09-02")
 06)----------TableScan: lineitem projection=[l_quantity, l_extendedprice, l_discount, l_tax, l_returnflag, l_linestatus, l_shipdate], partial_filters=[lineitem.l_shipdate <= Date32("1998-09-02")]
 physical_plan
 01)SortPreservingMergeExec: [l_returnflag@0 ASC NULLS LAST, l_linestatus@1 ASC NULLS LAST]
 02)--SortExec: expr=[l_returnflag@0 ASC NULLS LAST, l_linestatus@1 ASC NULLS LAST], preserve_partitioning=[true]
-03)----ProjectionExec: expr=[l_returnflag@0 as l_returnflag, l_linestatus@1 as l_linestatus, sum(lineitem.l_quantity)@2 as sum_qty, sum(lineitem.l_extendedprice)@3 as sum_base_price, sum(lineitem.l_extendedprice * Int64(1) - lineitem.l_discount)@4 as sum_disc_price, sum(lineitem.l_extendedprice * Int64(1) - lineitem.l_discount * Int64(1) + lineitem.l_tax)@5 as sum_charge, avg(lineitem.l_quantity)@6 as avg_qty, avg(lineitem.l_extendedprice)@7 as avg_price, avg(lineitem.l_discount)@8 as avg_disc, count(Int64(1))@9 as count_order]
-04)------AggregateExec: mode=FinalPartitioned, gby=[l_returnflag@0 as l_returnflag, l_linestatus@1 as l_linestatus], aggr=[sum(lineitem.l_quantity), sum(lineitem.l_extendedprice), sum(__common_expr_1) as sum(lineitem.l_extendedprice * Int64(1) - lineitem.l_discount), sum(__common_expr_1 * 1 + lineitem.l_tax) as sum(lineitem.l_extendedprice * Int64(1) - lineitem.l_discount * Int64(1) + lineitem.l_tax), avg(lineitem.l_quantity), avg(lineitem.l_extendedprice), avg(lineitem.l_discount), count(Int64(1))]
+03)----ProjectionExec: expr=[l_returnflag@0 as l_returnflag, l_linestatus@1 as l_linestatus, sum(lineitem.l_quantity)@2 as sum_qty, sum(lineitem.l_extendedprice)@3 as sum_base_price, sum(lineitem.l_extendedprice * (Int64(1) - lineitem.l_discount))@4 as sum_disc_price, sum(lineitem.l_extendedprice * (Int64(1) - lineitem.l_discount) * (Int64(1) + lineitem.l_tax))@5 as sum_charge, avg(lineitem.l_quantity)@6 as avg_qty, avg(lineitem.l_extendedprice)@7 as avg_price, avg(lineitem.l_discount)@8 as avg_disc, count(Int64(1))@9 as count_order]
+04)------AggregateExec: mode=FinalPartitioned, gby=[l_returnflag@0 as l_returnflag, l_linestatus@1 as l_linestatus], aggr=[sum(lineitem.l_quantity), sum(lineitem.l_extendedprice), sum(__common_expr_1) as sum(lineitem.l_extendedprice * (Int64(1) - lineitem.l_discount)), sum(__common_expr_1 * (1 + lineitem.l_tax)) as sum(lineitem.l_extendedprice * (Int64(1) - lineitem.l_discount) * (Int64(1) + lineitem.l_tax)), avg(lineitem.l_quantity), avg(lineitem.l_extendedprice), avg(lineitem.l_discount), count(Int64(1))]
 05)--------RepartitionExec: partitioning=Hash([l_returnflag@0, l_linestatus@1], 4), input_partitions=4
-06)----------AggregateExec: mode=Partial, gby=[l_returnflag@5 as l_returnflag, l_linestatus@6 as l_linestatus], aggr=[sum(lineitem.l_quantity), sum(lineitem.l_extendedprice), sum(__common_expr_1) as sum(lineitem.l_extendedprice * Int64(1) - lineitem.l_discount), sum(__common_expr_1 * 1 + lineitem.l_tax) as sum(lineitem.l_extendedprice * Int64(1) - lineitem.l_discount * Int64(1) + lineitem.l_tax), avg(lineitem.l_quantity), avg(lineitem.l_extendedprice), avg(lineitem.l_discount), count(Int64(1))]
+06)----------AggregateExec: mode=Partial, gby=[l_returnflag@5 as l_returnflag, l_linestatus@6 as l_linestatus], aggr=[sum(lineitem.l_quantity), sum(lineitem.l_extendedprice), sum(__common_expr_1) as sum(lineitem.l_extendedprice * (Int64(1) - lineitem.l_discount)), sum(__common_expr_1 * (1 + lineitem.l_tax)) as sum(lineitem.l_extendedprice * (Int64(1) - lineitem.l_discount) * (Int64(1) + lineitem.l_tax)), avg(lineitem.l_quantity), avg(lineitem.l_extendedprice), avg(lineitem.l_discount), count(Int64(1))]
 07)------------ProjectionExec: expr=[l_extendedprice@0 * (1 - l_discount@1) as __common_expr_1, l_quantity@2 as l_quantity, l_extendedprice@0 as l_extendedprice, l_discount@1 as l_discount, l_tax@3 as l_tax, l_returnflag@4 as l_returnflag, l_linestatus@5 as l_linestatus]
 08)--------------FilterExec: l_shipdate@6 <= 1998-09-02, projection=[l_extendedprice@1, l_discount@2, l_quantity@0, l_tax@3, l_returnflag@4, l_linestatus@5]
 09)----------------DataSourceExec: file_groups={4 groups: [[WORKSPACE_ROOT/datafusion/sqllogictest/test_files/tpch/data/lineitem.tbl:0..18561749], [WORKSPACE_ROOT/datafusion/sqllogictest/test_files/tpch/data/lineitem.tbl:18561749..37123498], [WORKSPACE_ROOT/datafusion/sqllogictest/test_files/tpch/data/lineitem.tbl:37123498..55685247], [WORKSPACE_ROOT/datafusion/sqllogictest/test_files/tpch/data/lineitem.tbl:55685247..74246996]]}, projection=[l_quantity, l_extendedprice, l_discount, l_tax, l_returnflag, l_linestatus, l_shipdate], constraints=[PrimaryKey([0, 3])], file_type=csv, has_header=false