alamb commented on a change in pull request #337:
URL: https://github.com/apache/arrow-datafusion/pull/337#discussion_r631961359
##########
File path: datafusion/src/logical_plan/display.rs
##########
@@ -58,8 +52,7 @@ impl<'a, 'b> PlanVisitor for IndentVisitor<'a, 'b> {
if self.indent > 0 {
writeln!(self.f)?;
}
- self.write_indent()?;
-
+ write!(self.f, "{:indent$}", "", indent = self.indent * 2)?;
Review comment:
this is a better way to make indents that I found while googling around
##########
File path: datafusion/src/physical_plan/display.rs
##########
@@ -0,0 +1,73 @@
+//! Implementation of physical plan display. See
+//! [`crate::physical_plan::displayable`] for examples of how to
+//! format
+
+use std::fmt;
+
+use super::{accept, ExecutionPlan, ExecutionPlanVisitor};
+
+/// Options for controlling how each [`ExecutionPlan`] should format itself
+#[derive(Debug, Clone, Copy)]
+pub enum DisplayFormatType {
+ /// Default, compact format. Example: `FilterExec: c12 < 10.0`
+ Default,
Review comment:
I envision adding more types (e.g. Graphviz) as needs evolve
##########
File path: datafusion/src/physical_plan/mod.rs
##########
@@ -152,6 +162,133 @@ pub trait ExecutionPlan: Debug + Send + Sync {
fn metrics(&self) -> HashMap<String, SQLMetric> {
HashMap::new()
}
+
+ /// Format this `ExecutionPlan` to `f` in the specified type.
+ ///
+ /// Should not include a newline
+ ///
+ /// Note this function prints a placeholder by default to preserve
+ /// backwards compatibility.
+ fn fmt_as(&self, _t: DisplayFormatType, f: &mut fmt::Formatter) ->
fmt::Result {
+ write!(f, "ExecutionPlan(PlaceHolder)")
+ }
+}
+
+/// Return a [wrapper](DisplayableExecutionPlan) around an
Review comment:
This is the main proposed interface: `displayable` which returns a
`struct` which then has several ways to display it. It would be ideal if I
could add this to `ExecutionPlan` directly itself, but since it is a trait this
was the best I could come up with (along with a bunch of documentation)
##########
File path: datafusion/tests/sql.rs
##########
@@ -2885,3 +2885,45 @@ async fn test_cast_expressions_error() -> Result<()> {
Ok(())
}
+
+#[tokio::test]
+async fn test_physical_plan_display_indent() {
+ let mut ctx = ExecutionContext::new();
+ register_aggregate_csv(&mut ctx).unwrap();
Review comment:
I thought one end to end test would be reasonable to make sure the
output was ok and that it didn't regress, but also wouldn't take too much
effort to maintain
--
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:
[email protected]
