feat: show statistics in explain verbose #8113

NGA-TRAN · 2023-11-09T22:58:52Z

Which issue does this PR close?

Closes ##8111

Rationale for this change

Always show statistics in explain verbose to get used by other DB such as IOx that do not want to use setting set datafusion.explain.show_statistics = true;

What changes are included in this PR?

Always show statistics in explain verbose

Are these changes tested?

Yes

Are there any user-facing changes?

Explain verbose now includes statistics

NGA-TRAN · 2023-11-09T22:59:53Z

@alamb : Instead of adding lines with stats, I just add stats to all physical plans which I think more reasonable. Let me know what you think

CC @berkaysynnada

berkaysynnada · 2023-11-10T07:06:16Z

I think it makes sense, thank you.

alamb

Thanks @NGA-TRAN and @berkaysynnada I think adding statistics to all explain plans is going to make verbose plans almost unreadable (they are already pretty hard to read), especially as we improve the statistics over time -- like imagine a plan with 100 columns after #8112 -- it will be almost impossible to read the verbose output

I would like to suggest rather than adding statistics to every plan in explain verbose, instead add only a single new line

Perhaps by adding a new entry in addition to

https://github.com/apache/arrow-datafusion/blob/fdf3f6c3304956cd56131d8783d7cb38a2242a9f/datafusion/core/src/physical_planner.rs#L1897-L1901

