fix: display the content of debug explain #434

NGA-TRAN · 2021-05-27T19:56:05Z

Which issue does this PR close?

Closes #430

Rationale for this change

What changes are included in this PR?

There are 2 main changes:

Fix the display of all LogicalPlan's display_ functions to have them display the plan content for not only executing plans but also the explain ones
Propagate the optimization down the plan of for the explain

Are there any user-facing changes?

. Number 1 won't because they are currently used for debug only
. Number 2 will help other TableProvider to push their predicate down correctly (ref: https://github.com/influxdata/influxdb_iox/issues/1538)

NGA-TRAN · 2021-05-27T19:57:44Z

datafusion/tests/sql.rs

+    let actual = format!("{}", plan.display_indent_schema());
+    assert_eq!(expected, actual);
+    // Verify the text format of the plan
+    let expected = "Explain";


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

I think including the plan tree like

Explain Project Filter Scan

Makes more sense than the current output of:

Explain

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

NGA-TRAN · 2021-05-27T19:58:31Z

datafusion/tests/sql.rs

+        4[shape=box label=\"Explain\\nSchema: [plan_type:Utf8, plan:Utf8]\"]\n
+      }\n
+    }\n
+    // End DataFusion GraphViz Plan";


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

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

NGA-TRAN · 2021-05-27T19:59:45Z

datafusion/tests/sql.rs

+    let actual = format!("{}", plan.display_indent_schema());
+    assert_eq!(expected, actual);
+    // Verify the text format of the plan
+    let expected = "Explain";


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

NGA-TRAN · 2021-05-27T20:00:10Z

datafusion/tests/sql.rs

+        graph[label=\"Detailed LogicalPlan\"]\n
+        4[shape=box label=\"Explain\\nSchema: [plan_type:Utf8, plan:Utf8]\"]\n
+      }\n
+    }\n


Similarly, graphviz does not have any useful info

NGA-TRAN · 2021-05-27T20:00:54Z

datafusion/tests/sql.rs

+    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";


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

datafusion/tests/sql.rs

alamb

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

alamb · 2021-05-27T20:07:22Z

datafusion/tests/sql.rs

+    let actual = format!("{}", plan.display_indent_schema());
+    assert_eq!(expected, actual);
+    // Verify the text format of the plan
+    let expected = "Explain";


I think including the plan tree like

Explain Project Filter Scan

Makes more sense than the current output of:

Explain

alamb · 2021-05-27T20:07:38Z

datafusion/tests/sql.rs

+        4[shape=box label=\"Explain\\nSchema: [plan_type:Utf8, plan:Utf8]\"]\n
+      }\n
+    }\n
+    // End DataFusion GraphViz Plan";


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

andygrove · 2021-05-27T20:57:28Z

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

Makes sense to me

…down

NGA-TRAN · 2021-05-28T19:13:43Z

datafusion/src/logical_plan/plan.rs

                }
                true
            }
+            LogicalPlan::Explain { plan, .. } => plan.accept(visitor)?,


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

NGA-TRAN · 2021-05-28T19:15:37Z

datafusion/src/logical_plan/plan.rs

            LogicalPlan::Limit { input, .. } => vec![input],
            LogicalPlan::Extension { node } => node.inputs(),
            LogicalPlan::Union { inputs, .. } => inputs.iter().collect(),
+            LogicalPlan::Explain { plan, .. } => vec![plan],


This is the fix that progpagates the optimization down the plan explain

NGA-TRAN · 2021-05-28T19:16:38Z

datafusion/src/optimizer/filter_push_down.rs

+        LogicalPlan::Explain { .. } => {
+            // push the optimization to the plan of this explain
+            push_down(&state, plan)
+        }


This is related to the fix to propagate the optimization down

alamb

The code changes look good to me @NGA-TRAN -- thank you.

I had a suggestion on how to make the tests perhaps easier to maintain (and easier to see what was different)

alamb · 2021-05-28T21:09:18Z

datafusion/src/logical_plan/plan.rs

            | LogicalPlan::EmptyRelation { .. }
-            | LogicalPlan::CreateExternalTable { .. }
-            | LogicalPlan::Explain { .. } => vec![],
+            //| LogicalPlan::Explain { .. }


