kosiew commented on code in PR #23651:
URL: https://github.com/apache/datafusion/pull/23651#discussion_r3613348524
##########
datafusion/core/src/physical_planner.rs:
##########
@@ -2680,6 +2684,7 @@ impl DefaultPhysicalPlanner {
InitialPhysicalPlan,
displayable(input.as_ref())
Review Comment:
The EXPLAIN indent path now repeats the same display setup for the initial,
per-optimizer, final, and verbose-with-stats plans. This looks correct, but it
could be easy for a future change to update one branch and miss another. Would
it be worth adding a small local helper or closure, perhaps
`render_physical_plan(plan, show_statistics, show_schema)`, to apply the
registry and common flags in one place? The schema-only branches could continue
to omit the registry as they do now.
##########
datafusion/physical-plan/src/display.rs:
##########
@@ -154,6 +156,7 @@ impl<'a> DisplayableExecutionPlan<'a> {
pub fn new(inner: &'a dyn ExecutionPlan) -> Self {
Review Comment:
`DisplayableExecutionPlan::new`, `with_metrics`, and `with_full_metrics` all
repeat the same default field initialization and only differ in `show_metrics`.
Would it make sense to add a private constructor such as `fn
with_show_metrics(inner, show_metrics) -> Self`? That would keep the public API
unchanged and make future display options, like this registry field, easier to
add consistently.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