if verbose {
   // add initial physical plan and its statistics in verbose mode
   stringified_plans.push( 
     displayable(input.as_ref()) 
         .set_show_statistics(true) 
         .to_stringified(e.verbose, InitialPhysicalPlanWithStats), 
 );

alamb · 2023-11-10T21:16:13Z

Something like this perhaps: #8111 (comment)

NGA-TRAN · 2023-11-10T21:21:43Z

Something like this perhaps: #8111 (comment)

Sounds good. I will add stats for initial and final physical plans only

NGA-TRAN · 2023-11-10T22:01:24Z

datafusion/proto/src/logical_plan/to_proto.rs

+                // We do not want to add proto plan type for these two becasue they are just the same as
+                // InitialPhysicalPlan and FinalPhysicalPlan
+                datafusion_expr::PlanType::InitialPhysicalPlanWithStats
+                | datafusion_expr::PlanType::FinalPhysicalPlanWithStats => todo!(),


@alamb Cargo suggested me to do this. Do you want to go with this or create 2 new types in photo enum?

I think adding two new types in the proto would be the most consistent with the existing code

However, as long as this doesn't panic I think it would be ok to merge -- for example, if it returned an internal error.

I don't really understand why there is special serialization for this to be honest 🤔 maybe we could simplify that too as a follow on PR

@alamb The todo! returns a panic of not yet implemented. Let me try to add new types to proto

I have add 2 new types to proto instead

NGA-TRAN · 2023-11-10T22:04:00Z

datafusion/sqllogictest/test_files/explain.slt

-set datafusion.execution.collect_statistics = false;
+# explain verbose with both collect & show statistics on
+query TT
+EXPLAIN VERBOSE SELECT * FROM alltypes_plain limit 10;


All plans have stats because show_stats is on. This works the same as before this PR

NGA-TRAN · 2023-11-10T22:04:33Z

datafusion/sqllogictest/test_files/explain.slt

+
+# explain verbose with collect on and & show statistics off: still has stats
+query TT
+EXPLAIN VERBOSE SELECT * FROM alltypes_plain limit 10;


2 new lines added for this one because show_stats is off

NGA-TRAN · 2023-11-10T22:05:03Z

@alamb I have addressed your comments but I have a new question in the code

alamb

Thanks @NGA-TRAN -- this looks good to me except for the panic in proto serialization.

alamb · 2023-11-10T23:23:20Z

datafusion/proto/src/logical_plan/to_proto.rs

+                // We do not want to add proto plan type for these two becasue they are just the same as
+                // InitialPhysicalPlan and FinalPhysicalPlan
+                datafusion_expr::PlanType::InitialPhysicalPlanWithStats
+                | datafusion_expr::PlanType::FinalPhysicalPlanWithStats => todo!(),


I think adding two new types in the proto would be the most consistent with the existing code

However, as long as this doesn't panic I think it would be ok to merge -- for example, if it returned an internal error.

I don't really understand why there is special serialization for this to be honest 🤔 maybe we could simplify that too as a follow on PR

alamb · 2023-11-10T23:23:29Z

datafusion/sqllogictest/test_files/explain.slt

@@ -245,6 +245,7 @@ logical_plan after eliminate_projection SAME TEXT AS ABOVE
 logical_plan after push_down_limit SAME TEXT AS ABOVE
 logical_plan TableScan: simple_explain_test projection=[a, b, c]
 initial_physical_plan CsvExec: file_groups={1 group: [[WORKSPACE_ROOT/datafusion/core/tests/data/example.csv]]}, projection=[a, b, c], has_header=true
+initial_physical_plan_with_stats CsvExec: file_groups={1 group: [[WORKSPACE_ROOT/datafusion/core/tests/data/example.csv]]}, projection=[a, b, c], has_header=true, statistics=[Rows=Absent, Bytes=Absent]


alamb · 2023-11-13T16:35:45Z

datafusion/core/src/physical_planner.rs

@@ -1900,6 +1900,18 @@ impl DefaultPhysicalPlanner {
                                .to_stringified(e.verbose, InitialPhysicalPlan),
                        );

+                        // Add plan with stats for verbose case even if show_statistics is false


This comment seems confusing as the next line checks for !config.show_statistics.

I have made the comments clearer

alamb · 2023-11-13T21:36:45Z

Looks like the CI failures are due to a logical merge conflict with #8112 -- I'll push an update

alamb

Thanks again @NGA-TRAN !

feat: show statistics in explain verbose

e3e6bc2

github-actions bot added core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Nov 9, 2023

alamb reviewed Nov 10, 2023

View reviewed changes

chore: address review comments

354d0a2

NGA-TRAN commented Nov 10, 2023

View reviewed changes

NGA-TRAN requested a review from alamb November 10, 2023 22:04

alamb reviewed Nov 10, 2023

View reviewed changes

alamb reviewed Nov 13, 2023

View reviewed changes

NGA-TRAN added 3 commits November 13, 2023 13:42

chore: address review comments

20b04f5

fix: add new enum types in pbjson

1e78ad2

fix: add new types into message proto

99c9f04

alamb added 2 commits November 13, 2023 16:37

Merge remote-tracking branch 'apache/main' into ntran/verbose_stats

729535b

Update explain plan

74952a7

alamb approved these changes Nov 13, 2023

View reviewed changes

alamb merged commit a38ac20 into apache:main Nov 13, 2023
22 checks passed

matthewgapp mentioned this pull request Jan 11, 2024

matt/feat/recursive ctes/config flag matthewgapp/arrow-datafusion#3

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: show statistics in explain verbose #8113

feat: show statistics in explain verbose #8113

NGA-TRAN commented Nov 9, 2023

NGA-TRAN commented Nov 9, 2023

berkaysynnada commented Nov 10, 2023

alamb left a comment •

edited

Loading

alamb commented Nov 10, 2023

NGA-TRAN commented Nov 10, 2023

NGA-TRAN Nov 10, 2023

alamb Nov 10, 2023

NGA-TRAN Nov 13, 2023

NGA-TRAN Nov 13, 2023

NGA-TRAN Nov 10, 2023

NGA-TRAN Nov 10, 2023

NGA-TRAN commented Nov 10, 2023

alamb left a comment

alamb Nov 10, 2023

alamb Nov 10, 2023

alamb Nov 13, 2023

NGA-TRAN Nov 13, 2023

alamb commented Nov 13, 2023

alamb left a comment

feat: show statistics in explain verbose #8113

feat: show statistics in explain verbose #8113

Conversation

NGA-TRAN commented Nov 9, 2023

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

NGA-TRAN commented Nov 9, 2023

berkaysynnada commented Nov 10, 2023

alamb left a comment • edited Loading

Choose a reason for hiding this comment

alamb commented Nov 10, 2023

NGA-TRAN commented Nov 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NGA-TRAN commented Nov 10, 2023

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Nov 13, 2023

alamb left a comment

Choose a reason for hiding this comment

alamb left a comment •

edited

Loading