[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #6711: Display all partitions and files in EXPLAIN VERBOSE

["alamb (via GitHub)" <gi...@apache.org>]

alamb commented on code in PR #6711:
URL: https://github.com/apache/arrow-datafusion/pull/6711#discussion_r1234109250


##########
datafusion/core/src/physical_plan/display.rs:
##########
@@ -30,6 +30,8 @@ use super::{accept, ExecutionPlan, ExecutionPlanVisitor};
 pub enum DisplayFormatType {
     /// Default, compact format. Example: `FilterExec: c12 < 10.0`
     Default,
+    /// Verbose, showing all available details

Review Comment:
   👍 



##########
datafusion/substrait/tests/cases/roundtrip_physical_plan.rs:
##########
@@ -66,8 +66,11 @@ async fn parquet_exec() -> Result<()> {
         consumer::from_substrait_rel(&mut ctx, substrait_rel.as_ref(), &HashMap::new())
             .await?;
 
-    let expected = format!("{}", displayable(parquet_exec.as_ref()).indent());
-    let actual = format!("{}", displayable(parquet_exec_roundtrip.as_ref()).indent());
+    let expected = format!("{}", displayable(parquet_exec.as_ref()).indent(true));

Review Comment:
   💯  for including verbose 



##########
datafusion/core/src/physical_plan/display.rs:
##########
@@ -192,11 +209,29 @@ impl<'a, 'b> ExecutionPlanVisitor for IndentVisitor<'a, 'b> {
     }
 }
 
-impl<'a> ToStringifiedPlan for DisplayableExecutionPlan<'a> {
-    fn to_stringified(
-        &self,
-        plan_type: crate::logical_expr::PlanType,
-    ) -> StringifiedPlan {
-        StringifiedPlan::new(plan_type, self.indent().to_string())
+/// Trait for types which could have additional details when formatted in `Verbose` mode

Review Comment:
   🤔  maybe eventually we can combine this into `ExecutionPlan` so that everything that implemented `ExecutionPlan` would also need to implement `DisplayAs`. 
   
   That would reduce the duplication across the traits, howe er, it would be an API change and result in non trivial code churn so I think we can consider it as part of a follow on PR
   
   ```
   trait ExecutionPlan: DisplayAs {
   ...
   }
   ```



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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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