You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/05/27 19:56:12 UTC

[GitHub] [arrow-datafusion] NGA-TRAN opened a new pull request #434: test: display of each query plan during plan generation

NGA-TRAN opened a new pull request #434:
URL: https://github.com/apache/arrow-datafusion/pull/434


   # Which issue does this PR close?
   
   <!--
   We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
   -->
   
   Closes #.
   
    # Rationale for this change
   <!--
    Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
    Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.  
   -->
   
   # What changes are included in this PR?
   @alamb In order for me to fix [IOx's issue 1538](https://github.com/influxdata/influxdb_iox/issues/1538) and Datafusion[ #430](https://github.com/apache/arrow-datafusion/issues/430), I want to know what is the right expectation for each query plan in debug and what functions we should use during debug. It seems to me the debug display functions do not display enough information yet.
   
   # Are there any user-facing changes?
   This PR includes test for my understanding only. No code changes
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [arrow-datafusion] NGA-TRAN commented on a change in pull request #434: test: display of each query plan during plan generation

Posted by GitBox <gi...@apache.org>.
NGA-TRAN commented on a change in pull request #434:
URL: https://github.com/apache/arrow-datafusion/pull/434#discussion_r640958296



##########
File path: datafusion/tests/sql.rs
##########
@@ -1573,6 +1575,101 @@ async fn csv_explain() {
     assert_eq!(expected, actual);
 }
 
+#[tokio::test]
+async fn csv_explain_plans() {
+    // This test verify the look of each plan in its full cycle plan creation
+
+    let mut ctx = ExecutionContext::new();
+    register_aggregate_csv_by_sql(&mut ctx).await;
+    let sql = "EXPLAIN SELECT c1 FROM aggregate_test_100 where c2 > 10";
+
+    // Logical plan
+    // Create plan
+    let msg = format!("Creating logical plan for '{}'", sql);
+    let plan = ctx.create_logical_plan(&sql).expect(&msg);
+    let logical_schema = plan.schema();
+    // 
+    println!("SQL: {}", sql);
+    // Verify schema
+    let expected = "Explain [plan_type:Utf8, plan:Utf8]";
+    let actual = format!("{}", plan.display_indent_schema());
+    assert_eq!(expected, actual);
+    // Verify the text format of the plan
+    let expected = "Explain";

Review comment:
       Agree. I actually made this happen in a different branch but want to verify with you first. Will bring it in here




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [arrow-datafusion] andygrove commented on pull request #434: test: display of each query plan during plan generation

Posted by GitBox <gi...@apache.org>.
andygrove commented on pull request #434:
URL: https://github.com/apache/arrow-datafusion/pull/434#issuecomment-849936594


   > I think including the input to the `Explain` node makes sense. Any thoughts @Dandandan or @andygrove ?
   
   Makes sense to me


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [arrow-datafusion] alamb commented on a change in pull request #434: fix: display the content of debug explain

Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #434:
URL: https://github.com/apache/arrow-datafusion/pull/434#discussion_r641819260



##########
File path: datafusion/tests/sql.rs
##########
@@ -1573,6 +1575,173 @@ async fn csv_explain() {
     assert_eq!(expected, actual);
 }
 
+#[tokio::test]
+async fn csv_explain_plans() {
+    // This test verify the look of each plan in its full cycle plan creation
+
+    let mut ctx = ExecutionContext::new();
+    register_aggregate_csv_by_sql(&mut ctx).await;
+    let sql = "EXPLAIN SELECT c1 FROM aggregate_test_100 where c2 > 10";
+
+    // Logical plan
+    // Create plan
+    let msg = format!("Creating logical plan for '{}'", sql);
+    let plan = ctx.create_logical_plan(&sql).expect(&msg);
+    let logical_schema = plan.schema();
+    //
+    println!("SQL: {}", sql);
+    //
+    // Verify schema
+    let expected = "Explain [plan_type:Utf8, plan:Utf8]\n
+      Projection: #c1 [c1:Utf8]\n
+          Filter: #c2 Gt Int64(10) [c1:Utf8, c2:Int32, c3:Int16, c4:Int16, c5:Int32, c6:Int64, c7:Int16, c8:Int32, c9:Int64, c10:Utf8, c11:Float32, c12:Float64, c13:Utf8]\n
+                TableScan: aggregate_test_100 projection=None [c1:Utf8, c2:Int32, c3:Int16, c4:Int16, c5:Int32, c6:Int64, c7:Int16, c8:Int32, c9:Int64, c10:Utf8, c11:Float32, c12:Float64, c13:Utf8]";
+    let actual = format!("{}", plan.display_indent_schema());
+    assert_eq!(
+        expected
+            .replace("\t", "")
+            .replace("\n", "")
+            .replace(" ", ""),
+        actual.replace("\t", "").replace("\n", "").replace(" ", "")
+    );
+    //
+    // Verify the text format of the plan
+    let expected = "Explain\n  Projection: #c1\n
+        Filter: #c2 Gt Int64(10)\n
+              TableScan: aggregate_test_100 projection=None";
+    let actual = format!("{}", plan.display_indent());
+    assert_eq!(
+        expected
+            .replace("\t", "")
+            .replace("\n", "")
+            .replace(" ", ""),
+        actual.replace("\t", "").replace("\n", "").replace(" ", "")
+    );
+    //
+    // verify the grahviz format of the plan
+    let expected = "// Begin DataFusion GraphViz Plan (see https://graphviz.org)\n
+    digraph {\n
+          subgraph cluster_1\n
+            {\n
+                graph[label=\"LogicalPlan\"]\n
+                    2[shape=box label=\"Explain\"]\n
+                    3[shape=box label=\"Projection: #c1\"]\n
+                    2 -> 3 [arrowhead=none, arrowtail=normal, dir=back]\n
+                    4[shape=box label=\"Filter: #c2 Gt Int64(10)\"]\n
+                    3 -> 4 [arrowhead=none, arrowtail=normal, dir=back]\n
+                    5[shape=box label=\"TableScan: aggregate_test_100 projection=None\"]\n
+                    4 -> 5 [arrowhead=none, arrowtail=normal, dir=back]\n
+            }\n
+         subgraph cluster_6\n
+            {\n
+                graph[label=\"Detailed LogicalPlan\"]\n
+                    7[shape=box label=\"Explain\\nSchema: [plan_type:Utf8, plan:Utf8]\"]\n
+                    8[shape=box label=\"Projection: #c1\\nSchema: [c1:Utf8]\"]\n
+                    7 -> 8 [arrowhead=none, arrowtail=normal, dir=back]\n
+                    9[shape=box label=\"Filter: #c2 Gt Int64(10)\\nSchema: [c1:Utf8, c2:Int32, c3:Int16, c4:Int16, c5:Int32, c6:Int64, c7:Int16, c8:Int32, c9:Int64, c10:Utf8, c11:Float32, c12:Float64, c13:Utf8]\"]\n
+                    8 -> 9 [arrowhead=none, arrowtail=normal, dir=back]\n
+                    10[shape=box label=\"TableScan: aggregate_test_100 projection=None\\nSchema: [c1:Utf8, c2:Int32, c3:Int16, c4:Int16, c5:Int32, c6:Int64, c7:Int16, c8:Int32, c9:Int64, c10:Utf8, c11:Float32, c12:Float64, c13:Utf8]\"]\n
+                    9 -> 10 [arrowhead=none, arrowtail=normal, dir=back]\n
+            }\n
+    }\n
+    // End DataFusion GraphViz Plan\n";
+    let actual = format!("{}", plan.display_graphviz());
+    assert_eq!(
+        expected
+            .replace("\t", "")
+            .replace("\n", "")
+            .replace(" ", ""),
+        actual.replace("\t", "").replace("\n", "").replace(" ", "")
+    );
+
+    // Optimized logical plan
+    //
+    let msg = format!("Optimizing logical plan for '{}': {:?}", sql, plan);
+    let plan = ctx.optimize(&plan).expect(&msg);
+    let optimized_logical_schema = plan.schema();
+    // Both schema has to be the same
+    assert_eq!(logical_schema.as_ref(), optimized_logical_schema.as_ref());
+    //
+    // Verify schema
+    let expected = "Explain [plan_type:Utf8, plan:Utf8]\n
+      Projection: #c1 [c1:Utf8]\n
+          Filter: #c2 Gt Int64(10) [c1:Utf8, c2:Int32]\n
+                TableScan: aggregate_test_100 projection=Some([0, 1]) [c1:Utf8, c2:Int32]";
+    let actual = format!("{}", plan.display_indent_schema());
+    assert_eq!(

Review comment:
       These tests look somewhat challenging to maintain. What do you think about the following pattern (mostly copied from `assert_batches_eq!`:
   
   ```rust
   let expected_lines = vec![
     "Explain [plan_type:Utf8, plan:Utf8]",
     "  Projection: #c1 [c1:Utf8]",
     ...
    ];
   
   let formatted = plan.display_indent_schema().to_string();
   let actual_lines: Vec<&str> = formatted.trim().lines().collect();
   
   assert_eq!(
     expected_lines, actual_lines,
     "\n\nexpected:\n\n{:#?}\nactual:\n\n{:#?}\n\n",
     expected_lines, actual_lines
   );
   ```

##########
File path: datafusion/src/logical_plan/plan.rs
##########
@@ -329,11 +329,12 @@ impl LogicalPlan {
             LogicalPlan::Limit { input, .. } => vec![input],
             LogicalPlan::Extension { node } => node.inputs(),
             LogicalPlan::Union { inputs, .. } => inputs.iter().collect(),
+            LogicalPlan::Explain { plan, .. } => vec![plan],
             // plans without inputs
             LogicalPlan::TableScan { .. }
             | LogicalPlan::EmptyRelation { .. }
-            | LogicalPlan::CreateExternalTable { .. }
-            | LogicalPlan::Explain { .. } => vec![],
+            //| LogicalPlan::Explain { .. }

Review comment:
       ```suggestion
   ```
   
   I think we can remove this line entirely (no need to leave it in)




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [arrow-datafusion] NGA-TRAN commented on a change in pull request #434: test: display of each query plan during plan generation

Posted by GitBox <gi...@apache.org>.
NGA-TRAN commented on a change in pull request #434:
URL: https://github.com/apache/arrow-datafusion/pull/434#discussion_r640926298



##########
File path: datafusion/tests/sql.rs
##########
@@ -1573,6 +1575,101 @@ async fn csv_explain() {
     assert_eq!(expected, actual);
 }
 
+#[tokio::test]
+async fn csv_explain_plans() {
+    // This test verify the look of each plan in its full cycle plan creation
+
+    let mut ctx = ExecutionContext::new();
+    register_aggregate_csv_by_sql(&mut ctx).await;
+    let sql = "EXPLAIN SELECT c1 FROM aggregate_test_100 where c2 > 10";
+
+    // Logical plan
+    // Create plan
+    let msg = format!("Creating logical plan for '{}'", sql);
+    let plan = ctx.create_logical_plan(&sql).expect(&msg);
+    let logical_schema = plan.schema();
+    // 
+    println!("SQL: {}", sql);
+    // Verify schema
+    let expected = "Explain [plan_type:Utf8, plan:Utf8]";
+    let actual = format!("{}", plan.display_indent_schema());
+    assert_eq!(expected, actual);
+    // Verify the text format of the plan
+    let expected = "Explain";

Review comment:
       Currently, the output of the logical plan for explain only incldues the word "Explain". Should we add the plan of the explain's select?

##########
File path: datafusion/tests/sql.rs
##########
@@ -1573,6 +1575,101 @@ async fn csv_explain() {
     assert_eq!(expected, actual);
 }
 
+#[tokio::test]
+async fn csv_explain_plans() {
+    // This test verify the look of each plan in its full cycle plan creation
+
+    let mut ctx = ExecutionContext::new();
+    register_aggregate_csv_by_sql(&mut ctx).await;
+    let sql = "EXPLAIN SELECT c1 FROM aggregate_test_100 where c2 > 10";
+
+    // Logical plan
+    // Create plan
+    let msg = format!("Creating logical plan for '{}'", sql);
+    let plan = ctx.create_logical_plan(&sql).expect(&msg);
+    let logical_schema = plan.schema();
+    // 
+    println!("SQL: {}", sql);
+    // Verify schema
+    let expected = "Explain [plan_type:Utf8, plan:Utf8]";
+    let actual = format!("{}", plan.display_indent_schema());
+    assert_eq!(expected, actual);
+    // Verify the text format of the plan
+    let expected = "Explain";
+    let actual = format!("{}", plan.display_indent());
+    assert_eq!(expected, actual);
+    // verify the grahviz format of the plan
+    let expected = "// Begin DataFusion GraphViz Plan (see https://graphviz.org)\n
+    digraph {\n
+      subgraph cluster_1\n
+      {\n
+        graph[label=\"LogicalPlan\"]\n
+        2[shape=box label=\"Explain\"]\n
+      }\n
+      subgraph cluster_3\n
+      {\n
+        graph[label=\"Detailed LogicalPlan\"]\n
+        4[shape=box label=\"Explain\\nSchema: [plan_type:Utf8, plan:Utf8]\"]\n
+      }\n
+    }\n
+    // End DataFusion GraphViz Plan";

Review comment:
       Similarly, the graphviz has nothing but Explain and the explain's schema node. Should it includes full explain of its plan's nodes?
   

##########
File path: datafusion/tests/sql.rs
##########
@@ -1573,6 +1575,101 @@ async fn csv_explain() {
     assert_eq!(expected, actual);
 }
 
+#[tokio::test]
+async fn csv_explain_plans() {
+    // This test verify the look of each plan in its full cycle plan creation
+
+    let mut ctx = ExecutionContext::new();
+    register_aggregate_csv_by_sql(&mut ctx).await;
+    let sql = "EXPLAIN SELECT c1 FROM aggregate_test_100 where c2 > 10";
+
+    // Logical plan
+    // Create plan
+    let msg = format!("Creating logical plan for '{}'", sql);
+    let plan = ctx.create_logical_plan(&sql).expect(&msg);
+    let logical_schema = plan.schema();
+    // 
+    println!("SQL: {}", sql);
+    // Verify schema
+    let expected = "Explain [plan_type:Utf8, plan:Utf8]";
+    let actual = format!("{}", plan.display_indent_schema());
+    assert_eq!(expected, actual);
+    // Verify the text format of the plan
+    let expected = "Explain";
+    let actual = format!("{}", plan.display_indent());
+    assert_eq!(expected, actual);
+    // verify the grahviz format of the plan
+    let expected = "// Begin DataFusion GraphViz Plan (see https://graphviz.org)\n
+    digraph {\n
+      subgraph cluster_1\n
+      {\n
+        graph[label=\"LogicalPlan\"]\n
+        2[shape=box label=\"Explain\"]\n
+      }\n
+      subgraph cluster_3\n
+      {\n
+        graph[label=\"Detailed LogicalPlan\"]\n
+        4[shape=box label=\"Explain\\nSchema: [plan_type:Utf8, plan:Utf8]\"]\n
+      }\n
+    }\n
+    // End DataFusion GraphViz Plan";
+    let actual = format!("{}", plan.display_graphviz());
+    assert_eq!(expected.replace("\t","").replace("\n","").replace(" ",""), actual.replace("\t","").replace("\n","").replace(" ",""));
+    // Optimized logical plan
+    //
+    let msg = format!("Optimizing logical plan for '{}': {:?}", sql, plan);
+    let plan = ctx.optimize(&plan).expect(&msg);
+    let optimized_logical_schema = plan.schema();
+    // Both schema has to be the same
+    assert_eq!(logical_schema.as_ref(), optimized_logical_schema.as_ref());
+    // Verify schema
+    let expected = "Explain [plan_type:Utf8, plan:Utf8]";
+    let actual = format!("{}", plan.display_indent_schema());
+    assert_eq!(expected, actual);
+    // Verify the text format of the plan
+    let expected = "Explain";
+    let actual = format!("{}", plan.display_indent());
+    assert_eq!(expected, actual);
+    // verify the grahviz format of the plan
+    let expected = "// Begin DataFusion GraphViz Plan (see https://graphviz.org)\n
+    digraph {\n
+      subgraph cluster_1\n
+      {\n
+        graph[label=\"LogicalPlan\"]\n
+        2[shape=box label=\"Explain\"]\n
+      }\n
+      subgraph cluster_3\n
+      {\n
+        graph[label=\"Detailed LogicalPlan\"]\n
+        4[shape=box label=\"Explain\\nSchema: [plan_type:Utf8, plan:Utf8]\"]\n
+      }\n
+    }\n
+    // End DataFusion GraphViz Plan";
+    let actual = format!("{}", plan.display_graphviz());
+    assert_eq!(expected.replace("\t","").replace("\n","").replace(" ",""), actual.replace("\t","").replace("\n","").replace(" ",""));
+
+    // Physical plan
+    // Create plan
+    let msg = format!("Creating physical plan for '{}': {:?}", sql, plan);
+    let plan = ctx.create_physical_plan(&plan).expect(&msg);
+    // Verify the text format of the plan
+    let expected = "ExplainExec";
+    let actual = format!("{}", displayable(plan.as_ref()).indent());
+    assert_eq!(expected.replace("\t","").replace("\n","").replace(" ",""), actual.replace("\t","").replace("\n","").replace(" ",""));
+
+    // Execute plan
+    let msg = format!("Executing physical plan for '{}': {:?}", sql, plan);
+    let results = collect(plan).await.expect(&msg);
+    // Compare final explain result from execution output
+    let expected = vec![
+        vec!["logical_plan",
+             "Projection: #c1\n  Filter: #c2 Gt Int64(10)\n    TableScan: aggregate_test_100 projection=None"]];

Review comment:
       The useful info only available after execute the EXPLAIN. Should everything above include this information?

##########
File path: datafusion/tests/sql.rs
##########
@@ -1573,6 +1575,101 @@ async fn csv_explain() {
     assert_eq!(expected, actual);
 }
 
+#[tokio::test]
+async fn csv_explain_plans() {
+    // This test verify the look of each plan in its full cycle plan creation
+
+    let mut ctx = ExecutionContext::new();
+    register_aggregate_csv_by_sql(&mut ctx).await;
+    let sql = "EXPLAIN SELECT c1 FROM aggregate_test_100 where c2 > 10";
+
+    // Logical plan
+    // Create plan
+    let msg = format!("Creating logical plan for '{}'", sql);
+    let plan = ctx.create_logical_plan(&sql).expect(&msg);
+    let logical_schema = plan.schema();
+    // 
+    println!("SQL: {}", sql);
+    // Verify schema
+    let expected = "Explain [plan_type:Utf8, plan:Utf8]";
+    let actual = format!("{}", plan.display_indent_schema());
+    assert_eq!(expected, actual);
+    // Verify the text format of the plan
+    let expected = "Explain";
+    let actual = format!("{}", plan.display_indent());
+    assert_eq!(expected, actual);
+    // verify the grahviz format of the plan
+    let expected = "// Begin DataFusion GraphViz Plan (see https://graphviz.org)\n
+    digraph {\n
+      subgraph cluster_1\n
+      {\n
+        graph[label=\"LogicalPlan\"]\n
+        2[shape=box label=\"Explain\"]\n
+      }\n
+      subgraph cluster_3\n
+      {\n
+        graph[label=\"Detailed LogicalPlan\"]\n
+        4[shape=box label=\"Explain\\nSchema: [plan_type:Utf8, plan:Utf8]\"]\n
+      }\n
+    }\n
+    // End DataFusion GraphViz Plan";
+    let actual = format!("{}", plan.display_graphviz());
+    assert_eq!(expected.replace("\t","").replace("\n","").replace(" ",""), actual.replace("\t","").replace("\n","").replace(" ",""));
+    // Optimized logical plan
+    //
+    let msg = format!("Optimizing logical plan for '{}': {:?}", sql, plan);
+    let plan = ctx.optimize(&plan).expect(&msg);
+    let optimized_logical_schema = plan.schema();
+    // Both schema has to be the same
+    assert_eq!(logical_schema.as_ref(), optimized_logical_schema.as_ref());
+    // Verify schema
+    let expected = "Explain [plan_type:Utf8, plan:Utf8]";
+    let actual = format!("{}", plan.display_indent_schema());
+    assert_eq!(expected, actual);
+    // Verify the text format of the plan
+    let expected = "Explain";
+    let actual = format!("{}", plan.display_indent());
+    assert_eq!(expected, actual);
+    // verify the grahviz format of the plan
+    let expected = "// Begin DataFusion GraphViz Plan (see https://graphviz.org)\n
+    digraph {\n
+      subgraph cluster_1\n
+      {\n
+        graph[label=\"LogicalPlan\"]\n
+        2[shape=box label=\"Explain\"]\n
+      }\n
+      subgraph cluster_3\n
+      {\n
+        graph[label=\"Detailed LogicalPlan\"]\n
+        4[shape=box label=\"Explain\\nSchema: [plan_type:Utf8, plan:Utf8]\"]\n
+      }\n
+    }\n
+    // End DataFusion GraphViz Plan";
+    let actual = format!("{}", plan.display_graphviz());
+    assert_eq!(expected.replace("\t","").replace("\n","").replace(" ",""), actual.replace("\t","").replace("\n","").replace(" ",""));
+
+    // Physical plan
+    // Create plan
+    let msg = format!("Creating physical plan for '{}': {:?}", sql, plan);
+    let plan = ctx.create_physical_plan(&plan).expect(&msg);
+    // Verify the text format of the plan
+    let expected = "ExplainExec";

Review comment:
       Similarly, display of physical plan has no useful info at all

##########
File path: datafusion/tests/sql.rs
##########
@@ -1591,6 +1688,105 @@ async fn csv_explain_verbose() {
     assert!(actual.contains("#c2 Gt Int64(10)"), "Actual: '{}'", actual);
 }
 
+#[tokio::test]
+async fn csv_explain_verbose_plans() {

Review comment:
       Same behavior for explain verbose

##########
File path: datafusion/tests/sql.rs
##########
@@ -1573,6 +1575,101 @@ async fn csv_explain() {
     assert_eq!(expected, actual);
 }
 
+#[tokio::test]
+async fn csv_explain_plans() {
+    // This test verify the look of each plan in its full cycle plan creation
+
+    let mut ctx = ExecutionContext::new();
+    register_aggregate_csv_by_sql(&mut ctx).await;
+    let sql = "EXPLAIN SELECT c1 FROM aggregate_test_100 where c2 > 10";
+
+    // Logical plan
+    // Create plan
+    let msg = format!("Creating logical plan for '{}'", sql);
+    let plan = ctx.create_logical_plan(&sql).expect(&msg);
+    let logical_schema = plan.schema();
+    // 
+    println!("SQL: {}", sql);
+    // Verify schema
+    let expected = "Explain [plan_type:Utf8, plan:Utf8]";
+    let actual = format!("{}", plan.display_indent_schema());
+    assert_eq!(expected, actual);
+    // Verify the text format of the plan
+    let expected = "Explain";
+    let actual = format!("{}", plan.display_indent());
+    assert_eq!(expected, actual);
+    // verify the grahviz format of the plan
+    let expected = "// Begin DataFusion GraphViz Plan (see https://graphviz.org)\n
+    digraph {\n
+      subgraph cluster_1\n
+      {\n
+        graph[label=\"LogicalPlan\"]\n
+        2[shape=box label=\"Explain\"]\n
+      }\n
+      subgraph cluster_3\n
+      {\n
+        graph[label=\"Detailed LogicalPlan\"]\n
+        4[shape=box label=\"Explain\\nSchema: [plan_type:Utf8, plan:Utf8]\"]\n
+      }\n
+    }\n
+    // End DataFusion GraphViz Plan";
+    let actual = format!("{}", plan.display_graphviz());
+    assert_eq!(expected.replace("\t","").replace("\n","").replace(" ",""), actual.replace("\t","").replace("\n","").replace(" ",""));
+    // Optimized logical plan
+    //
+    let msg = format!("Optimizing logical plan for '{}': {:?}", sql, plan);
+    let plan = ctx.optimize(&plan).expect(&msg);
+    let optimized_logical_schema = plan.schema();
+    // Both schema has to be the same
+    assert_eq!(logical_schema.as_ref(), optimized_logical_schema.as_ref());
+    // Verify schema
+    let expected = "Explain [plan_type:Utf8, plan:Utf8]";
+    let actual = format!("{}", plan.display_indent_schema());
+    assert_eq!(expected, actual);
+    // Verify the text format of the plan
+    let expected = "Explain";
+    let actual = format!("{}", plan.display_indent());
+    assert_eq!(expected, actual);
+    // verify the grahviz format of the plan
+    let expected = "// Begin DataFusion GraphViz Plan (see https://graphviz.org)\n
+    digraph {\n
+      subgraph cluster_1\n
+      {\n
+        graph[label=\"LogicalPlan\"]\n
+        2[shape=box label=\"Explain\"]\n
+      }\n
+      subgraph cluster_3\n
+      {\n
+        graph[label=\"Detailed LogicalPlan\"]\n
+        4[shape=box label=\"Explain\\nSchema: [plan_type:Utf8, plan:Utf8]\"]\n
+      }\n
+    }\n

Review comment:
       Similarly, graphviz does not have any useful info

##########
File path: datafusion/tests/sql.rs
##########
@@ -1573,6 +1575,101 @@ async fn csv_explain() {
     assert_eq!(expected, actual);
 }
 
+#[tokio::test]
+async fn csv_explain_plans() {
+    // This test verify the look of each plan in its full cycle plan creation
+
+    let mut ctx = ExecutionContext::new();
+    register_aggregate_csv_by_sql(&mut ctx).await;
+    let sql = "EXPLAIN SELECT c1 FROM aggregate_test_100 where c2 > 10";
+
+    // Logical plan
+    // Create plan
+    let msg = format!("Creating logical plan for '{}'", sql);
+    let plan = ctx.create_logical_plan(&sql).expect(&msg);
+    let logical_schema = plan.schema();
+    // 
+    println!("SQL: {}", sql);
+    // Verify schema
+    let expected = "Explain [plan_type:Utf8, plan:Utf8]";
+    let actual = format!("{}", plan.display_indent_schema());
+    assert_eq!(expected, actual);
+    // Verify the text format of the plan
+    let expected = "Explain";
+    let actual = format!("{}", plan.display_indent());
+    assert_eq!(expected, actual);
+    // verify the grahviz format of the plan
+    let expected = "// Begin DataFusion GraphViz Plan (see https://graphviz.org)\n
+    digraph {\n
+      subgraph cluster_1\n
+      {\n
+        graph[label=\"LogicalPlan\"]\n
+        2[shape=box label=\"Explain\"]\n
+      }\n
+      subgraph cluster_3\n
+      {\n
+        graph[label=\"Detailed LogicalPlan\"]\n
+        4[shape=box label=\"Explain\\nSchema: [plan_type:Utf8, plan:Utf8]\"]\n
+      }\n
+    }\n
+    // End DataFusion GraphViz Plan";
+    let actual = format!("{}", plan.display_graphviz());
+    assert_eq!(expected.replace("\t","").replace("\n","").replace(" ",""), actual.replace("\t","").replace("\n","").replace(" ",""));
+    // Optimized logical plan
+    //
+    let msg = format!("Optimizing logical plan for '{}': {:?}", sql, plan);
+    let plan = ctx.optimize(&plan).expect(&msg);
+    let optimized_logical_schema = plan.schema();
+    // Both schema has to be the same
+    assert_eq!(logical_schema.as_ref(), optimized_logical_schema.as_ref());
+    // Verify schema
+    let expected = "Explain [plan_type:Utf8, plan:Utf8]";
+    let actual = format!("{}", plan.display_indent_schema());
+    assert_eq!(expected, actual);
+    // Verify the text format of the plan
+    let expected = "Explain";

Review comment:
       Similarly, display of optimized logical plan has almost nothing in there




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [arrow-datafusion] NGA-TRAN commented on pull request #434: fix: display the content of debug explain

Posted by GitBox <gi...@apache.org>.
NGA-TRAN commented on pull request #434:
URL: https://github.com/apache/arrow-datafusion/pull/434#issuecomment-852397463


   @alamb Finally all checks have passed :)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [arrow-datafusion] NGA-TRAN commented on pull request #434: fix: display the content of debug explain

Posted by GitBox <gi...@apache.org>.
NGA-TRAN commented on pull request #434:
URL: https://github.com/apache/arrow-datafusion/pull/434#issuecomment-852328005


   | @NGA-TRAN it looks like this may have failed on windows: https://github.com/apache/arrow-datafusion/pull/434/checks?check_run_id=2720284528
   
   Silly me. Working on it. Thanks @alamb 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [arrow-datafusion] alamb commented on pull request #434: fix: display the content of debug explain

Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #434:
URL: https://github.com/apache/arrow-datafusion/pull/434#issuecomment-852250670


   @NGA-TRAN  it looks like this may have failed on windows: https://github.com/apache/arrow-datafusion/pull/434/checks?check_run_id=2720284528


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [arrow-datafusion] NGA-TRAN commented on a change in pull request #434: fix: display the content of debug explain

Posted by GitBox <gi...@apache.org>.
NGA-TRAN commented on a change in pull request #434:
URL: https://github.com/apache/arrow-datafusion/pull/434#discussion_r643228053



##########
File path: datafusion/tests/sql.rs
##########
@@ -1573,6 +1575,173 @@ async fn csv_explain() {
     assert_eq!(expected, actual);
 }
 
+#[tokio::test]
+async fn csv_explain_plans() {
+    // This test verify the look of each plan in its full cycle plan creation
+
+    let mut ctx = ExecutionContext::new();
+    register_aggregate_csv_by_sql(&mut ctx).await;
+    let sql = "EXPLAIN SELECT c1 FROM aggregate_test_100 where c2 > 10";
+
+    // Logical plan
+    // Create plan
+    let msg = format!("Creating logical plan for '{}'", sql);
+    let plan = ctx.create_logical_plan(&sql).expect(&msg);
+    let logical_schema = plan.schema();
+    //
+    println!("SQL: {}", sql);
+    //
+    // Verify schema
+    let expected = "Explain [plan_type:Utf8, plan:Utf8]\n
+      Projection: #c1 [c1:Utf8]\n
+          Filter: #c2 Gt Int64(10) [c1:Utf8, c2:Int32, c3:Int16, c4:Int16, c5:Int32, c6:Int64, c7:Int16, c8:Int32, c9:Int64, c10:Utf8, c11:Float32, c12:Float64, c13:Utf8]\n
+                TableScan: aggregate_test_100 projection=None [c1:Utf8, c2:Int32, c3:Int16, c4:Int16, c5:Int32, c6:Int64, c7:Int16, c8:Int32, c9:Int64, c10:Utf8, c11:Float32, c12:Float64, c13:Utf8]";
+    let actual = format!("{}", plan.display_indent_schema());
+    assert_eq!(
+        expected
+            .replace("\t", "")
+            .replace("\n", "")
+            .replace(" ", ""),
+        actual.replace("\t", "").replace("\n", "").replace(" ", "")
+    );
+    //
+    // Verify the text format of the plan
+    let expected = "Explain\n  Projection: #c1\n
+        Filter: #c2 Gt Int64(10)\n
+              TableScan: aggregate_test_100 projection=None";
+    let actual = format!("{}", plan.display_indent());
+    assert_eq!(
+        expected
+            .replace("\t", "")
+            .replace("\n", "")
+            .replace(" ", ""),
+        actual.replace("\t", "").replace("\n", "").replace(" ", "")
+    );
+    //
+    // verify the grahviz format of the plan
+    let expected = "// Begin DataFusion GraphViz Plan (see https://graphviz.org)\n
+    digraph {\n
+          subgraph cluster_1\n
+            {\n
+                graph[label=\"LogicalPlan\"]\n
+                    2[shape=box label=\"Explain\"]\n
+                    3[shape=box label=\"Projection: #c1\"]\n
+                    2 -> 3 [arrowhead=none, arrowtail=normal, dir=back]\n
+                    4[shape=box label=\"Filter: #c2 Gt Int64(10)\"]\n
+                    3 -> 4 [arrowhead=none, arrowtail=normal, dir=back]\n
+                    5[shape=box label=\"TableScan: aggregate_test_100 projection=None\"]\n
+                    4 -> 5 [arrowhead=none, arrowtail=normal, dir=back]\n
+            }\n
+         subgraph cluster_6\n
+            {\n
+                graph[label=\"Detailed LogicalPlan\"]\n
+                    7[shape=box label=\"Explain\\nSchema: [plan_type:Utf8, plan:Utf8]\"]\n
+                    8[shape=box label=\"Projection: #c1\\nSchema: [c1:Utf8]\"]\n
+                    7 -> 8 [arrowhead=none, arrowtail=normal, dir=back]\n
+                    9[shape=box label=\"Filter: #c2 Gt Int64(10)\\nSchema: [c1:Utf8, c2:Int32, c3:Int16, c4:Int16, c5:Int32, c6:Int64, c7:Int16, c8:Int32, c9:Int64, c10:Utf8, c11:Float32, c12:Float64, c13:Utf8]\"]\n
+                    8 -> 9 [arrowhead=none, arrowtail=normal, dir=back]\n
+                    10[shape=box label=\"TableScan: aggregate_test_100 projection=None\\nSchema: [c1:Utf8, c2:Int32, c3:Int16, c4:Int16, c5:Int32, c6:Int64, c7:Int16, c8:Int32, c9:Int64, c10:Utf8, c11:Float32, c12:Float64, c13:Utf8]\"]\n
+                    9 -> 10 [arrowhead=none, arrowtail=normal, dir=back]\n
+            }\n
+    }\n
+    // End DataFusion GraphViz Plan\n";
+    let actual = format!("{}", plan.display_graphviz());
+    assert_eq!(
+        expected
+            .replace("\t", "")
+            .replace("\n", "")
+            .replace(" ", ""),
+        actual.replace("\t", "").replace("\n", "").replace(" ", "")
+    );
+
+    // Optimized logical plan
+    //
+    let msg = format!("Optimizing logical plan for '{}': {:?}", sql, plan);
+    let plan = ctx.optimize(&plan).expect(&msg);
+    let optimized_logical_schema = plan.schema();
+    // Both schema has to be the same
+    assert_eq!(logical_schema.as_ref(), optimized_logical_schema.as_ref());
+    //
+    // Verify schema
+    let expected = "Explain [plan_type:Utf8, plan:Utf8]\n
+      Projection: #c1 [c1:Utf8]\n
+          Filter: #c2 Gt Int64(10) [c1:Utf8, c2:Int32]\n
+                TableScan: aggregate_test_100 projection=Some([0, 1]) [c1:Utf8, c2:Int32]";
+    let actual = format!("{}", plan.display_indent_schema());
+    assert_eq!(

Review comment:
       Addressed




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [arrow-datafusion] NGA-TRAN commented on a change in pull request #434: test: display of each query plan during plan generation

Posted by GitBox <gi...@apache.org>.
NGA-TRAN commented on a change in pull request #434:
URL: https://github.com/apache/arrow-datafusion/pull/434#discussion_r641767736



##########
File path: datafusion/src/logical_plan/plan.rs
##########
@@ -438,11 +439,11 @@ impl LogicalPlan {
                 }
                 true
             }
+            LogicalPlan::Explain { plan, .. } => plan.accept(visitor)?,

Review comment:
       This is the fix that enables LogicalPlan's `display_...` functions to show all info of the plan. The newly added tests capture this fix.

##########
File path: datafusion/src/logical_plan/plan.rs
##########
@@ -329,11 +329,12 @@ impl LogicalPlan {
             LogicalPlan::Limit { input, .. } => vec![input],
             LogicalPlan::Extension { node } => node.inputs(),
             LogicalPlan::Union { inputs, .. } => inputs.iter().collect(),
+            LogicalPlan::Explain { plan, .. } => vec![plan],

Review comment:
       This is the fix that progpagates the optimization down the plan explain 

##########
File path: datafusion/src/optimizer/filter_push_down.rs
##########
@@ -234,6 +234,10 @@ fn split_members<'a>(predicate: &'a Expr, predicates: &mut Vec<&'a Expr>) {
 
 fn optimize(plan: &LogicalPlan, mut state: State) -> Result<LogicalPlan> {
     match plan {
+        LogicalPlan::Explain { .. } => {
+            // push the optimization to the plan of this explain
+            push_down(&state, plan)
+        }

Review comment:
       This is related to the fix to propagate the optimization down




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [arrow-datafusion] codecov-commenter commented on pull request #434: fix: display the content of debug explain

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #434:
URL: https://github.com/apache/arrow-datafusion/pull/434#issuecomment-852363880


   # [Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/434?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#434](https://codecov.io/gh/apache/arrow-datafusion/pull/434?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (373a347) into [master](https://codecov.io/gh/apache/arrow-datafusion/commit/6c050b896215717233dafe441b5c08ae40baf174?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6c050b8) will **increase** coverage by `0.11%`.
   > The diff coverage is `88.79%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/arrow-datafusion/pull/434/graphs/tree.svg?width=650&height=150&src=pr&token=JXwWBKD3D9&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/arrow-datafusion/pull/434?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #434      +/-   ##
   ==========================================
   + Coverage   75.72%   75.84%   +0.11%     
   ==========================================
     Files         143      153      +10     
     Lines       23910    25872    +1962     
   ==========================================
   + Hits        18107    19622    +1515     
   - Misses       5803     6250     +447     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/arrow-datafusion/pull/434?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [datafusion/src/logical\_plan/plan.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/434/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvbG9naWNhbF9wbGFuL3BsYW4ucnM=) | `80.62% <50.00%> (-0.58%)` | :arrow_down: |
   | [datafusion/tests/sql.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/434/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi90ZXN0cy9zcWwucnM=) | `99.27% <90.00%> (-0.62%)` | :arrow_down: |
   | [datafusion/src/optimizer/filter\_push\_down.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/434/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvb3B0aW1pemVyL2ZpbHRlcl9wdXNoX2Rvd24ucnM=) | `97.79% <100.00%> (+0.05%)` | :arrow_up: |
   | [datafusion-cli/src/print\_format.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/434/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi1jbGkvc3JjL3ByaW50X2Zvcm1hdC5ycw==) | `81.25% <0.00%> (-9.17%)` | :arrow_down: |
   | [...ta/rust/core/src/serde/physical\_plan/from\_proto.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/434/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmFsbGlzdGEvcnVzdC9jb3JlL3NyYy9zZXJkZS9waHlzaWNhbF9wbGFuL2Zyb21fcHJvdG8ucnM=) | `39.78% <0.00%> (-7.62%)` | :arrow_down: |
   | [datafusion/src/physical\_plan/mod.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/434/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9tb2QucnM=) | `78.70% <0.00%> (-6.01%)` | :arrow_down: |
   | [...lista/rust/core/src/serde/logical\_plan/to\_proto.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/434/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmFsbGlzdGEvcnVzdC9jb3JlL3NyYy9zZXJkZS9sb2dpY2FsX3BsYW4vdG9fcHJvdG8ucnM=) | `62.40% <0.00%> (-5.43%)` | :arrow_down: |
   | [...sta/rust/core/src/serde/logical\_plan/from\_proto.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/434/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmFsbGlzdGEvcnVzdC9jb3JlL3NyYy9zZXJkZS9sb2dpY2FsX3BsYW4vZnJvbV9wcm90by5ycw==) | `36.22% <0.00%> (-3.97%)` | :arrow_down: |
   | [datafusion/src/physical\_plan/parquet.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/434/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9wYXJxdWV0LnJz) | `82.19% <0.00%> (-3.61%)` | :arrow_down: |
   | [datafusion/src/logical\_plan/operators.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/434/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvbG9naWNhbF9wbGFuL29wZXJhdG9ycy5ycw==) | `77.14% <0.00%> (-2.86%)` | :arrow_down: |
   | ... and [57 more](https://codecov.io/gh/apache/arrow-datafusion/pull/434/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/434?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/434?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [6c050b8...373a347](https://codecov.io/gh/apache/arrow-datafusion/pull/434?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [arrow-datafusion] alamb commented on a change in pull request #434: test: display of each query plan during plan generation

Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #434:
URL: https://github.com/apache/arrow-datafusion/pull/434#discussion_r640931526



##########
File path: datafusion/tests/sql.rs
##########
@@ -1573,6 +1575,101 @@ async fn csv_explain() {
     assert_eq!(expected, actual);
 }
 
+#[tokio::test]
+async fn csv_explain_plans() {
+    // This test verify the look of each plan in its full cycle plan creation
+
+    let mut ctx = ExecutionContext::new();
+    register_aggregate_csv_by_sql(&mut ctx).await;
+    let sql = "EXPLAIN SELECT c1 FROM aggregate_test_100 where c2 > 10";
+
+    // Logical plan
+    // Create plan
+    let msg = format!("Creating logical plan for '{}'", sql);
+    let plan = ctx.create_logical_plan(&sql).expect(&msg);
+    let logical_schema = plan.schema();
+    // 
+    println!("SQL: {}", sql);
+    // Verify schema
+    let expected = "Explain [plan_type:Utf8, plan:Utf8]";
+    let actual = format!("{}", plan.display_indent_schema());
+    assert_eq!(expected, actual);
+    // Verify the text format of the plan
+    let expected = "Explain";

Review comment:
       I think including the plan tree like
   ```
   Explain
     Project
       Filter
         Scan
   ```
   
   Makes more sense than the current output of:
   ```
   Explain
   ```
   

##########
File path: datafusion/tests/sql.rs
##########
@@ -1573,6 +1575,101 @@ async fn csv_explain() {
     assert_eq!(expected, actual);
 }
 
+#[tokio::test]
+async fn csv_explain_plans() {
+    // This test verify the look of each plan in its full cycle plan creation
+
+    let mut ctx = ExecutionContext::new();
+    register_aggregate_csv_by_sql(&mut ctx).await;
+    let sql = "EXPLAIN SELECT c1 FROM aggregate_test_100 where c2 > 10";
+
+    // Logical plan
+    // Create plan
+    let msg = format!("Creating logical plan for '{}'", sql);
+    let plan = ctx.create_logical_plan(&sql).expect(&msg);
+    let logical_schema = plan.schema();
+    // 
+    println!("SQL: {}", sql);
+    // Verify schema
+    let expected = "Explain [plan_type:Utf8, plan:Utf8]";
+    let actual = format!("{}", plan.display_indent_schema());
+    assert_eq!(expected, actual);
+    // Verify the text format of the plan
+    let expected = "Explain";
+    let actual = format!("{}", plan.display_indent());
+    assert_eq!(expected, actual);
+    // verify the grahviz format of the plan
+    let expected = "// Begin DataFusion GraphViz Plan (see https://graphviz.org)\n
+    digraph {\n
+      subgraph cluster_1\n
+      {\n
+        graph[label=\"LogicalPlan\"]\n
+        2[shape=box label=\"Explain\"]\n
+      }\n
+      subgraph cluster_3\n
+      {\n
+        graph[label=\"Detailed LogicalPlan\"]\n
+        4[shape=box label=\"Explain\\nSchema: [plan_type:Utf8, plan:Utf8]\"]\n
+      }\n
+    }\n
+    // End DataFusion GraphViz Plan";

Review comment:
       I think it should include the actual plan that is being explained




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [arrow-datafusion] alamb merged pull request #434: fix: display the content of debug explain

Posted by GitBox <gi...@apache.org>.
alamb merged pull request #434:
URL: https://github.com/apache/arrow-datafusion/pull/434


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org