Suggested change

//| LogicalPlan::Explain { .. }

I think we can remove this line entirely (no need to leave it in)

alamb · 2021-05-28T21:17:42Z

datafusion/tests/sql.rs

+          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!(


These tests look somewhat challenging to maintain. What do you think about the following pattern (mostly copied from assert_batches_eq!:

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 );

NGA-TRAN

Thanks @alamb for your review comments. I have addressed them.

NGA-TRAN · 2021-06-01T15:45:42Z

datafusion/tests/sql.rs

+          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!(


alamb

I think this is looking great @NGA-TRAN -- thank you

jorgecarleitao

LGTM.

Impressive testing, @NGA-TRAN 💯

alamb · 2021-06-01T16:11:06Z

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

NGA-TRAN · 2021-06-01T17:51:07Z

| @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

codecov-commenter · 2021-06-01T18:50:15Z

Codecov Report

Merging #434 (373a347) into master (6c050b8) will increase coverage by 0.11%.
The diff coverage is 88.79%.

@@            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	Coverage Δ
datafusion/src/logical_plan/plan.rs	`80.62% <50.00%> (-0.58%)`	⬇️
datafusion/tests/sql.rs	`99.27% <90.00%> (-0.62%)`	⬇️
datafusion/src/optimizer/filter_push_down.rs	`97.79% <100.00%> (+0.05%)`	⬆️
datafusion-cli/src/print_format.rs	`81.25% <0.00%> (-9.17%)`	⬇️
...ta/rust/core/src/serde/physical_plan/from_proto.rs	`39.78% <0.00%> (-7.62%)`	⬇️
datafusion/src/physical_plan/mod.rs	`78.70% <0.00%> (-6.01%)`	⬇️
...lista/rust/core/src/serde/logical_plan/to_proto.rs	`62.40% <0.00%> (-5.43%)`	⬇️
...sta/rust/core/src/serde/logical_plan/from_proto.rs	`36.22% <0.00%> (-3.97%)`	⬇️
datafusion/src/physical_plan/parquet.rs	`82.19% <0.00%> (-3.61%)`	⬇️
datafusion/src/logical_plan/operators.rs	`77.14% <0.00%> (-2.86%)`	⬇️
... and 57 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c050b8...373a347. Read the comment docs.

NGA-TRAN · 2021-06-01T19:42:03Z

@alamb Finally all checks have passed :)

test: display of each plan

124ec3f

NGA-TRAN commented May 27, 2021

View reviewed changes

alamb reviewed May 27, 2021

View reviewed changes

fix: Fix debug display of explain and potential fix of predicate push…

1926591

…down

NGA-TRAN commented May 28, 2021

View reviewed changes

NGA-TRAN changed the title ~~test: display of each query plan during plan generation~~ fix: display the content of debug explain May 28, 2021

alamb approved these changes May 28, 2021

View reviewed changes

refactor: adddress Andrew's comments

9dbd9f1

NGA-TRAN commented Jun 1, 2021

View reviewed changes

alamb approved these changes Jun 1, 2021

View reviewed changes

jorgecarleitao approved these changes Jun 1, 2021

View reviewed changes

fix: avoid comparing file path that are test environmentally dependant

373a347

alamb merged commit 1601112 into apache:master Jun 1, 2021

houqp added datafusion enhancement New feature or request labels Jul 30, 2021

H0TB0X420 pushed a commit to H0TB0X420/datafusion that referenced this pull request Oct 7, 2025

Upgrade to 28.0.0-rc1 (apache#434)

5793db3

fix: display the content of debug explain #434

fix: display the content of debug explain #434

Uh oh!

Conversation

NGA-TRAN commented May 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andygrove commented May 27, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NGA-TRAN left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

jorgecarleitao left a comment

Choose a reason for hiding this comment

Uh oh!

alamb commented Jun 1, 2021

Uh oh!

NGA-TRAN commented Jun 1, 2021

Uh oh!

codecov-commenter commented Jun 1, 2021

Codecov Report

Uh oh!

NGA-TRAN commented Jun 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

NGA-TRAN commented May 27, 2021 •

edited

Loading