Implement Display for Expr, improve operator display (apache#971)

* # This is a combination of 3 commits.
# This is the 1st commit message:

Add Display for Expr::BinaryExpr

# This is the commit message #2:

Update logical_plan/operators tests

# This is the commit message #3:

rebase and debug display for non binary expr

* Add Display for Expr::BinaryExpr

Update logical_plan/operators tests

rebase and debug display for non binary expr

Add Display for Expr::BinaryExpr

Update logical_plan/operators tests

Updating tests

Update aggregate display

Updating tests without aggregate

More tests

Working on agg/scalar functions

Fix binary_expr in create_name function and attendant tests

More tests

More tests

Doc tests

Rebase and update new tests

* Submodule update

* Restore submodule references from master

Co-authored-by: Andrew Lamb <[email protected]>

Author: matthewmturner

datafusion/src/execution/context.rs

@@ -1696,15 +1696,15 @@ mod tests {
         .await?;
 
         let expected = vec![
-            "+----+----+--------------+-----------------------------------+----------------------------------+------------------------------------------+--------------+----------------+--------------+--------------+--------------+",
-            "| c1 | c2 | ROW_NUMBER() | FIRST_VALUE(test.c2 Plus test.c1) | LAST_VALUE(test.c2 Plus test.c1) | NTH_VALUE(test.c2 Plus test.c1,Int64(1)) | SUM(test.c2) | COUNT(test.c2) | MAX(test.c2) | MIN(test.c2) | AVG(test.c2) |",
-            "+----+----+--------------+-----------------------------------+----------------------------------+------------------------------------------+--------------+----------------+--------------+--------------+--------------+",
-            "| 0  | 1  | 1            | 1                                 | 1                                | 1                                        | 1            | 1              | 1            | 1            | 1            |",
-            "| 0  | 2  | 1            | 2                                 | 2                                | 2                                        | 2            | 1              | 2            | 2            | 2            |",
-            "| 0  | 3  | 1            | 3                                 | 3                                | 3                                        | 3            | 1              | 3            | 3            | 3            |",
-            "| 0  | 4  | 1            | 4                                 | 4                                | 4                                        | 4            | 1              | 4            | 4            | 4            |",
-            "| 0  | 5  | 1            | 5                                 | 5                                | 5                                        | 5            | 1              | 5            | 5            | 5            |",
-            "+----+----+--------------+-----------------------------------+----------------------------------+------------------------------------------+--------------+----------------+--------------+--------------+--------------+",
+            "+----+----+--------------+--------------------------------+-------------------------------+---------------------------------------+--------------+----------------+--------------+--------------+--------------+",
+            "| c1 | c2 | ROW_NUMBER() | FIRST_VALUE(test.c2 + test.c1) | LAST_VALUE(test.c2 + test.c1) | NTH_VALUE(test.c2 + test.c1,Int64(1)) | SUM(test.c2) | COUNT(test.c2) | MAX(test.c2) | MIN(test.c2) | AVG(test.c2) |",
+            "+----+----+--------------+--------------------------------+-------------------------------+---------------------------------------+--------------+----------------+--------------+--------------+--------------+",
+            "| 0  | 1  | 1            | 1                              | 1                             | 1                                     | 1            | 1              | 1            | 1            | 1            |",
+            "| 0  | 2  | 1            | 2                              | 2                             | 2                                     | 2            | 1              | 2            | 2            | 2            |",
+            "| 0  | 3  | 1            | 3                              | 3                             | 3                                     | 3            | 1              | 3            | 3            | 3            |",
+            "| 0  | 4  | 1            | 4                              | 4                             | 4                                     | 4            | 1              | 4            | 4            | 4            |",
+            "| 0  | 5  | 1            | 5                              | 5                             | 5                                     | 5            | 1              | 5            | 5            | 5            |",
+            "+----+----+--------------+--------------------------------+-------------------------------+---------------------------------------+--------------+----------------+--------------+--------------+--------------+",
         ];
 
         // window function shall respect ordering

datafusion/src/logical_plan/builder.rs

@@ -663,7 +663,7 @@ mod tests {
         .build()?;
 
         let expected = "Projection: #employee_csv.id\
-        \n  Filter: #employee_csv.state Eq Utf8(\"CO\")\
+        \n  Filter: #employee_csv.state = Utf8(\"CO\")\
         \n    TableScan: employee_csv projection=Some([0, 3])";
 
         assert_eq!(expected, format!("{:?}", plan));

datafusion/src/logical_plan/expr.rs

@@ -941,6 +941,33 @@ impl Not for Expr {
     }
 }
 
+impl std::fmt::Display for Expr {
+    fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+        match self {
+            Expr::BinaryExpr {
+                ref left,
+                ref right,
+                ref op,
+            } => write!(f, "{} {} {}", left, op, right),
+            Expr::AggregateFunction {
+                /// Name of the function
+                ref fun,
+                /// List of expressions to feed to the functions as arguments
+                ref args,
+                /// Whether this is a DISTINCT aggregation or not
+                ref distinct,
+            } => fmt_function(f, &fun.to_string(), *distinct, args, true),
+            Expr::ScalarFunction {
+                /// Name of the function
+                ref fun,
+                /// List of expressions to feed to the functions as arguments
+                ref args,
+            } => fmt_function(f, &fun.to_string(), false, args, true),
+            _ => write!(f, "{:?}", self),
+        }
+    }
+}
+
 #[allow(clippy::boxed_local)]
 fn rewrite_boxed<R>(boxed_expr: Box<Expr>, rewriter: &mut R) -> Result<Box<Expr>>
 where
@@ -1603,8 +1630,14 @@ fn fmt_function(
     fun: &str,
     distinct: bool,
     args: &[Expr],
+    display: bool,
 ) -> fmt::Result {
-    let args: Vec<String> = args.iter().map(|arg| format!("{:?}", arg)).collect();
+    let args: Vec<String> = match display {
+        true => args.iter().map(|arg| format!("{}", arg)).collect(),
+        false => args.iter().map(|arg| format!("{:?}", arg)).collect(),
+    };
+
+    // let args: Vec<String> = args.iter().map(|arg| format!("{:?}", arg)).collect();
     let distinct_str = match distinct {
         true => "DISTINCT ",
         false => "",
@@ -1648,7 +1681,7 @@ impl fmt::Debug for Expr {
             Expr::IsNull(expr) => write!(f, "{:?} IS NULL", expr),
             Expr::IsNotNull(expr) => write!(f, "{:?} IS NOT NULL", expr),
             Expr::BinaryExpr { left, op, right } => {
-                write!(f, "{:?} {:?} {:?}", left, op, right)
+                write!(f, "{:?} {} {:?}", left, op, right)
             }
             Expr::Sort {
                 expr,
@@ -1667,10 +1700,10 @@ impl fmt::Debug for Expr {
                 }
             }
             Expr::ScalarFunction { fun, args, .. } => {
-                fmt_function(f, &fun.to_string(), false, args)
+                fmt_function(f, &fun.to_string(), false, args, false)
             }
             Expr::ScalarUDF { fun, ref args, .. } => {
-                fmt_function(f, &fun.name, false, args)
+                fmt_function(f, &fun.name, false, args, false)
             }
             Expr::WindowFunction {
                 fun,
@@ -1679,7 +1712,7 @@ impl fmt::Debug for Expr {
                 order_by,
                 window_frame,
             } => {
-                fmt_function(f, &fun.to_string(), false, args)?;
+                fmt_function(f, &fun.to_string(), false, args, false)?;
                 if !partition_by.is_empty() {
                     write!(f, " PARTITION BY {:?}", partition_by)?;
                 }
@@ -1702,9 +1735,9 @@ impl fmt::Debug for Expr {
                 distinct,
                 ref args,
                 ..
-            } => fmt_function(f, &fun.to_string(), *distinct, args),
+            } => fmt_function(f, &fun.to_string(), *distinct, args, true),
             Expr::AggregateUDF { fun, ref args, .. } => {
-                fmt_function(f, &fun.name, false, args)
+                fmt_function(f, &fun.name, false, args, false)
             }
             Expr::Between {
                 expr,
@@ -1762,7 +1795,7 @@ fn create_name(e: &Expr, input_schema: &DFSchema) -> Result<String> {
         Expr::BinaryExpr { left, op, right } => {
             let left = create_name(left, input_schema)?;
             let right = create_name(right, input_schema)?;
-            Ok(format!("{} {:?} {}", left, op, right))
+            Ok(format!("{} {} {}", left, op, right))
         }
         Expr::Case {
             expr,
@@ -1925,12 +1958,12 @@ mod tests {
         assert_eq!(
             rewriter.v,
             vec![
-                "Previsited #state Eq Utf8(\"CO\")",
+                "Previsited #state = Utf8(\"CO\")",
                 "Previsited #state",
                 "Mutated #state",
                 "Previsited Utf8(\"CO\")",
                 "Mutated Utf8(\"CO\")",
-                "Mutated #state Eq Utf8(\"CO\")"
+                "Mutated #state = Utf8(\"CO\")"
             ]
         )
     }

datafusion/src/logical_plan/operators.rs

@@ -129,19 +129,19 @@ mod tests {
     fn test_operators() {
         assert_eq!(
             format!("{:?}", lit(1u32) + lit(2u32)),
-            "UInt32(1) Plus UInt32(2)"
+            "UInt32(1) + UInt32(2)"
         );
         assert_eq!(
             format!("{:?}", lit(1u32) - lit(2u32)),
-            "UInt32(1) Minus UInt32(2)"
+            "UInt32(1) - UInt32(2)"
         );
         assert_eq!(
             format!("{:?}", lit(1u32) * lit(2u32)),
-            "UInt32(1) Multiply UInt32(2)"
+            "UInt32(1) * UInt32(2)"
         );
         assert_eq!(
             format!("{:?}", lit(1u32) / lit(2u32)),
-            "UInt32(1) Divide UInt32(2)"
+            "UInt32(1) / UInt32(2)"
         );
     }
 }

datafusion/src/logical_plan/plan.rs

@@ -551,7 +551,7 @@ impl LogicalPlan {
     /// // Format using display_indent
     /// let display_string = format!("{}", plan.display_indent());
     ///
-    /// assert_eq!("Filter: #foo_csv.id Eq Int32(5)\
+    /// assert_eq!("Filter: #foo_csv.id = Int32(5)\
     ///              \n  TableScan: foo_csv projection=None",
     ///             display_string);
     /// ```
@@ -575,7 +575,7 @@ impl LogicalPlan {
     ///
     /// ```text
     /// Projection: #employee.id [id:Int32]\
-    ///    Filter: #employee.state Eq Utf8(\"CO\") [id:Int32, state:Utf8]\
+    ///    Filter: #employee.state = Utf8(\"CO\") [id:Int32, state:Utf8]\
     ///      TableScan: employee projection=Some([0, 3]) [id:Int32, state:Utf8]";
     /// ```
     ///
@@ -592,7 +592,7 @@ impl LogicalPlan {
     /// // Format using display_indent_schema
     /// let display_string = format!("{}", plan.display_indent_schema());
     ///
-    /// assert_eq!("Filter: #foo_csv.id Eq Int32(5) [id:Int32]\
+    /// assert_eq!("Filter: #foo_csv.id = Int32(5) [id:Int32]\
     ///             \n  TableScan: foo_csv projection=None [id:Int32]",
     ///             display_string);
     /// ```
@@ -939,7 +939,7 @@ mod tests {
         let plan = display_plan();
 
         let expected = "Projection: #employee_csv.id\
-        \n  Filter: #employee_csv.state Eq Utf8(\"CO\")\
+        \n  Filter: #employee_csv.state = Utf8(\"CO\")\
         \n    TableScan: employee_csv projection=Some([0, 3])";
 
         assert_eq!(expected, format!("{}", plan.display_indent()));
@@ -950,7 +950,7 @@ mod tests {
         let plan = display_plan();
 
         let expected = "Projection: #employee_csv.id [id:Int32]\
-                        \n  Filter: #employee_csv.state Eq Utf8(\"CO\") [id:Int32, state:Utf8]\
+                        \n  Filter: #employee_csv.state = Utf8(\"CO\") [id:Int32, state:Utf8]\
                         \n    TableScan: employee_csv projection=Some([0, 3]) [id:Int32, state:Utf8]";
 
         assert_eq!(expected, format!("{}", plan.display_indent_schema()));

datafusion/src/optimizer/common_subexpr_eliminate.rs

@@ -714,8 +714,8 @@ mod test {
             )?
             .build()?;
 
-        let expected = "Aggregate: groupBy=[[]], aggr=[[SUM(#BinaryExpr-*BinaryExpr--Column-test.bLiteral1Column-test.a AS test.a Multiply Int32(1) Minus test.b), SUM(#BinaryExpr-*BinaryExpr--Column-test.bLiteral1Column-test.a AS test.a Multiply Int32(1) Minus test.b Multiply Int32(1) Plus #test.c)]]\
-        \n  Projection: #test.a Multiply Int32(1) Minus #test.b AS BinaryExpr-*BinaryExpr--Column-test.bLiteral1Column-test.a, #test.a, #test.b, #test.c\
+        let expected = "Aggregate: groupBy=[[]], aggr=[[SUM(#BinaryExpr-*BinaryExpr--Column-test.bLiteral1Column-test.a AS test.a * Int32(1) - test.b), SUM(#BinaryExpr-*BinaryExpr--Column-test.bLiteral1Column-test.a AS test.a * Int32(1) - test.b * Int32(1) + #test.c)]]\
+        \n  Projection: #test.a * Int32(1) - #test.b AS BinaryExpr-*BinaryExpr--Column-test.bLiteral1Column-test.a, #test.a, #test.b, #test.c\
         \n    TableScan: test projection=None";
 
         assert_optimized_plan_eq(&plan, expected);
@@ -737,7 +737,7 @@ mod test {
             )?
             .build()?;
 
-        let expected = "Aggregate: groupBy=[[]], aggr=[[Int32(1) Plus #AggregateFunction-AVGfalseColumn-test.a AS AVG(test.a), Int32(1) Minus #AggregateFunction-AVGfalseColumn-test.a AS AVG(test.a)]]\
+        let expected = "Aggregate: groupBy=[[]], aggr=[[Int32(1) + #AggregateFunction-AVGfalseColumn-test.a AS AVG(test.a), Int32(1) - #AggregateFunction-AVGfalseColumn-test.a AS AVG(test.a)]]\
         \n  Projection: AVG(#test.a) AS AggregateFunction-AVGfalseColumn-test.a, #test.a, #test.b, #test.c\
         \n    TableScan: test projection=None";
 
@@ -757,8 +757,8 @@ mod test {
             ])?
             .build()?;
 
-        let expected = "Projection: #BinaryExpr-+Column-test.aLiteral1 AS Int32(1) Plus test.a AS first, #BinaryExpr-+Column-test.aLiteral1 AS Int32(1) Plus test.a AS second\
-        \n  Projection: Int32(1) Plus #test.a AS BinaryExpr-+Column-test.aLiteral1, #test.a, #test.b, #test.c\
+        let expected = "Projection: #BinaryExpr-+Column-test.aLiteral1 AS Int32(1) + test.a AS first, #BinaryExpr-+Column-test.aLiteral1 AS Int32(1) + test.a AS second\
+        \n  Projection: Int32(1) + #test.a AS BinaryExpr-+Column-test.aLiteral1, #test.a, #test.b, #test.c\
         \n    TableScan: test projection=None";
 
         assert_optimized_plan_eq(&plan, expected);
@@ -777,7 +777,7 @@ mod test {
             ])?
             .build()?;
 
-        let expected = "Projection: Int32(1) Plus #test.a, #test.a Plus Int32(1)\
+        let expected = "Projection: Int32(1) + #test.a, #test.a + Int32(1)\
         \n  TableScan: test projection=None";
 
         assert_optimized_plan_eq(&plan, expected);
@@ -794,8 +794,8 @@ mod test {
             .project(vec![binary_expr(lit(1), Operator::Plus, col("a"))])?
             .build()?;
 
-        let expected = "Projection: #Int32(1) Plus test.a\
-        \n  Projection: Int32(1) Plus #test.a\
+        let expected = "Projection: #Int32(1) + test.a\
+        \n  Projection: Int32(1) + #test.a\
         \n    TableScan: test projection=None";
 
         assert_optimized_plan_eq(&plan, expected);

datafusion/src/optimizer/constant_folding.rs

@@ -592,7 +592,7 @@ mod tests {
 
         let expected = "\
         Projection: #test.a\
-        \n  Filter: NOT #test.b And #test.c\
+        \n  Filter: NOT #test.b AND #test.c\
         \n    TableScan: test projection=None";
 
         assert_optimized_plan_eq(&plan, expected);
@@ -609,7 +609,7 @@ mod tests {
 
         let expected = "\
         Projection: #test.a\
-        \n  Filter: NOT #test.b Or NOT #test.c\
+        \n  Filter: NOT #test.b OR NOT #test.c\
         \n    TableScan: test projection=None";
 
         assert_optimized_plan_eq(&plan, expected);