From 11fe6b5d584e8d235c90a6f6da946d1e985a63a5 Mon Sep 17 00:00:00 2001
From: Andrew Lamb <andrew@nerdnetworks.org>
Date: Mon, 26 Jul 2021 11:20:22 -0400
Subject: [PATCH 01/17] Improve SQLMetric APIs, port existing metrics

---
 .../src/physical_optimizer/repartition.rs     |  13 +-
 datafusion/src/physical_plan/analyze.rs       |   8 +
 datafusion/src/physical_plan/display.rs       |  69 ++-
 .../src/physical_plan/hash_aggregate.rs       |  33 +-
 datafusion/src/physical_plan/hash_join.rs     | 100 ++--
 datafusion/src/physical_plan/metrics.rs       | 461 ++++++++++++++++++
 .../src/physical_plan/metrics/wrappers.rs     | 105 ++++
 datafusion/src/physical_plan/mod.rs           | 111 +----
 datafusion/src/physical_plan/parquet.rs       | 126 +++--
 datafusion/src/physical_plan/repartition.rs   |  90 ++--
 datafusion/src/physical_plan/sort.rs          |  45 +-
 datafusion/tests/parquet_pruning.rs           |  67 +--
 datafusion/tests/sql.rs                       |   2 +
 13 files changed, 886 insertions(+), 344 deletions(-)
 create mode 100644 datafusion/src/physical_plan/metrics.rs
 create mode 100644 datafusion/src/physical_plan/metrics/wrappers.rs

diff --git a/datafusion/src/physical_optimizer/repartition.rs b/datafusion/src/physical_optimizer/repartition.rs
index 4504c81daa06d..4b1a536b2f124 100644
--- a/datafusion/src/physical_optimizer/repartition.rs
+++ b/datafusion/src/physical_optimizer/repartition.rs
@@ -110,24 +110,25 @@ mod tests {
 
     use super::*;
     use crate::datasource::datasource::Statistics;
-    use crate::physical_plan::parquet::{
-        ParquetExec, ParquetExecMetrics, ParquetPartition,
-    };
+    use crate::physical_plan::metrics::SharedMetricsSet;
+    use crate::physical_plan::parquet::{ParquetExec, ParquetPartition};
     use crate::physical_plan::projection::ProjectionExec;
 
     #[test]
     fn added_repartition_to_single_partition() -> Result<()> {
         let schema = Arc::new(Schema::empty());
+        let metrics = Arc::new(SharedMetricsSet::new());
         let parquet_project = ProjectionExec::try_new(
             vec![],
             Arc::new(ParquetExec::new(
                 vec![ParquetPartition::new(
                     vec!["x".to_string()],
                     Statistics::default(),
+                    metrics.clone(),
                 )],
                 schema,
                 None,
-                ParquetExecMetrics::new(),
+                metrics,
                 None,
                 2048,
                 None,
@@ -154,6 +155,7 @@ mod tests {
     #[test]
     fn repartition_deepest_node() -> Result<()> {
         let schema = Arc::new(Schema::empty());
+        let metrics = Arc::new(SharedMetricsSet::new());
         let parquet_project = ProjectionExec::try_new(
             vec![],
             Arc::new(ProjectionExec::try_new(
@@ -162,10 +164,11 @@ mod tests {
                     vec![ParquetPartition::new(
                         vec!["x".to_string()],
                         Statistics::default(),
+                        metrics.clone(),
                     )],
                     schema,
                     None,
-                    ParquetExecMetrics::new(),
+                    metrics,
                     None,
                     2048,
                     None,
diff --git a/datafusion/src/physical_plan/analyze.rs b/datafusion/src/physical_plan/analyze.rs
index 36726ad3a7b48..d0125579ace25 100644
--- a/datafusion/src/physical_plan/analyze.rs
+++ b/datafusion/src/physical_plan/analyze.rs
@@ -164,6 +164,14 @@ impl ExecutionPlan for AnalyzeExec {
             // Verbose output
             // TODO make this more sophisticated
             if verbose {
+                type_builder.append_value("Plan with Full Metrics").unwrap();
+
+                let annotated_plan =
+                    DisplayableExecutionPlan::with_full_metrics(captured_input.as_ref())
+                        .indent()
+                        .to_string();
+                plan_builder.append_value(annotated_plan).unwrap();
+
                 type_builder.append_value("Output Rows").unwrap();
                 plan_builder.append_value(total_rows.to_string()).unwrap();
 
diff --git a/datafusion/src/physical_plan/display.rs b/datafusion/src/physical_plan/display.rs
index e251e4ea53db1..5ff99e5f9704f 100644
--- a/datafusion/src/physical_plan/display.rs
+++ b/datafusion/src/physical_plan/display.rs
@@ -35,8 +35,8 @@ pub enum DisplayFormatType {
 /// Wraps an `ExecutionPlan` with various ways to display this plan
 pub struct DisplayableExecutionPlan<'a> {
     inner: &'a dyn ExecutionPlan,
-    /// whether to show metrics or not
-    with_metrics: bool,
+    /// How to show metrics
+    show_metrics: ShowMetrics,
 }
 
 impl<'a> DisplayableExecutionPlan<'a> {
@@ -45,16 +45,27 @@ impl<'a> DisplayableExecutionPlan<'a> {
     pub fn new(inner: &'a dyn ExecutionPlan) -> Self {
         Self {
             inner,
-            with_metrics: false,
+            show_metrics: ShowMetrics::None,
         }
     }
 
     /// Create a wrapper around an [`'ExecutionPlan'] which can be
-    /// pretty printed in a variety of ways
+    /// pretty printed in a variety of ways that also shows aggregated
+    /// metrics
     pub fn with_metrics(inner: &'a dyn ExecutionPlan) -> Self {
         Self {
             inner,
-            with_metrics: true,
+            show_metrics: ShowMetrics::Aggregated,
+        }
+    }
+
+    /// Create a wrapper around an [`'ExecutionPlan'] which can be
+    /// pretty printed in a variety of ways that also shows all low
+    /// level metrics
+    pub fn with_full_metrics(inner: &'a dyn ExecutionPlan) -> Self {
+        Self {
+            inner,
+            show_metrics: ShowMetrics::Full,
         }
     }
 
@@ -71,7 +82,7 @@ impl<'a> DisplayableExecutionPlan<'a> {
     pub fn indent(&self) -> impl fmt::Display + 'a {
         struct Wrapper<'a> {
             plan: &'a dyn ExecutionPlan,
-            with_metrics: bool,
+            show_metrics: ShowMetrics,
         }
         impl<'a> fmt::Display for Wrapper<'a> {
             fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
@@ -80,18 +91,30 @@ impl<'a> DisplayableExecutionPlan<'a> {
                     t,
                     f,
                     indent: 0,
-                    with_metrics: self.with_metrics,
+                    show_metrics: self.show_metrics,
                 };
                 accept(self.plan, &mut visitor)
             }
         }
         Wrapper {
             plan: self.inner,
-            with_metrics: self.with_metrics,
+            show_metrics: self.show_metrics,
         }
     }
 }
 
+#[derive(Debug, Clone, Copy)]
+enum ShowMetrics {
+    /// Do not show any metrics
+    None,
+
+    /// Show aggregrated metrics across partition
+    Aggregated,
+
+    /// Show full per-partition metrics
+    Full,
+}
+
 /// Formats plans with a single line per node.
 struct IndentVisitor<'a, 'b> {
     /// How to format each node
@@ -100,8 +123,8 @@ struct IndentVisitor<'a, 'b> {
     f: &'a mut fmt::Formatter<'b>,
     /// Indent size
     indent: usize,
-    /// whether to show metrics or not
-    with_metrics: bool,
+    /// How to show metrics
+    show_metrics: ShowMetrics,
 }
 
 impl<'a, 'b> ExecutionPlanVisitor for IndentVisitor<'a, 'b> {
@@ -112,16 +135,22 @@ impl<'a, 'b> ExecutionPlanVisitor for IndentVisitor<'a, 'b> {
     ) -> std::result::Result<bool, Self::Error> {
         write!(self.f, "{:indent$}", "", indent = self.indent * 2)?;
         plan.fmt_as(self.t, self.f)?;
-        if self.with_metrics {
-            write!(
-                self.f,
-                ", metrics=[{}]",
-                plan.metrics()
-                    .iter()
-                    .map(|(k, v)| format!("{}={:?}", k, v.value))
-                    .collect::<Vec<_>>()
-                    .join(", ")
-            )?;
+        match self.show_metrics {
+            ShowMetrics::None => {}
+            ShowMetrics::Aggregated => {
+                if let Some(metrics) = plan.metrics() {
+                    write!(self.f, ", metrics=[{}]", metrics.aggregate_by_partition())?;
+                } else {
+                    write!(self.f, ", metrics=[]")?;
+                }
+            }
+            ShowMetrics::Full => {
+                if let Some(metrics) = plan.metrics() {
+                    write!(self.f, ", metrics=[{}]", metrics)?;
+                } else {
+                    write!(self.f, ", metrics=[]")?;
+                }
+            }
         }
         writeln!(self.f)?;
         self.indent += 1;
diff --git a/datafusion/src/physical_plan/hash_aggregate.rs b/datafusion/src/physical_plan/hash_aggregate.rs
index 1c07f61f10cd5..683edbb6ac0e2 100644
--- a/datafusion/src/physical_plan/hash_aggregate.rs
+++ b/datafusion/src/physical_plan/hash_aggregate.rs
@@ -27,13 +27,12 @@ use futures::{
     stream::{Stream, StreamExt},
     Future,
 };
-use hashbrown::HashMap;
 
 use crate::error::{DataFusionError, Result};
 use crate::physical_plan::hash_utils::create_hashes;
 use crate::physical_plan::{
     Accumulator, AggregateExpr, DisplayFormatType, Distribution, ExecutionPlan,
-    Partitioning, PhysicalExpr, SQLMetric,
+    Partitioning, PhysicalExpr,
 };
 use crate::scalar::ScalarValue;
 
@@ -51,6 +50,8 @@ use pin_project_lite::pin_project;
 
 use async_trait::async_trait;
 
+use super::metrics::wrappers::Count;
+use super::metrics::{MetricBuilder, MetricsSet, SharedMetricsSet};
 use super::{expressions::Column, RecordBatchStream, SendableRecordBatchStream};
 
 /// Hash aggregate modes
@@ -86,8 +87,8 @@ pub struct HashAggregateExec {
     /// same as input.schema() but for the final aggregate it will be the same as the input
     /// to the partial aggregate
     input_schema: SchemaRef,
-    /// Metric to track number of output rows
-    output_rows: Arc<SQLMetric>,
+    /// Execution Metrics
+    metrics: SharedMetricsSet,
 }
 
 fn create_schema(
@@ -136,8 +137,6 @@ impl HashAggregateExec {
 
         let schema = Arc::new(schema);
 
-        let output_rows = SQLMetric::counter();
-
         Ok(HashAggregateExec {
             mode,
             group_expr,
@@ -145,7 +144,7 @@ impl HashAggregateExec {
             input,
             schema,
             input_schema,
-            output_rows,
+            metrics: SharedMetricsSet::new(),
         })
     }
 
@@ -209,6 +208,8 @@ impl ExecutionPlan for HashAggregateExec {
         let input = self.input.execute(partition).await?;
         let group_expr = self.group_expr.iter().map(|x| x.0.clone()).collect();
 
+        let output_rows = MetricBuilder::new(&self.metrics).output_rows(partition);
+
         if self.group_expr.is_empty() {
             Ok(Box::pin(HashAggregateStream::new(
                 self.mode,
@@ -223,7 +224,7 @@ impl ExecutionPlan for HashAggregateExec {
                 group_expr,
                 self.aggr_expr.clone(),
                 input,
-                self.output_rows.clone(),
+                output_rows,
             )))
         }
     }
@@ -246,10 +247,8 @@ impl ExecutionPlan for HashAggregateExec {
         }
     }
 
-    fn metrics(&self) -> HashMap<String, SQLMetric> {
-        let mut metrics = HashMap::new();
-        metrics.insert("outputRows".to_owned(), (*self.output_rows).clone());
-        metrics
+    fn metrics(&self) -> Option<MetricsSet> {
+        Some(self.metrics.clone_inner())
     }
 
     fn fmt_as(
@@ -317,7 +316,7 @@ pin_project! {
         #[pin]
         output: futures::channel::oneshot::Receiver<ArrowResult<RecordBatch>>,
         finished: bool,
-        output_rows: Arc<SQLMetric>,
+        output_rows: Count,
     }
 }
 
@@ -526,7 +525,7 @@ impl GroupedHashAggregateStream {
         group_expr: Vec<Arc<dyn PhysicalExpr>>,
         aggr_expr: Vec<Arc<dyn AggregateExpr>>,
         input: SendableRecordBatchStream,
-        output_rows: Arc<SQLMetric>,
+        output_rows: Count,
     ) -> Self {
         let (tx, rx) = futures::channel::oneshot::channel();
 
@@ -1072,9 +1071,9 @@ mod tests {
 
         assert_batches_sorted_eq!(&expected, &result);
 
-        let metrics = merged_aggregate.metrics();
-        let output_rows = metrics.get("outputRows").unwrap();
-        assert_eq!(3, output_rows.value());
+        let metrics = merged_aggregate.metrics().unwrap();
+        let output_rows = metrics.output_rows().unwrap();
+        assert_eq!(3, output_rows);
 
         Ok(())
     }
diff --git a/datafusion/src/physical_plan/hash_join.rs b/datafusion/src/physical_plan/hash_join.rs
index 99708249fc6a7..eb607c17d05b2 100644
--- a/datafusion/src/physical_plan/hash_join.rs
+++ b/datafusion/src/physical_plan/hash_join.rs
@@ -35,7 +35,6 @@ use std::{time::Instant, vec};
 
 use async_trait::async_trait;
 use futures::{Stream, StreamExt, TryStreamExt};
-use hashbrown::HashMap;
 use tokio::sync::Mutex;
 
 use arrow::array::Array;
@@ -51,12 +50,15 @@ use arrow::array::{
 
 use hashbrown::raw::RawTable;
 
-use super::expressions::Column;
 use super::hash_utils::create_hashes;
 use super::{
     coalesce_partitions::CoalescePartitionsExec,
     hash_utils::{build_join_schema, check_join_is_valid, JoinOn},
 };
+use super::{
+    expressions::Column,
+    metrics::{self, MetricBuilder, MetricsSet, SharedMetricsSet},
+};
 use crate::error::{DataFusionError, Result};
 use crate::logical_plan::JoinType;
 
@@ -65,7 +67,7 @@ use super::{
     SendableRecordBatchStream,
 };
 use crate::physical_plan::coalesce_batches::concat_batches;
-use crate::physical_plan::{PhysicalExpr, SQLMetric};
+use crate::physical_plan::PhysicalExpr;
 use log::debug;
 use std::fmt;
 
@@ -111,33 +113,45 @@ pub struct HashJoinExec {
     random_state: RandomState,
     /// Partitioning mode to use
     mode: PartitionMode,
-    /// Metrics
-    metrics: Arc<HashJoinMetrics>,
+    /// Execution metrics
+    metrics: SharedMetricsSet,
 }
 
 /// Metrics for HashJoinExec
 #[derive(Debug)]
 struct HashJoinMetrics {
     /// Total time for joining probe-side batches to the build-side batches
-    join_time: Arc<SQLMetric>,
+    join_time: metrics::wrappers::Time,
     /// Number of batches consumed by this operator
-    input_batches: Arc<SQLMetric>,
+    input_batches: metrics::wrappers::Count,
     /// Number of rows consumed by this operator
-    input_rows: Arc<SQLMetric>,
+    input_rows: metrics::wrappers::Count,
     /// Number of batches produced by this operator
-    output_batches: Arc<SQLMetric>,
+    output_batches: metrics::wrappers::Count,
     /// Number of rows produced by this operator
-    output_rows: Arc<SQLMetric>,
+    output_rows: metrics::wrappers::Count,
 }
 
 impl HashJoinMetrics {
-    fn new() -> Self {
+    pub fn new(partition: usize, metrics: &SharedMetricsSet) -> Self {
+        let join_time = MetricBuilder::new(metrics).subset_time("join_time", partition);
+
+        let input_batches =
+            MetricBuilder::new(metrics).counter("input_batches", partition);
+
+        let input_rows = MetricBuilder::new(metrics).counter("input_rows", partition);
+
+        let output_batches =
+            MetricBuilder::new(metrics).counter("output_rows", partition);
+
+        let output_rows = MetricBuilder::new(metrics).output_rows(partition);
+
         Self {
-            join_time: SQLMetric::time_nanos(),
-            input_batches: SQLMetric::counter(),
-            input_rows: SQLMetric::counter(),
-            output_batches: SQLMetric::counter(),
-            output_rows: SQLMetric::counter(),
+            join_time,
+            input_batches,
+            input_rows,
+            output_batches,
+            output_rows,
         }
     }
 }
@@ -187,7 +201,7 @@ impl HashJoinExec {
             build_side: Arc::new(Mutex::new(None)),
             random_state,
             mode: partition_mode,
-            metrics: Arc::new(HashJoinMetrics::new()),
+            metrics: SharedMetricsSet::new(),
         })
     }
 
@@ -425,7 +439,7 @@ impl ExecutionPlan for HashJoinExec {
             column_indices,
             self.random_state.clone(),
             visited_left_side,
-            self.metrics.clone(),
+            HashJoinMetrics::new(partition, &self.metrics),
         )))
     }
 
@@ -445,20 +459,8 @@ impl ExecutionPlan for HashJoinExec {
         }
     }
 
-    fn metrics(&self) -> HashMap<String, SQLMetric> {
-        let mut metrics = HashMap::new();
-        metrics.insert("joinTime".to_owned(), (*self.metrics.join_time).clone());
-        metrics.insert(
-            "inputBatches".to_owned(),
-            (*self.metrics.input_batches).clone(),
-        );
-        metrics.insert("inputRows".to_owned(), (*self.metrics.input_rows).clone());
-        metrics.insert(
-            "outputBatches".to_owned(),
-            (*self.metrics.output_batches).clone(),
-        );
-        metrics.insert("outputRows".to_owned(), (*self.metrics.output_rows).clone());
-        metrics
+    fn metrics(&self) -> Option<MetricsSet> {
+        Some(self.metrics.clone_inner())
     }
 }
 
@@ -522,7 +524,7 @@ struct HashJoinStream {
     /// There is nothing to process anymore and left side is processed in case of left join
     is_exhausted: bool,
     /// Metrics
-    metrics: Arc<HashJoinMetrics>,
+    join_metrics: HashJoinMetrics,
 }
 
 #[allow(clippy::too_many_arguments)]
@@ -537,7 +539,7 @@ impl HashJoinStream {
         column_indices: Vec<ColumnIndex>,
         random_state: RandomState,
         visited_left_side: Vec<bool>,
-        metrics: Arc<HashJoinMetrics>,
+        join_metrics: HashJoinMetrics,
     ) -> Self {
         HashJoinStream {
             schema,
@@ -550,7 +552,7 @@ impl HashJoinStream {
             random_state,
             visited_left_side,
             is_exhausted: false,
-            metrics,
+            join_metrics,
         }
     }
 }
@@ -876,7 +878,7 @@ impl Stream for HashJoinStream {
             .poll_next_unpin(cx)
             .map(|maybe_batch| match maybe_batch {
                 Some(Ok(batch)) => {
-                    let start = Instant::now();
+                    let timer = self.join_metrics.join_time.timer();
                     let result = build_batch(
                         &batch,
                         &self.left_data,
@@ -887,14 +889,12 @@ impl Stream for HashJoinStream {
                         &self.column_indices,
                         &self.random_state,
                     );
-                    self.metrics.input_batches.add(1);
-                    self.metrics.input_rows.add(batch.num_rows());
+                    self.join_metrics.input_batches.add(1);
+                    self.join_metrics.input_rows.add(batch.num_rows());
                     if let Ok((ref batch, ref left_side)) = result {
-                        self.metrics
-                            .join_time
-                            .add(start.elapsed().as_millis() as usize);
-                        self.metrics.output_batches.add(1);
-                        self.metrics.output_rows.add(batch.num_rows());
+                        timer.done();
+                        self.join_metrics.output_batches.add(1);
+                        self.join_metrics.output_rows.add(batch.num_rows());
 
                         match self.join_type {
                             JoinType::Left
@@ -911,7 +911,7 @@ impl Stream for HashJoinStream {
                     Some(result.map(|x| x.0))
                 }
                 other => {
-                    let start = Instant::now();
+                    let timer = self.join_metrics.join_time.timer();
                     // For the left join, produce rows for unmatched rows
                     match self.join_type {
                         JoinType::Left
@@ -928,16 +928,14 @@ impl Stream for HashJoinStream {
                                 self.join_type != JoinType::Semi,
                             );
                             if let Ok(ref batch) = result {
-                                self.metrics.input_batches.add(1);
-                                self.metrics.input_rows.add(batch.num_rows());
+                                self.join_metrics.input_batches.add(1);
+                                self.join_metrics.input_rows.add(batch.num_rows());
                                 if let Ok(ref batch) = result {
-                                    self.metrics
-                                        .join_time
-                                        .add(start.elapsed().as_millis() as usize);
-                                    self.metrics.output_batches.add(1);
-                                    self.metrics.output_rows.add(batch.num_rows());
+                                    self.join_metrics.output_batches.add(1);
+                                    self.join_metrics.output_rows.add(batch.num_rows());
                                 }
                             }
+                            timer.done();
                             self.is_exhausted = true;
                             return Some(result);
                         }
diff --git a/datafusion/src/physical_plan/metrics.rs b/datafusion/src/physical_plan/metrics.rs
new file mode 100644
index 0000000000000..8028210d01259
--- /dev/null
+++ b/datafusion/src/physical_plan/metrics.rs
@@ -0,0 +1,461 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Metrics for recording information about execution
+
+pub mod wrappers;
+
+use std::{
+    fmt::{Debug, Display},
+    sync::{
+        atomic::{AtomicUsize, Ordering},
+        Arc, Mutex,
+    },
+};
+
+use hashbrown::HashMap;
+
+use self::wrappers::{Count, Time};
+
+/// Structure for constructing metrics, counters, timers, etc
+pub struct MetricBuilder<'a> {
+    /// Location that the metric created by this builder will be added do
+    metrics: &'a SharedMetricsSet,
+
+    /// optional partition number
+    partition: Option<usize>,
+
+    /// arbitrary name=value pairs identifiying this metric
+    labels: Vec<Label>,
+}
+
+impl<'a> MetricBuilder<'a> {
+    /// Create a new `MetricBuilder` that will register the result of `build()` with the `metrics`
+    pub fn new(metrics: &'a SharedMetricsSet) -> Self {
+        Self {
+            metrics,
+            partition: None,
+            labels: vec![],
+        }
+    }
+
+    /// Add a label to the metric being constructed
+    pub fn with_label(mut self, label: Label) -> Self {
+        self.labels.push(label);
+        self
+    }
+
+    /// Add a label to the metric being constructed
+    pub fn with_new_label(
+        self,
+        name: impl Into<Arc<str>>,
+        value: impl Into<Arc<str>>,
+    ) -> Self {
+        self.with_label(Label::new(name.into(), value.into()))
+    }
+
+    /// Set the partition of the metric being constructed
+    pub fn with_partition(mut self, partition: usize) -> Self {
+        self.partition = Some(partition);
+        self
+    }
+
+    /// Consume self and create a metric of the specified kind
+    /// registered with the MetricsSet
+    pub fn build(self, kind: MetricKind) -> Arc<SQLMetric> {
+        let Self {
+            labels,
+            partition,
+            metrics,
+        } = self;
+        let metric = Arc::new(SQLMetric::new_with_labels(kind, partition, labels));
+        metrics.register(metric.clone());
+        metric
+    }
+
+    /// Consume self and create a new counter for recording output rows
+    pub fn output_rows(self, partition: usize) -> Count {
+        let metric = self.with_partition(partition).build(MetricKind::OutputRows);
+        Count::new(metric)
+    }
+
+    /// Consumes self and creates a new Countr for recording
+    /// some metric of an operators
+    pub fn counter(self, counter_name: impl Into<Arc<str>>, partition: usize) -> Count {
+        let metric = self
+            .with_partition(partition)
+            .build(MetricKind::Custom(counter_name.into()));
+        Count::new(metric)
+    }
+
+    /// Consumes self and creates a new Counter for recording
+    /// some metric of an overall operator (not per partition
+    pub fn global_counter(self, counter_name: impl Into<Arc<str>>) -> Count {
+        let metric = self.build(MetricKind::Custom(counter_name.into()));
+        Count::new(metric)
+    }
+
+    /// Consume self and create a new Timer for recording the overall cpu time
+    /// spent by an operator
+    pub fn cpu_time(self, partition: usize) -> Time {
+        let metric = self.with_partition(partition).build(MetricKind::CPUTime);
+        Time::new(metric)
+    }
+
+    /// Consumes self and creates a new Timer for recording some
+    /// subset of of an operators execution time
+    pub fn subset_time(self, subset_name: impl Into<Arc<str>>, partition: usize) -> Time {
+        let metric = self
+            .with_partition(partition)
+            .build(MetricKind::Custom(subset_name.into()));
+        Time::new(metric)
+    }
+}
+
+/// Something that tracks the metrics of an execution using an atomic
+/// usize
+#[derive(Debug)]
+pub struct SQLMetric {
+    /// value of the metric
+    value: AtomicUsize,
+
+    /// arbitrary name=value pairs identifiying this metric
+    labels: Vec<Label>,
+
+    /// To which partition of an operators output did this metric
+    /// apply? If None means all partitions.
+    partition: Option<usize>,
+
+    /// The kind of metric (how to logically interpret the value)
+    kind: MetricKind,
+}
+
+impl Display for SQLMetric {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "{}", self.kind)?;
+
+        let mut iter = self
+            .partition
+            .iter()
+            .map(|partition| {
+                Label::new(
+                    Arc::from("partition"),
+                    Arc::from(partition.to_string().as_str()),
+                )
+            })
+            .chain(self.labels().iter().cloned())
+            .peekable();
+
+        // print out the labels specially
+        if iter.peek().is_some() {
+            write!(f, "{{")?;
+
+            let mut is_first = true;
+            for i in iter {
+                if !is_first {
+                    write!(f, ", ")?;
+                } else {
+                    is_first = false;
+                }
+
+                write!(f, "{}", i)?;
+            }
+
+            write!(f, "}}")?;
+        }
+
+        // and now the value
+        let format_as_duration = match &self.kind {
+            MetricKind::OutputRows => false,
+            MetricKind::CPUTime => true,
+            MetricKind::Custom(name) => name.contains("Time") || name.contains("time"),
+        };
+
+        if format_as_duration {
+            let duration = std::time::Duration::from_nanos(self.value() as u64);
+            write!(f, "={:?}", duration)
+        } else {
+            write!(f, "={}", self.value())
+        }
+    }
+}
+
+impl SQLMetric {
+    /// Create a new SQLMetric
+    pub fn new(kind: MetricKind, partition: Option<usize>) -> Self {
+        Self {
+            value: 0.into(),
+            labels: vec![],
+            partition,
+            kind,
+        }
+    }
+
+    /// Add a new label to this metric
+    pub fn new_with_labels(
+        kind: MetricKind,
+        partition: Option<usize>,
+        labels: Vec<Label>,
+    ) -> Self {
+        Self {
+            value: 0.into(),
+            labels,
+            partition,
+            kind,
+        }
+    }
+
+    /// Add a new label to this metric
+    pub fn with(mut self, label: Label) -> Self {
+        self.labels.push(label);
+        self
+    }
+
+    /// Add the standard name for output rows
+
+    /// Get the current value
+    pub fn value(&self) -> usize {
+        self.value.load(Ordering::Relaxed)
+    }
+
+    /// get the kind of this metric
+    pub fn kind(&self) -> &MetricKind {
+        &self.kind
+    }
+
+    /// Add `n` to the metric's value
+    pub fn add(&self, n: usize) {
+        // relaxed ordering for operations on `value` poses no issues
+        // we're purely using atomic ops with no associated memory ops
+        self.value.fetch_add(n, Ordering::Relaxed);
+    }
+
+    /// Set the metric's value to `n`
+    pub fn set(&self, n: usize) {
+        self.value.store(n, Ordering::Relaxed);
+    }
+
+    /// What labels are present for this metric?
+    fn labels(&self) -> &[Label] {
+        &self.labels
+    }
+}
+
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+/// How should the value of the metric be interpreted?
+pub enum MetricKind {
+    /// Number of output rows produced
+    OutputRows,
+    /// CPU time
+    CPUTime,
+    // TODO timestamp, etc
+    // https://github.com/apache/arrow-datafusion/issues/866
+    /// Arbitarary user defined type
+    Custom(Arc<str>),
+}
+
+impl Display for MetricKind {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "{}", self.name())
+    }
+}
+
+impl MetricKind {
+    /// return a user displayable name of this kind of metric
+    pub fn name(&self) -> &str {
+        match self {
+            MetricKind::OutputRows => "outputRows",
+            MetricKind::Custom(name) => &name,
+            MetricKind::CPUTime => "cpuTime",
+        }
+    }
+}
+
+/// A set of SQLMetrics for a particular operator
+#[derive(Default, Debug, Clone)]
+pub struct MetricsSet {
+    metrics: Vec<Arc<SQLMetric>>,
+}
+
+impl MetricsSet {
+    /// Create a new container of metrics
+    pub fn new() -> Self {
+        Default::default()
+    }
+
+    /// Add the specified metric
+    pub fn push(&mut self, metric: Arc<SQLMetric>) {
+        self.metrics.push(metric)
+    }
+
+    /// Add all [`SQLMetric`]s in this set to the specified array.
+    fn extend_other(&mut self, metrics: &mut Vec<Arc<SQLMetric>>) {
+        metrics.extend(self.metrics.iter().cloned())
+    }
+
+    /// convenience: return the number of rows produced, aggregated
+    /// across partitions or None if no metric is present
+    pub fn output_rows(&self) -> Option<usize> {
+        self.sum(|metric| matches!(metric.kind(), MetricKind::OutputRows))
+    }
+
+    /// convenience: return the amount of CPU time spent, aggregated
+    /// across partitions or None if no metric is present
+    pub fn cpu_time(&self) -> Option<usize> {
+        self.sum(|metric| matches!(metric.kind(), MetricKind::CPUTime))
+    }
+
+    /// Sums the values for metrics for which `f(metric)` returns
+    /// true, and returns the value. Returns None if no metrics match
+    /// the predicate.
+    pub fn sum<F>(&self, mut f: F) -> Option<usize>
+    where
+        F: FnMut(&SQLMetric) -> bool,
+    {
+        let mut iter = self
+            .metrics
+            .iter()
+            .filter(|metric| f(metric.as_ref()))
+            .peekable();
+
+        if iter.peek().is_none() {
+            None
+        } else {
+            Some(iter.map(|metric| metric.value()).sum())
+        }
+    }
+
+    /// Returns returns a new derived `MetricsSet` where all metrics
+    /// that had partition=`Some(..)` have been aggregated
+    /// together. The resulting `MetricsSet` has all metrics with `Partition=None`
+    pub fn aggregate_by_partition(&self) -> Self {
+        let mut map = HashMap::new();
+
+        // There are all sorts of ways to make this more efficient
+        for metric in &self.metrics {
+            let key = (metric.kind.clone(), metric.labels.clone());
+            map.entry(key)
+                .and_modify(|accum: &mut SQLMetric| {
+                    accum.set(accum.value() + metric.value())
+                })
+                .or_insert_with(|| {
+                    // accumuate with no partition
+                    let partition = None;
+                    let accum = SQLMetric::new_with_labels(
+                        metric.kind().clone(),
+                        partition,
+                        metric.labels().to_vec(),
+                    );
+                    accum.set(metric.value());
+                    accum
+                });
+        }
+
+        let new_metrics = map
+            .into_iter()
+            .map(|(_k, v)| Arc::new(v))
+            .collect::<Vec<_>>();
+
+        Self {
+            metrics: new_metrics,
+        }
+    }
+}
+
+impl Display for MetricsSet {
+    /// format the MetricsSet as a single string
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        let mut is_first = true;
+        for i in self.metrics.iter() {
+            if !is_first {
+                write!(f, ", ")?;
+            } else {
+                is_first = false;
+            }
+
+            write!(f, "{}", i)?;
+        }
+        Ok(())
+    }
+}
+
+/// A set of SQLMetrics that can be added to as partitions
+/// execute. Designed to be a convenience for operator implementation
+#[derive(Default, Debug)]
+pub struct SharedMetricsSet {
+    inner: Mutex<MetricsSet>,
+}
+
+impl SharedMetricsSet {
+    /// Create a new empty shared metrics set
+    pub fn new() -> Self {
+        Self {
+            inner: Mutex::new(MetricsSet::new()),
+        }
+    }
+
+    /// Add the specified metric
+    pub fn register(&self, metric: Arc<SQLMetric>) {
+        self.inner.lock().expect("not poisoned").push(metric)
+    }
+
+    /// Add all [`SQLMetric`]s for this `ExecutionPlan` to the
+    /// specified array.
+    pub fn extend_other(&self, metrics: &mut Vec<Arc<SQLMetric>>) {
+        self.inner
+            .lock()
+            .expect("not poisoned")
+            .extend_other(metrics)
+    }
+
+    /// Return a clone of the inner MetricsSet
+    pub fn clone_inner(&self) -> MetricsSet {
+        let guard = self.inner.lock().expect("not poisoned");
+        (*guard).clone()
+    }
+}
+
+/// name=value pairs identifiying a metric. This concept is called various things
+/// in various different systems:
+///
+/// "labels" in
+/// [prometheus](https://prometheus.io/docs/concepts/data_model/) and
+/// "tags" in
+/// [InfluxDB](https://docs.influxdata.com/influxdb/v1.8/write_protocols/line_protocol_tutorial/)
+/// , "attributes" in [open
+/// telemetry](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/datamodel.md],
+/// etc.
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+pub struct Label {
+    name: Arc<str>,
+    value: Arc<str>,
+}
+
+impl Label {
+    /// Create a new Label
+    pub fn new(name: impl Into<Arc<str>>, value: impl Into<Arc<str>>) -> Self {
+        let name = name.into();
+        let value = value.into();
+        Self { name, value }
+    }
+}
+
+impl Display for Label {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "{}={}", self.name, self.value)
+    }
+}
diff --git a/datafusion/src/physical_plan/metrics/wrappers.rs b/datafusion/src/physical_plan/metrics/wrappers.rs
new file mode 100644
index 0000000000000..0a023713494f3
--- /dev/null
+++ b/datafusion/src/physical_plan/metrics/wrappers.rs
@@ -0,0 +1,105 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//!  wrappers for `SQLMetrics` for more conveniently recording execution metrics
+
+use std::{sync::Arc, time::Instant};
+
+use super::SQLMetric;
+
+// pub trait MetricSource {
+//     /// Return the underlying metric
+//     pub fn metric(&self) -> Arc<SQLMetric>;
+// }
+
+/// a SQLMetric wrapper for a counter (number of input or output rows)
+///
+/// Note `clone` counters update the same underlying metrics
+#[derive(Debug, Clone)]
+pub struct Count {
+    inner: Arc<SQLMetric>,
+}
+
+impl Count {
+    /// create a new counter wrapper around this metric
+    pub fn new(inner: Arc<SQLMetric>) -> Self {
+        Self { inner }
+    }
+
+    /// Add `n` to the counter's value
+    pub fn add(&self, n: usize) {
+        self.inner.add(n)
+    }
+}
+
+/// a SQLMetric wrapper for CPU timing information
+#[derive(Debug, Clone)]
+pub struct Time {
+    inner: Arc<SQLMetric>,
+}
+
+impl Time {
+    /// Create a new [`Time`] wrapper suitable for recording elapsed
+    /// times for operations.
+    pub fn new(inner: Arc<SQLMetric>) -> Self {
+        Self { inner }
+    }
+
+    /// Add elapsed nanoseconds since `start`to self
+    pub fn add_elapsed(&self, start: Instant) {
+        let nanos = start.elapsed().as_nanos() as usize;
+        self.inner.add(nanos)
+    }
+
+    /// return a scoped guard that adds the amount of time elapsed
+    /// between its creation and its drop or call to `stop` to the
+    /// underlying metric.
+    pub fn timer(&self) -> ScopedTimerGuard<'_> {
+        ScopedTimerGuard {
+            inner: self,
+            start: Some(Instant::now()),
+        }
+    }
+}
+
+/// RAAI structure that adds all time between its construction and
+/// destruction to the CPU time or the first call to `stop` whichever
+/// comes first
+pub struct ScopedTimerGuard<'a> {
+    inner: &'a Time,
+    start: Option<Instant>,
+}
+
+impl<'a> ScopedTimerGuard<'a> {
+    /// Stop the timer timing and record the time taken
+    pub fn stop(&mut self) {
+        if let Some(start) = self.start.take() {
+            self.inner.add_elapsed(start)
+        }
+    }
+
+    /// Stop the timer, record the time taken and consume self
+    pub fn done(mut self) {
+        self.stop()
+    }
+}
+
+impl<'a> Drop for ScopedTimerGuard<'a> {
+    fn drop(&mut self) {
+        self.stop()
+    }
+}
diff --git a/datafusion/src/physical_plan/mod.rs b/datafusion/src/physical_plan/mod.rs
index 8f7db72484c96..322cff9281406 100644
--- a/datafusion/src/physical_plan/mod.rs
+++ b/datafusion/src/physical_plan/mod.rs
@@ -17,14 +17,8 @@
 
 //! Traits for physical query plan, supporting parallel execution for partitioned relations.
 
-use std::fmt;
-use std::fmt::{Debug, Display};
-use std::ops::Range;
-use std::sync::atomic::{AtomicUsize, Ordering};
-use std::sync::Arc;
-use std::task::{Context, Poll};
-use std::{any::Any, pin::Pin};
-
+use self::metrics::MetricsSet;
+pub use self::metrics::SQLMetric;
 use self::{
     coalesce_partitions::CoalescePartitionsExec, display::DisplayableExecutionPlan,
 };
@@ -42,7 +36,12 @@ use arrow::{array::ArrayRef, datatypes::Field};
 use async_trait::async_trait;
 pub use display::DisplayFormatType;
 use futures::stream::Stream;
-use hashbrown::HashMap;
+use std::fmt;
+use std::fmt::{Debug, Display};
+use std::ops::Range;
+use std::sync::Arc;
+use std::task::{Context, Poll};
+use std::{any::Any, pin::Pin};
 
 /// Trait for types that stream [arrow::record_batch::RecordBatch]
 pub trait RecordBatchStream: Stream<Item = ArrowResult<RecordBatch>> {
@@ -87,72 +86,6 @@ impl Stream for EmptyRecordBatchStream {
     }
 }
 
-/// SQL metric type
-#[derive(Debug, Clone)]
-pub enum MetricType {
-    /// Simple counter
-    Counter,
-    /// Wall clock time in nanoseconds
-    TimeNanos,
-}
-
-/// SQL metric such as counter (number of input or output rows) or timing information about
-/// a physical operator.
-#[derive(Debug)]
-pub struct SQLMetric {
-    /// Metric value
-    value: AtomicUsize,
-    /// Metric type
-    metric_type: MetricType,
-}
-
-impl Clone for SQLMetric {
-    fn clone(&self) -> Self {
-        Self {
-            value: AtomicUsize::new(self.value.load(Ordering::Relaxed)),
-            metric_type: self.metric_type.clone(),
-        }
-    }
-}
-
-impl SQLMetric {
-    // relaxed ordering for operations on `value` poses no issues
-    // we're purely using atomic ops with no associated memory ops
-
-    /// Create a new metric for tracking a counter
-    pub fn counter() -> Arc<SQLMetric> {
-        Arc::new(SQLMetric::new(MetricType::Counter))
-    }
-
-    /// Create a new metric for tracking time in nanoseconds
-    pub fn time_nanos() -> Arc<SQLMetric> {
-        Arc::new(SQLMetric::new(MetricType::TimeNanos))
-    }
-
-    /// Create a new SQLMetric
-    pub fn new(metric_type: MetricType) -> Self {
-        Self {
-            value: AtomicUsize::new(0),
-            metric_type,
-        }
-    }
-
-    /// Add to the value
-    pub fn add(&self, n: usize) {
-        self.value.fetch_add(n, Ordering::Relaxed);
-    }
-
-    /// Add elapsed nanoseconds since `start`to self
-    pub fn add_elapsed(&self, start: std::time::Instant) {
-        self.add(start.elapsed().as_nanos() as usize)
-    }
-
-    /// Get the current value
-    pub fn value(&self) -> usize {
-        self.value.load(Ordering::Relaxed)
-    }
-}
-
 /// Physical planner interface
 pub use self::planner::PhysicalPlanner;
 
@@ -193,9 +126,16 @@ pub trait ExecutionPlan: Debug + Send + Sync {
     /// creates an iterator
     async fn execute(&self, partition: usize) -> Result<SendableRecordBatchStream>;
 
-    /// Return a snapshot of the metrics collected during execution
-    fn metrics(&self) -> HashMap<String, SQLMetric> {
-        HashMap::new()
+    /// Return a copy of the set of [`SQLMetric`]s for this
+    /// `ExecutionPlan`.
+    ///
+    /// The [`MetricsSet`]s returned may not be the complete list of
+    /// metrics. Once `self.execute()` has returned (aka the future is
+    /// resolved) for all available partitions, the set of metrics
+    /// should be complete. If this function is called prior to
+    /// `execute()` new metrics may appear in subsequent calls.
+    fn metrics(&self) -> Option<MetricsSet> {
+        return None;
     }
 
     /// Format this `ExecutionPlan` to `f` in the specified type.
@@ -330,20 +270,6 @@ pub fn visit_execution_plan<V: ExecutionPlanVisitor>(
     Ok(())
 }
 
-/// Recursively gateher all execution metrics from this plan and all of its input plans
-pub fn plan_metrics(plan: Arc<dyn ExecutionPlan>) -> HashMap<String, SQLMetric> {
-    fn get_metrics_inner(
-        plan: &dyn ExecutionPlan,
-        mut metrics: HashMap<String, SQLMetric>,
-    ) -> HashMap<String, SQLMetric> {
-        metrics.extend(plan.metrics().into_iter());
-        plan.children().into_iter().fold(metrics, |metrics, child| {
-            get_metrics_inner(child.as_ref(), metrics)
-        })
-    }
-    get_metrics_inner(plan.as_ref(), HashMap::new())
-}
-
 /// Execute the [ExecutionPlan] and collect the results in memory
 pub async fn collect(plan: Arc<dyn ExecutionPlan>) -> Result<Vec<RecordBatch>> {
     let stream = execute_stream(plan).await?;
@@ -663,6 +589,7 @@ pub mod json;
 pub mod limit;
 pub mod math_expressions;
 pub mod memory;
+pub mod metrics;
 pub mod parquet;
 pub mod planner;
 pub mod projection;
diff --git a/datafusion/src/physical_plan/parquet.rs b/datafusion/src/physical_plan/parquet.rs
index ff8bb5b32678d..50267706524a4 100644
--- a/datafusion/src/physical_plan/parquet.rs
+++ b/datafusion/src/physical_plan/parquet.rs
@@ -38,7 +38,6 @@ use arrow::{
     error::{ArrowError, Result as ArrowResult},
     record_batch::RecordBatch,
 };
-use hashbrown::HashMap;
 use log::debug;
 use parquet::file::{
     metadata::RowGroupMetaData,
@@ -57,8 +56,8 @@ use tokio::{
 use crate::datasource::datasource::{ColumnStatistics, Statistics};
 use async_trait::async_trait;
 
+use super::metrics::{self, MetricBuilder, MetricsSet, SharedMetricsSet};
 use super::stream::RecordBatchReceiverStream;
-use super::SQLMetric;
 use crate::physical_plan::expressions::{MaxAccumulator, MinAccumulator};
 use crate::physical_plan::Accumulator;
 
@@ -75,8 +74,8 @@ pub struct ParquetExec {
     batch_size: usize,
     /// Statistics for the data set (sum of statistics for all partitions)
     statistics: Statistics,
-    /// metrics for the overall execution
-    metrics: ParquetExecMetrics,
+    /// Execution metrics
+    metrics: Arc<SharedMetricsSet>,
     /// Optional predicate builder
     predicate_builder: Option<PruningPredicate>,
     /// Optional limit of the number of rows
@@ -99,23 +98,16 @@ pub struct ParquetPartition {
     /// Statistics for this partition
     pub statistics: Statistics,
     /// Execution metrics
-    metrics: ParquetPartitionMetrics,
-}
-
-/// Stores metrics about the overall parquet execution
-#[derive(Debug, Clone)]
-pub struct ParquetExecMetrics {
-    /// Numer of times the pruning predicate could not be created
-    pub predicate_creation_errors: Arc<SQLMetric>,
+    metrics: Arc<SharedMetricsSet>,
 }
 
-/// Stores metrics about the parquet execution for a particular ParquetPartition
+/// Stores metrics about the parquet execution for a particular parquet file
 #[derive(Debug, Clone)]
-struct ParquetPartitionMetrics {
+struct ParquetFileMetrics {
     /// Numer of times the predicate could not be evaluated
-    pub predicate_evaluation_errors: Arc<SQLMetric>,
+    pub predicate_evaluation_errors: metrics::wrappers::Count,
     /// Number of row groups pruned using
-    pub row_groups_pruned: Arc<SQLMetric>,
+    pub row_groups_pruned: metrics::wrappers::Count,
 }
 
 impl ParquetExec {
@@ -167,6 +159,7 @@ impl ParquetExec {
                filenames, projection, predicate, limit);
         // build a list of Parquet partitions with statistics and gather all unique schemas
         // used in this data set
+        let metrics = Arc::new(SharedMetricsSet::new());
         let mut schemas: Vec<Schema> = vec![];
         let mut partitions = Vec::with_capacity(max_concurrency);
         let filenames: Vec<String> = filenames.iter().map(|s| s.to_string()).collect();
@@ -393,7 +386,11 @@ impl ParquetExec {
             };
             // remove files that are not needed in case of limit
             filenames.truncate(total_files);
-            partitions.push(ParquetPartition::new(filenames, statistics));
+            partitions.push(ParquetPartition::new(
+                filenames,
+                statistics,
+                metrics.clone(),
+            ));
             if limit_exhausted {
                 break;
             }
@@ -410,7 +407,10 @@ impl ParquetExec {
             )));
         }
         let schema = Arc::new(schemas.pop().unwrap());
-        let metrics = ParquetExecMetrics::new();
+
+        let metrics = Arc::new(SharedMetricsSet::new());
+        let predicate_creation_errors =
+            MetricBuilder::new(&metrics).global_counter("numPredicateCreationErrors");
 
         let predicate_builder = predicate.and_then(|predicate_expr| {
             match PruningPredicate::try_new(&predicate_expr, schema.clone()) {
@@ -420,7 +420,7 @@ impl ParquetExec {
                         "Could not create pruning predicate for {:?}: {}",
                         predicate_expr, e
                     );
-                    metrics.predicate_creation_errors.add(1);
+                    predicate_creation_errors.add(1);
                     None
                 }
             }
@@ -442,7 +442,7 @@ impl ParquetExec {
         partitions: Vec<ParquetPartition>,
         schema: SchemaRef,
         projection: Option<Vec<usize>>,
-        metrics: ParquetExecMetrics,
+        metrics: Arc<SharedMetricsSet>,
         predicate_builder: Option<PruningPredicate>,
         batch_size: usize,
         limit: Option<usize>,
@@ -582,11 +582,15 @@ impl ParquetExec {
 
 impl ParquetPartition {
     /// Create a new parquet partition
-    pub fn new(filenames: Vec<String>, statistics: Statistics) -> Self {
+    pub fn new(
+        filenames: Vec<String>,
+        statistics: Statistics,
+        metrics: Arc<SharedMetricsSet>,
+    ) -> Self {
         Self {
             filenames,
             statistics,
-            metrics: ParquetPartitionMetrics::new(),
+            metrics,
         }
     }
 
@@ -601,21 +605,20 @@ impl ParquetPartition {
     }
 }
 
-impl ParquetExecMetrics {
+impl ParquetFileMetrics {
     /// Create new metrics
-    pub fn new() -> Self {
-        Self {
-            predicate_creation_errors: SQLMetric::counter(),
-        }
-    }
-}
+    pub fn new(partition: usize, filename: &str, metrics: &SharedMetricsSet) -> Self {
+        let predicate_evaluation_errors = MetricBuilder::new(metrics)
+            .with_new_label("filename", filename)
+            .counter("numPredicateEvaluationErrors", partition);
+
+        let row_groups_pruned = MetricBuilder::new(metrics)
+            .with_new_label("filename", filename)
+            .counter("numRowGroupsPruned", partition);
 
-impl ParquetPartitionMetrics {
-    /// Create new metrics
-    pub fn new() -> Self {
         Self {
-            predicate_evaluation_errors: SQLMetric::counter(),
-            row_groups_pruned: SQLMetric::counter(),
+            predicate_evaluation_errors,
+            row_groups_pruned,
         }
     }
 }
@@ -663,9 +666,9 @@ impl ExecutionPlan for ParquetExec {
             Receiver<ArrowResult<RecordBatch>>,
         ) = channel(2);
 
-        let partition = &self.partitions[partition];
-        let filenames = partition.filenames.clone();
-        let metrics = partition.metrics.clone();
+        let parquet_partition = &self.partitions[partition];
+        let filenames = parquet_partition.filenames.clone();
+        let metrics = self.metrics.clone();
         let projection = self.projection.clone();
         let predicate_builder = self.predicate_builder.clone();
         let batch_size = self.batch_size;
@@ -673,6 +676,7 @@ impl ExecutionPlan for ParquetExec {
 
         task::spawn_blocking(move || {
             if let Err(e) = read_files(
+                partition,
                 &filenames,
                 metrics,
                 &projection,
@@ -714,29 +718,8 @@ impl ExecutionPlan for ParquetExec {
         }
     }
 
-    fn metrics(&self) -> HashMap<String, SQLMetric> {
-        self.partitions
-            .iter()
-            .flat_map(|p| {
-                vec![
-                    (
-                        format!(
-                            "numPredicateEvaluationErrors for {}",
-                            p.filenames.join(",")
-                        ),
-                        p.metrics.predicate_evaluation_errors.as_ref().clone(),
-                    ),
-                    (
-                        format!("numRowGroupsPruned for {}", p.filenames.join(",")),
-                        p.metrics.row_groups_pruned.as_ref().clone(),
-                    ),
-                ]
-            })
-            .chain(std::iter::once((
-                "numPredicateCreationErrors".to_string(),
-                self.metrics.predicate_creation_errors.as_ref().clone(),
-            )))
-            .collect()
+    fn metrics(&self) -> Option<MetricsSet> {
+        Some(self.metrics.clone_inner())
     }
 }
 
@@ -837,7 +820,7 @@ impl<'a> PruningStatistics for RowGroupPruningStatistics<'a> {
 
 fn build_row_group_predicate(
     predicate_builder: &PruningPredicate,
-    metrics: ParquetPartitionMetrics,
+    metrics: ParquetFileMetrics,
     row_group_metadata: &[RowGroupMetaData],
 ) -> Box<dyn Fn(&RowGroupMetaData, usize) -> bool> {
     let parquet_schema = predicate_builder.schema().as_ref();
@@ -866,8 +849,9 @@ fn build_row_group_predicate(
 }
 
 fn read_files(
+    partition: usize,
     filenames: &[String],
-    metrics: ParquetPartitionMetrics,
+    metrics: Arc<SharedMetricsSet>,
     projection: &[usize],
     predicate_builder: &Option<PruningPredicate>,
     batch_size: usize,
@@ -876,12 +860,13 @@ fn read_files(
 ) -> Result<()> {
     let mut total_rows = 0;
     'outer: for filename in filenames {
+        let file_metrics = ParquetFileMetrics::new(partition, filename, &metrics);
         let file = File::open(&filename)?;
         let mut file_reader = SerializedFileReader::new(file)?;
         if let Some(predicate_builder) = predicate_builder {
             let row_group_predicate = build_row_group_predicate(
                 predicate_builder,
-                metrics.clone(),
+                file_metrics,
                 file_reader.metadata().row_groups(),
             );
             file_reader.filter_row_groups(&row_group_predicate);
@@ -1016,6 +1001,11 @@ mod tests {
         Ok(())
     }
 
+    fn parquet_file_metrics() -> ParquetFileMetrics {
+        let metrics = Arc::new(SharedMetricsSet::new());
+        ParquetFileMetrics::new(0, "file.parquet", &metrics)
+    }
+
     #[test]
     fn row_group_predicate_builder_simple_expr() -> Result<()> {
         use crate::logical_plan::{col, lit};
@@ -1036,7 +1026,7 @@ mod tests {
         let row_group_metadata = vec![rgm1, rgm2];
         let row_group_predicate = build_row_group_predicate(
             &predicate_builder,
-            ParquetPartitionMetrics::new(),
+            parquet_file_metrics(),
             &row_group_metadata,
         );
         let row_group_filter = row_group_metadata
@@ -1069,7 +1059,7 @@ mod tests {
         let row_group_metadata = vec![rgm1, rgm2];
         let row_group_predicate = build_row_group_predicate(
             &predicate_builder,
-            ParquetPartitionMetrics::new(),
+            parquet_file_metrics(),
             &row_group_metadata,
         );
         let row_group_filter = row_group_metadata
@@ -1117,7 +1107,7 @@ mod tests {
         let row_group_metadata = vec![rgm1, rgm2];
         let row_group_predicate = build_row_group_predicate(
             &predicate_builder,
-            ParquetPartitionMetrics::new(),
+            parquet_file_metrics(),
             &row_group_metadata,
         );
         let row_group_filter = row_group_metadata
@@ -1135,7 +1125,7 @@ mod tests {
         let predicate_builder = PruningPredicate::try_new(&expr, schema)?;
         let row_group_predicate = build_row_group_predicate(
             &predicate_builder,
-            ParquetPartitionMetrics::new(),
+            parquet_file_metrics(),
             &row_group_metadata,
         );
         let row_group_filter = row_group_metadata
@@ -1182,7 +1172,7 @@ mod tests {
         let row_group_metadata = vec![rgm1, rgm2];
         let row_group_predicate = build_row_group_predicate(
             &predicate_builder,
-            ParquetPartitionMetrics::new(),
+            parquet_file_metrics(),
             &row_group_metadata,
         );
         let row_group_filter = row_group_metadata
diff --git a/datafusion/src/physical_plan/repartition.rs b/datafusion/src/physical_plan/repartition.rs
index eb3fe5560fd6d..e197dfdbcfe06 100644
--- a/datafusion/src/physical_plan/repartition.rs
+++ b/datafusion/src/physical_plan/repartition.rs
@@ -21,17 +21,17 @@
 use std::pin::Pin;
 use std::sync::Arc;
 use std::task::{Context, Poll};
-use std::time::Instant;
 use std::{any::Any, vec};
 
 use crate::error::{DataFusionError, Result};
 use crate::physical_plan::hash_utils::create_hashes;
-use crate::physical_plan::{DisplayFormatType, ExecutionPlan, Partitioning, SQLMetric};
+use crate::physical_plan::{DisplayFormatType, ExecutionPlan, Partitioning};
 use arrow::record_batch::RecordBatch;
 use arrow::{array::Array, error::Result as ArrowResult};
 use arrow::{compute::take, datatypes::SchemaRef};
 use tokio_stream::wrappers::UnboundedReceiverStream;
 
+use super::metrics::{self, MetricBuilder, MetricsSet, SharedMetricsSet};
 use super::{RecordBatchStream, SendableRecordBatchStream};
 use async_trait::async_trait;
 
@@ -63,38 +63,48 @@ pub struct RepartitionExec {
     >,
 
     /// Execution metrics
-    metrics: RepartitionMetrics,
+    metrics: SharedMetricsSet,
 }
 
 #[derive(Debug, Clone)]
 struct RepartitionMetrics {
     /// Time in nanos to execute child operator and fetch batches
-    fetch_nanos: Arc<SQLMetric>,
+    fetch_time: metrics::wrappers::Time,
     /// Time in nanos to perform repartitioning
-    repart_nanos: Arc<SQLMetric>,
+    repart_time: metrics::wrappers::Time,
     /// Time in nanos for sending resulting batches to channels
-    send_nanos: Arc<SQLMetric>,
+    send_time: metrics::wrappers::Time,
 }
 
 impl RepartitionMetrics {
-    fn new() -> Self {
+    pub fn new(
+        output_partition: usize,
+        input_partition: usize,
+        metrics: &SharedMetricsSet,
+    ) -> Self {
+        let label = metrics::Label::new("inputPartition", input_partition.to_string());
+
+        // Time in nanos to execute child operator and fetch batches
+        let fetch_time = MetricBuilder::new(metrics)
+            .with_label(label.clone())
+            .subset_time("fetch_time", output_partition);
+
+        // Time in nanos to perform repartitioning
+        let repart_time = MetricBuilder::new(metrics)
+            .with_label(label.clone())
+            .subset_time("repart_time", output_partition);
+
+        // Time in nanos for sending resulting batches to channels
+        let send_time = MetricBuilder::new(metrics)
+            .with_label(label.clone())
+            .subset_time("send_time", output_partition);
+
         Self {
-            fetch_nanos: SQLMetric::time_nanos(),
-            repart_nanos: SQLMetric::time_nanos(),
-            send_nanos: SQLMetric::time_nanos(),
+            fetch_time,
+            repart_time,
+            send_time,
         }
     }
-    /// Convert into the external metrics form
-    fn to_hashmap(&self) -> HashMap<String, SQLMetric> {
-        let mut metrics = HashMap::new();
-        metrics.insert("fetchTime".to_owned(), self.fetch_nanos.as_ref().clone());
-        metrics.insert(
-            "repartitionTime".to_owned(),
-            self.repart_nanos.as_ref().clone(),
-        );
-        metrics.insert("sendTime".to_owned(), self.send_nanos.as_ref().clone());
-        metrics
-    }
 }
 
 impl RepartitionExec {
@@ -175,6 +185,8 @@ impl ExecutionPlan for RepartitionExec {
                     .map(|(partition, (tx, _rx))| (*partition, tx.clone()))
                     .collect();
 
+                let r_metrics = RepartitionMetrics::new(i, partition, &self.metrics);
+
                 let input_task: JoinHandle<Result<()>> =
                     tokio::spawn(Self::pull_from_input(
                         random.clone(),
@@ -182,7 +194,7 @@ impl ExecutionPlan for RepartitionExec {
                         i,
                         txs.clone(),
                         self.partitioning.clone(),
-                        self.metrics.clone(),
+                        r_metrics,
                     ));
 
                 // In a separate task, wait for each input to be done
@@ -201,8 +213,8 @@ impl ExecutionPlan for RepartitionExec {
         }))
     }
 
-    fn metrics(&self) -> HashMap<String, SQLMetric> {
-        self.metrics.to_hashmap()
+    fn metrics(&self) -> Option<MetricsSet> {
+        Some(self.metrics.clone_inner())
     }
 
     fn fmt_as(
@@ -228,7 +240,7 @@ impl RepartitionExec {
             input,
             partitioning,
             channels: Arc::new(Mutex::new(HashMap::new())),
-            metrics: RepartitionMetrics::new(),
+            metrics: SharedMetricsSet::new(),
         })
     }
 
@@ -244,14 +256,14 @@ impl RepartitionExec {
         i: usize,
         mut txs: HashMap<usize, UnboundedSender<Option<ArrowResult<RecordBatch>>>>,
         partitioning: Partitioning,
-        metrics: RepartitionMetrics,
+        r_metrics: RepartitionMetrics,
     ) -> Result<()> {
         let num_output_partitions = txs.len();
 
         // execute the child operator
-        let now = Instant::now();
+        let timer = r_metrics.fetch_time.timer();
         let mut stream = input.execute(i).await?;
-        metrics.fetch_nanos.add_elapsed(now);
+        timer.done();
 
         let mut counter = 0;
         let hashes_buf = &mut vec![];
@@ -260,9 +272,9 @@ impl RepartitionExec {
         // pulling inputs
         while !txs.is_empty() {
             // fetch the next batch
-            let now = Instant::now();
+            let timer = r_metrics.fetch_time.timer();
             let result = stream.next().await;
-            metrics.fetch_nanos.add_elapsed(now);
+            timer.done();
 
             // Input is done
             if result.is_none() {
@@ -272,7 +284,7 @@ impl RepartitionExec {
 
             match &partitioning {
                 Partitioning::RoundRobinBatch(_) => {
-                    let now = Instant::now();
+                    let timer = r_metrics.send_time.timer();
                     let output_partition = counter % num_output_partitions;
                     // if there is still a receiver, send to it
                     if let Some(tx) = txs.get_mut(&output_partition) {
@@ -281,10 +293,10 @@ impl RepartitionExec {
                             txs.remove(&output_partition);
                         }
                     }
-                    metrics.send_nanos.add_elapsed(now);
+                    timer.done();
                 }
                 Partitioning::Hash(exprs, _) => {
-                    let now = Instant::now();
+                    let timer = r_metrics.repart_time.timer();
                     let input_batch = result?;
                     let arrays = exprs
                         .iter()
@@ -303,11 +315,12 @@ impl RepartitionExec {
                         indices[(*hash % num_output_partitions as u64) as usize]
                             .push(index as u64)
                     }
-                    metrics.repart_nanos.add_elapsed(now);
+                    timer.done();
+
                     for (num_output_partition, partition_indices) in
                         indices.into_iter().enumerate()
                     {
-                        let now = Instant::now();
+                        let timer = r_metrics.repart_time.timer();
                         let indices = partition_indices.into();
                         // Produce batches based on indices
                         let columns = input_batch
@@ -321,8 +334,9 @@ impl RepartitionExec {
                             .collect::<Result<Vec<Arc<dyn Array>>>>()?;
                         let output_batch =
                             RecordBatch::try_new(input_batch.schema(), columns);
-                        metrics.repart_nanos.add_elapsed(now);
-                        let now = Instant::now();
+                        timer.done();
+
+                        let timer = r_metrics.repart_time.timer();
                         // if there is still a receiver, send to it
                         if let Some(tx) = txs.get_mut(&num_output_partition) {
                             if tx.send(Some(output_batch)).is_err() {
@@ -330,7 +344,7 @@ impl RepartitionExec {
                                 txs.remove(&num_output_partition);
                             }
                         }
-                        metrics.send_nanos.add_elapsed(now);
+                        timer.done();
                     }
                 }
                 other => {
diff --git a/datafusion/src/physical_plan/sort.rs b/datafusion/src/physical_plan/sort.rs
index faaa10d109361..0c6a3d83e5841 100644
--- a/datafusion/src/physical_plan/sort.rs
+++ b/datafusion/src/physical_plan/sort.rs
@@ -17,11 +17,13 @@
 
 //! Defines the SORT plan
 
+use super::metrics::wrappers::{Count, Time};
+use super::metrics::{MetricBuilder, MetricsSet, SharedMetricsSet};
 use super::{RecordBatchStream, SendableRecordBatchStream};
 use crate::error::{DataFusionError, Result};
 use crate::physical_plan::expressions::PhysicalSortExpr;
 use crate::physical_plan::{
-    common, DisplayFormatType, Distribution, ExecutionPlan, Partitioning, SQLMetric,
+    common, DisplayFormatType, Distribution, ExecutionPlan, Partitioning,
 };
 pub use arrow::compute::SortOptions;
 use arrow::compute::{lexsort_to_indices, take, SortColumn, TakeOptions};
@@ -32,13 +34,11 @@ use arrow::{array::ArrayRef, error::ArrowError};
 use async_trait::async_trait;
 use futures::stream::Stream;
 use futures::Future;
-use hashbrown::HashMap;
 use pin_project_lite::pin_project;
 use std::any::Any;
 use std::pin::Pin;
 use std::sync::Arc;
 use std::task::{Context, Poll};
-use std::time::Instant;
 
 /// Sort execution plan
 #[derive(Debug)]
@@ -47,10 +47,8 @@ pub struct SortExec {
     input: Arc<dyn ExecutionPlan>,
     /// Sort expressions
     expr: Vec<PhysicalSortExpr>,
-    /// Output rows
-    output_rows: Arc<SQLMetric>,
-    /// Time to sort batches
-    sort_time_nanos: Arc<SQLMetric>,
+    /// Execution metrics
+    metrics: SharedMetricsSet,
     /// Preserve partitions of input plan
     preserve_partitioning: bool,
 }
@@ -74,9 +72,8 @@ impl SortExec {
         Self {
             expr,
             input,
+            metrics: SharedMetricsSet::new(),
             preserve_partitioning,
-            output_rows: SQLMetric::counter(),
-            sort_time_nanos: SQLMetric::time_nanos(),
         }
     }
 
@@ -157,11 +154,15 @@ impl ExecutionPlan for SortExec {
 
         let input = self.input.execute(partition).await?;
 
+        let output_rows = MetricBuilder::new(&self.metrics).output_rows(partition);
+
+        let cpu_time = MetricBuilder::new(&self.metrics).cpu_time(partition);
+
         Ok(Box::pin(SortStream::new(
             input,
             self.expr.clone(),
-            self.output_rows.clone(),
-            self.sort_time_nanos.clone(),
+            output_rows,
+            cpu_time,
         )))
     }
 
@@ -178,11 +179,8 @@ impl ExecutionPlan for SortExec {
         }
     }
 
-    fn metrics(&self) -> HashMap<String, SQLMetric> {
-        let mut metrics = HashMap::new();
-        metrics.insert("outputRows".to_owned(), (*self.output_rows).clone());
-        metrics.insert("sortTime".to_owned(), (*self.sort_time_nanos).clone());
-        metrics
+    fn metrics(&self) -> Option<MetricsSet> {
+        Some(self.metrics.clone_inner())
     }
 }
 
@@ -229,7 +227,7 @@ pin_project! {
         output: futures::channel::oneshot::Receiver<ArrowResult<Option<RecordBatch>>>,
         finished: bool,
         schema: SchemaRef,
-        output_rows: Arc<SQLMetric>,
+        output_rows: Count
     }
 }
 
@@ -237,8 +235,8 @@ impl SortStream {
     fn new(
         input: SendableRecordBatchStream,
         expr: Vec<PhysicalSortExpr>,
-        output_rows: Arc<SQLMetric>,
-        sort_time: Arc<SQLMetric>,
+        output_rows: Count,
+        sort_time: Time,
     ) -> Self {
         let (tx, rx) = futures::channel::oneshot::channel();
         let schema = input.schema();
@@ -248,14 +246,14 @@ impl SortStream {
                 .await
                 .map_err(DataFusionError::into_arrow_external_error)
                 .and_then(move |batches| {
-                    let now = Instant::now();
+                    let timer = sort_time.timer();
                     // combine all record batches into one for each column
                     let combined = common::combine_batches(&batches, schema.clone())?;
                     // sort combined record batch
                     let result = combined
                         .map(|batch| sort_batch(batch, schema, &expr))
                         .transpose()?;
-                    sort_time.add(now.elapsed().as_nanos() as usize);
+                    timer.done();
                     Ok(result)
                 });
 
@@ -438,8 +436,9 @@ mod tests {
         assert_eq!(DataType::Float64, *sort_exec.schema().field(1).data_type());
 
         let result: Vec<RecordBatch> = collect(sort_exec.clone()).await?;
-        assert!(sort_exec.metrics().get("sortTime").unwrap().value() > 0);
-        assert_eq!(sort_exec.metrics().get("outputRows").unwrap().value(), 8);
+        let metrics = sort_exec.metrics().unwrap();
+        assert!(metrics.cpu_time().unwrap() > 0);
+        assert_eq!(metrics.output_rows().unwrap(), 8);
         assert_eq!(result.len(), 1);
 
         let columns = result[0].columns();
diff --git a/datafusion/tests/parquet_pruning.rs b/datafusion/tests/parquet_pruning.rs
index 789f0810c983a..edcaeeef68cfc 100644
--- a/datafusion/tests/parquet_pruning.rs
+++ b/datafusion/tests/parquet_pruning.rs
@@ -33,11 +33,13 @@ use chrono::{Datelike, Duration};
 use datafusion::{
     datasource::TableProvider,
     logical_plan::{col, lit, Expr, LogicalPlan, LogicalPlanBuilder},
-    physical_plan::{plan_metrics, SQLMetric},
+    physical_plan::{
+        accept, metrics::MetricsSet, parquet::ParquetExec, ExecutionPlan,
+        ExecutionPlanVisitor,
+    },
     prelude::{ExecutionConfig, ExecutionContext},
     scalar::ScalarValue,
 };
-use hashbrown::HashMap;
 use parquet::{arrow::ArrowWriter, file::properties::WriterProperties};
 use tempfile::NamedTempFile;
 
@@ -417,6 +419,9 @@ enum Scenario {
 /// table "t" registered, pointing at a parquet file made with
 /// `make_test_file`
 struct ContextWithParquet {
+    #[allow(dead_code)]
+    /// temp file parquet data is written to. The file is cleaned up
+    /// when dropped
     file: NamedTempFile,
     provider: Arc<dyn TableProvider>,
     ctx: ExecutionContext,
@@ -426,8 +431,8 @@ struct ContextWithParquet {
 struct TestOutput {
     /// The input string
     sql: String,
-    /// Normalized metrics (filename replaced by a constant)
-    metrics: HashMap<String, SQLMetric>,
+    /// Execution metrics for the Parquet Scan
+    parquet_metrics: MetricsSet,
     /// number of rows in results
     result_rows: usize,
     /// the contents of the input, as a string
@@ -439,32 +444,24 @@ struct TestOutput {
 impl TestOutput {
     /// retrieve the value of the named metric, if any
     fn metric_value(&self, metric_name: &str) -> Option<usize> {
-        self.metrics.get(metric_name).map(|m| m.value())
+        self.parquet_metrics
+            .sum(|metric| metric.kind().name() == metric_name)
     }
 
     /// The number of times the pruning predicate evaluation errors
     fn predicate_evaluation_errors(&self) -> Option<usize> {
-        self.metric_value("numPredicateEvaluationErrors for PARQUET_FILE")
+        self.metric_value("numPredicateEvaluationErrors")
     }
 
     /// The number of times the pruning predicate evaluation errors
     fn row_groups_pruned(&self) -> Option<usize> {
-        self.metric_value("numRowGroupsPruned for PARQUET_FILE")
+        self.metric_value("numRowGroupsPruned")
     }
 
     fn description(&self) -> String {
-        let metrics = self
-            .metrics
-            .iter()
-            .map(|(name, val)| format!("  {} = {:?}", name, val))
-            .collect::<Vec<_>>();
-
         format!(
             "Input:\n{}\nQuery:\n{}\nOutput:\n{}\nMetrics:\n{}",
-            self.pretty_input,
-            self.sql,
-            self.pretty_results,
-            metrics.join("\n")
+            self.pretty_input, self.sql, self.pretty_results, self.parquet_metrics,
         )
     }
 }
@@ -532,25 +529,35 @@ impl ContextWithParquet {
         let pretty_input = pretty_format_batches(&input).unwrap();
 
         let logical_plan = self.ctx.optimize(&logical_plan).expect("optimizing plan");
-        let execution_plan = self
+        let physical_plan = self
             .ctx
             .create_physical_plan(&logical_plan)
             .expect("creating physical plan");
 
-        let results = datafusion::physical_plan::collect(execution_plan.clone())
+        let results = datafusion::physical_plan::collect(physical_plan.clone())
             .await
             .expect("Running");
 
-        // replace the path name, which varies test to test,a with some
-        // constant for test comparisons
-        let path = self.file.path();
-        let path_name = path.to_string_lossy();
-        let metrics = plan_metrics(execution_plan)
-            .into_iter()
-            .map(|(name, metric)| {
-                (name.replace(path_name.as_ref(), "PARQUET_FILE"), metric)
-            })
-            .collect();
+        // find the parquet metrics
+        struct MetricsFinder {
+            metrics: Option<MetricsSet>,
+        }
+        impl ExecutionPlanVisitor for MetricsFinder {
+            type Error = std::convert::Infallible;
+            fn pre_visit(
+                &mut self,
+                plan: &dyn ExecutionPlan,
+            ) -> Result<bool, Self::Error> {
+                if plan.as_any().downcast_ref::<ParquetExec>().is_some() {
+                    self.metrics = plan.metrics();
+                }
+                // stop searching once we have found the metrics
+                Ok(self.metrics.is_none())
+            }
+        }
+        let mut finder = MetricsFinder { metrics: None };
+        accept(physical_plan.as_ref(), &mut finder).unwrap();
+        let parquet_metrics = finder.metrics.unwrap();
 
         let result_rows = results.iter().map(|b| b.num_rows()).sum();
 
@@ -559,7 +566,7 @@ impl ContextWithParquet {
         let sql = sql.into();
         TestOutput {
             sql,
-            metrics,
+            parquet_metrics,
             result_rows,
             pretty_input,
             pretty_results,
diff --git a/datafusion/tests/sql.rs b/datafusion/tests/sql.rs
index 0f385680deed4..57b0eac35e152 100644
--- a/datafusion/tests/sql.rs
+++ b/datafusion/tests/sql.rs
@@ -2148,6 +2148,8 @@ async fn csv_explain_analyze() {
     let sql = "EXPLAIN ANALYZE SELECT count(*), c1 FROM aggregate_test_100 group by c1";
     let actual = execute_to_batches(&mut ctx, sql).await;
     let formatted = arrow::util::pretty::pretty_format_batches(&actual).unwrap();
+    println!("Analyze output is {}", formatted);
+
     let formatted = normalize_for_explain(&formatted);
 
     // Only test basic plumbing and try to avoid having to change too

From 685a090e08312e57a55d75228946d37f0faea651 Mon Sep 17 00:00:00 2001
From: Andrew Lamb <andrew@nerdnetworks.org>
Date: Mon, 16 Aug 2021 14:25:32 -0400
Subject: [PATCH 02/17] Update ballista for new interface

---
 .../src/execution_plans/shuffle_reader.rs     | 20 ++++---
 .../src/execution_plans/shuffle_writer.rs     | 58 ++++++++++---------
 ballista/rust/core/src/utils.rs               | 12 ++--
 datafusion/src/physical_plan/repartition.rs   |  6 +-
 4 files changed, 52 insertions(+), 44 deletions(-)

diff --git a/ballista/rust/core/src/execution_plans/shuffle_reader.rs b/ballista/rust/core/src/execution_plans/shuffle_reader.rs
index 0447ca9953135..4929e5a3d9653 100644
--- a/ballista/rust/core/src/execution_plans/shuffle_reader.rs
+++ b/ballista/rust/core/src/execution_plans/shuffle_reader.rs
@@ -28,6 +28,7 @@ use async_trait::async_trait;
 use datafusion::arrow::datatypes::SchemaRef;
 use datafusion::arrow::error::Result as ArrowResult;
 use datafusion::arrow::record_batch::RecordBatch;
+use datafusion::physical_plan::metrics::{MetricBuilder, MetricsSet, SharedMetricsSet};
 use datafusion::physical_plan::{
     DisplayFormatType, ExecutionPlan, Partitioning, SQLMetric,
 };
@@ -47,8 +48,8 @@ pub struct ShuffleReaderExec {
     /// Each partition of a shuffle can read data from multiple locations
     pub(crate) partition: Vec<Vec<PartitionLocation>>,
     pub(crate) schema: SchemaRef,
-    /// Time to fetch data from executor
-    fetch_time: Arc<SQLMetric>,
+    /// Execution metrics
+    metrics: Arc<SharedMetricsSet>,
 }
 
 impl ShuffleReaderExec {
@@ -60,7 +61,7 @@ impl ShuffleReaderExec {
         Ok(Self {
             partition,
             schema,
-            fetch_time: SQLMetric::time_nanos(),
+            metrics: Arc::new(SharedMetricsSet::new()),
         })
     }
 }
@@ -100,13 +101,16 @@ impl ExecutionPlan for ShuffleReaderExec {
     ) -> Result<Pin<Box<dyn RecordBatchStream + Send + Sync>>> {
         info!("ShuffleReaderExec::execute({})", partition);
 
-        let start = Instant::now();
+        let fetch_time =
+            MetricBuilder::new(&self.metrics).subset_time("fetchTime", partition);
+        let timer = fetch_time.timer();
+
         let partition_locations = &self.partition[partition];
         let result = future::join_all(partition_locations.iter().map(fetch_partition))
             .await
             .into_iter()
             .collect::<Result<Vec<_>>>()?;
-        self.fetch_time.add_elapsed(start);
+        timer.done();
 
         let result = WrappedStream::new(
             Box::pin(futures::stream::iter(result).flatten()),
@@ -149,10 +153,8 @@ impl ExecutionPlan for ShuffleReaderExec {
         }
     }
 
-    fn metrics(&self) -> HashMap<String, SQLMetric> {
-        let mut metrics = HashMap::new();
-        metrics.insert("fetchTime".to_owned(), (*self.fetch_time).clone());
-        metrics
+    fn metrics(&self) -> Option<MetricsSet> {
+        Some(self.metrics.clone_inner())
     }
 }
 
diff --git a/ballista/rust/core/src/execution_plans/shuffle_writer.rs b/ballista/rust/core/src/execution_plans/shuffle_writer.rs
index b1db21fa90a1b..3d346be7db3fa 100644
--- a/ballista/rust/core/src/execution_plans/shuffle_writer.rs
+++ b/ballista/rust/core/src/execution_plans/shuffle_writer.rs
@@ -45,6 +45,9 @@ use datafusion::arrow::ipc::writer::FileWriter;
 use datafusion::arrow::record_batch::RecordBatch;
 use datafusion::error::{DataFusionError, Result};
 use datafusion::physical_plan::hash_utils::create_hashes;
+use datafusion::physical_plan::metrics::{
+    self, MetricBuilder, MetricsSet, SharedMetricsSet,
+};
 use datafusion::physical_plan::repartition::RepartitionExec;
 use datafusion::physical_plan::Partitioning::RoundRobinBatch;
 use datafusion::physical_plan::{
@@ -71,24 +74,30 @@ pub struct ShuffleWriterExec {
     work_dir: String,
     /// Optional shuffle output partitioning
     shuffle_output_partitioning: Option<Partitioning>,
-    /// Shuffle write metrics
-    metrics: ShuffleWriteMetrics,
+    /// Execution metrics
+    metrics: Arc<SharedMetricsSet>,
 }
 
 #[derive(Debug, Clone)]
 struct ShuffleWriteMetrics {
     /// Time spend writing batches to shuffle files
-    write_time: Arc<SQLMetric>,
-    input_rows: Arc<SQLMetric>,
-    output_rows: Arc<SQLMetric>,
+    write_time: metrics::wrappers::Time,
+    input_rows: metrics::wrappers::Count,
+    output_rows: metrics::wrappers::Count,
 }
 
 impl ShuffleWriteMetrics {
-    fn new() -> Self {
+    fn new(partition: usize, metrics: &SharedMetricsSet) -> Self {
+        let write_time = MetricBuilder::new(metrics).subset_time("writeTime", partition);
+
+        let input_rows = MetricBuilder::new(metrics).counter("inputRows", partition);
+
+        let output_rows = MetricBuilder::new(metrics).output_rows(partition);
+
         Self {
-            write_time: SQLMetric::time_nanos(),
-            input_rows: SQLMetric::counter(),
-            output_rows: SQLMetric::counter(),
+            write_time,
+            input_rows,
+            output_rows,
         }
     }
 }
@@ -108,7 +117,7 @@ impl ShuffleWriterExec {
             plan,
             work_dir,
             shuffle_output_partitioning,
-            metrics: ShuffleWriteMetrics::new(),
+            metrics: Arc::new(SharedMetricsSet::new()),
         })
     }
 
@@ -139,9 +148,11 @@ impl ShuffleWriterExec {
         path.push(&self.job_id);
         path.push(&format!("{}", self.stage_id));
 
+        let write_metrics = ShuffleWriteMetrics::new(input_partition, &self.metrics);
+
         match &self.shuffle_output_partitioning {
             None => {
-                let start = Instant::now();
+                let timer = write_metrics.write_time.timer();
                 path.push(&format!("{}", input_partition));
                 std::fs::create_dir_all(&path)?;
                 path.push("data.arrow");
@@ -152,18 +163,18 @@ impl ShuffleWriterExec {
                 let stats = utils::write_stream_to_disk(
                     &mut stream,
                     path,
-                    self.metrics.write_time.clone(),
+                    &write_metrics.write_time,
                 )
                 .await
                 .map_err(|e| DataFusionError::Execution(format!("{:?}", e)))?;
 
-                self.metrics
+                write_metrics
                     .input_rows
                     .add(stats.num_rows.unwrap_or(0) as usize);
-                self.metrics
+                write_metrics
                     .output_rows
                     .add(stats.num_rows.unwrap_or(0) as usize);
-                self.metrics.write_time.add_elapsed(start);
+                timer.done();
 
                 info!(
                     "Executed partition {} in {} seconds. Statistics: {}",
@@ -197,7 +208,7 @@ impl ShuffleWriterExec {
                 while let Some(result) = stream.next().await {
                     let input_batch = result?;
 
-                    self.metrics.input_rows.add(input_batch.num_rows());
+                    write_metrics.input_rows.add(input_batch.num_rows());
 
                     let arrays = exprs
                         .iter()
@@ -239,7 +250,7 @@ impl ShuffleWriterExec {
 
                         //TODO optimize so we don't write or fetch empty partitions
                         //if output_batch.num_rows() > 0 {
-                        let start = Instant::now();
+                        let timer = write_metrics.write_time.timer();
                         match &mut writers[output_partition] {
                             Some(w) => {
                                 w.write(&output_batch)?;
@@ -260,9 +271,8 @@ impl ShuffleWriterExec {
                                 writers[output_partition] = Some(writer);
                             }
                         }
-                        self.metrics.output_rows.add(output_batch.num_rows());
-                        self.metrics.write_time.add_elapsed(start);
-                        //}
+                        write_metrics.output_rows.add(output_batch.num_rows());
+                        timer.done();
                     }
                 }
 
@@ -388,12 +398,8 @@ impl ExecutionPlan for ShuffleWriterExec {
         Ok(Box::pin(MemoryStream::try_new(vec![batch], schema, None)?))
     }
 
-    fn metrics(&self) -> HashMap<String, SQLMetric> {
-        let mut metrics = HashMap::new();
-        metrics.insert("inputRows".to_owned(), (*self.metrics.input_rows).clone());
-        metrics.insert("outputRows".to_owned(), (*self.metrics.output_rows).clone());
-        metrics.insert("writeTime".to_owned(), (*self.metrics.write_time).clone());
-        metrics
+    fn metrics(&self) -> Option<MetricsSet> {
+        Some(self.metrics.clone_inner())
     }
 
     fn fmt_as(
diff --git a/ballista/rust/core/src/utils.rs b/ballista/rust/core/src/utils.rs
index d753f70fa22e7..92429e83d1fcc 100644
--- a/ballista/rust/core/src/utils.rs
+++ b/ballista/rust/core/src/utils.rs
@@ -61,7 +61,7 @@ use datafusion::physical_plan::parquet::ParquetExec;
 use datafusion::physical_plan::projection::ProjectionExec;
 use datafusion::physical_plan::sort::SortExec;
 use datafusion::physical_plan::{
-    AggregateExpr, ExecutionPlan, PhysicalExpr, RecordBatchStream, SQLMetric,
+    metrics, AggregateExpr, ExecutionPlan, PhysicalExpr, RecordBatchStream, SQLMetric,
 };
 use futures::{future, Stream, StreamExt};
 use std::time::Instant;
@@ -71,7 +71,7 @@ use std::time::Instant;
 pub async fn write_stream_to_disk(
     stream: &mut Pin<Box<dyn RecordBatchStream + Send + Sync>>,
     path: &str,
-    disk_write_metric: Arc<SQLMetric>,
+    disk_write_metric: &metrics::wrappers::Time,
 ) -> Result<PartitionStats> {
     let file = File::create(&path).map_err(|e| {
         BallistaError::General(format!(
@@ -97,13 +97,13 @@ pub async fn write_stream_to_disk(
         num_rows += batch.num_rows();
         num_bytes += batch_size_bytes;
 
-        let start = Instant::now();
+        let timer = disk_write_metric.timer();
         writer.write(&batch)?;
-        disk_write_metric.add_elapsed(start);
+        timer.done();
     }
-    let start = Instant::now();
+    let timer = disk_write_metric.timer();
     writer.finish()?;
-    disk_write_metric.add_elapsed(start);
+    timer.done();
     Ok(PartitionStats::new(
         Some(num_rows as u64),
         Some(num_batches),
diff --git a/datafusion/src/physical_plan/repartition.rs b/datafusion/src/physical_plan/repartition.rs
index e197dfdbcfe06..4760a22a45a48 100644
--- a/datafusion/src/physical_plan/repartition.rs
+++ b/datafusion/src/physical_plan/repartition.rs
@@ -87,17 +87,17 @@ impl RepartitionMetrics {
         // Time in nanos to execute child operator and fetch batches
         let fetch_time = MetricBuilder::new(metrics)
             .with_label(label.clone())
-            .subset_time("fetch_time", output_partition);
+            .subset_time("fetchTime", output_partition);
 
         // Time in nanos to perform repartitioning
         let repart_time = MetricBuilder::new(metrics)
             .with_label(label.clone())
-            .subset_time("repart_time", output_partition);
+            .subset_time("repartTime", output_partition);
 
         // Time in nanos for sending resulting batches to channels
         let send_time = MetricBuilder::new(metrics)
             .with_label(label.clone())
-            .subset_time("send_time", output_partition);
+            .subset_time("sendTime", output_partition);
 
         Self {
             fetch_time,

From bafa042b6e2c986cfbfa8c7e92b745f72bcf2b07 Mon Sep 17 00:00:00 2001
From: Andrew Lamb <andrew@nerdnetworks.org>
Date: Mon, 16 Aug 2021 15:06:03 -0400
Subject: [PATCH 03/17] clippy

---
 datafusion/src/physical_plan/metrics.rs     | 2 +-
 datafusion/src/physical_plan/mod.rs         | 2 +-
 datafusion/src/physical_plan/parquet.rs     | 1 +
 datafusion/src/physical_plan/repartition.rs | 2 +-
 datafusion/tests/sql.rs                     | 2 --
 5 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/datafusion/src/physical_plan/metrics.rs b/datafusion/src/physical_plan/metrics.rs
index 8028210d01259..8c354781468bf 100644
--- a/datafusion/src/physical_plan/metrics.rs
+++ b/datafusion/src/physical_plan/metrics.rs
@@ -279,7 +279,7 @@ impl MetricKind {
     pub fn name(&self) -> &str {
         match self {
             MetricKind::OutputRows => "outputRows",
-            MetricKind::Custom(name) => &name,
+            MetricKind::Custom(name) => name,
             MetricKind::CPUTime => "cpuTime",
         }
     }
diff --git a/datafusion/src/physical_plan/mod.rs b/datafusion/src/physical_plan/mod.rs
index 322cff9281406..9ebde71d9e596 100644
--- a/datafusion/src/physical_plan/mod.rs
+++ b/datafusion/src/physical_plan/mod.rs
@@ -135,7 +135,7 @@ pub trait ExecutionPlan: Debug + Send + Sync {
     /// should be complete. If this function is called prior to
     /// `execute()` new metrics may appear in subsequent calls.
     fn metrics(&self) -> Option<MetricsSet> {
-        return None;
+        None
     }
 
     /// Format this `ExecutionPlan` to `f` in the specified type.
diff --git a/datafusion/src/physical_plan/parquet.rs b/datafusion/src/physical_plan/parquet.rs
index 50267706524a4..a9ae92ff85fc4 100644
--- a/datafusion/src/physical_plan/parquet.rs
+++ b/datafusion/src/physical_plan/parquet.rs
@@ -848,6 +848,7 @@ fn build_row_group_predicate(
     }
 }
 
+#[allow(clippy::too_many_arguments)]
 fn read_files(
     partition: usize,
     filenames: &[String],
diff --git a/datafusion/src/physical_plan/repartition.rs b/datafusion/src/physical_plan/repartition.rs
index 4760a22a45a48..7181dce550750 100644
--- a/datafusion/src/physical_plan/repartition.rs
+++ b/datafusion/src/physical_plan/repartition.rs
@@ -96,7 +96,7 @@ impl RepartitionMetrics {
 
         // Time in nanos for sending resulting batches to channels
         let send_time = MetricBuilder::new(metrics)
-            .with_label(label.clone())
+            .with_label(label)
             .subset_time("sendTime", output_partition);
 
         Self {
diff --git a/datafusion/tests/sql.rs b/datafusion/tests/sql.rs
index 57b0eac35e152..0f385680deed4 100644
--- a/datafusion/tests/sql.rs
+++ b/datafusion/tests/sql.rs
@@ -2148,8 +2148,6 @@ async fn csv_explain_analyze() {
     let sql = "EXPLAIN ANALYZE SELECT count(*), c1 FROM aggregate_test_100 group by c1";
     let actual = execute_to_batches(&mut ctx, sql).await;
     let formatted = arrow::util::pretty::pretty_format_batches(&actual).unwrap();
-    println!("Analyze output is {}", formatted);
-
     let formatted = normalize_for_explain(&formatted);
 
     // Only test basic plumbing and try to avoid having to change too

From a49295a0f8ce2542dcd19244df74989e6ea44645 Mon Sep 17 00:00:00 2001
From: Andrew Lamb <andrew@nerdnetworks.org>
Date: Wed, 18 Aug 2021 12:53:42 -0400
Subject: [PATCH 04/17] Move Arc inside SharedMetricsSet

---
 .../core/src/execution_plans/shuffle_reader.rs     |  4 ++--
 .../core/src/execution_plans/shuffle_writer.rs     |  4 ++--
 datafusion/src/physical_optimizer/repartition.rs   |  4 ++--
 datafusion/src/physical_plan/metrics.rs            |  6 +++---
 datafusion/src/physical_plan/parquet.rs            | 14 +++++++-------
 5 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/ballista/rust/core/src/execution_plans/shuffle_reader.rs b/ballista/rust/core/src/execution_plans/shuffle_reader.rs
index 4929e5a3d9653..cd5e014e32eff 100644
--- a/ballista/rust/core/src/execution_plans/shuffle_reader.rs
+++ b/ballista/rust/core/src/execution_plans/shuffle_reader.rs
@@ -49,7 +49,7 @@ pub struct ShuffleReaderExec {
     pub(crate) partition: Vec<Vec<PartitionLocation>>,
     pub(crate) schema: SchemaRef,
     /// Execution metrics
-    metrics: Arc<SharedMetricsSet>,
+    metrics: SharedMetricsSet,
 }
 
 impl ShuffleReaderExec {
@@ -61,7 +61,7 @@ impl ShuffleReaderExec {
         Ok(Self {
             partition,
             schema,
-            metrics: Arc::new(SharedMetricsSet::new()),
+            metrics: SharedMetricsSet::new(),
         })
     }
 }
diff --git a/ballista/rust/core/src/execution_plans/shuffle_writer.rs b/ballista/rust/core/src/execution_plans/shuffle_writer.rs
index 3d346be7db3fa..ed5b35623bdde 100644
--- a/ballista/rust/core/src/execution_plans/shuffle_writer.rs
+++ b/ballista/rust/core/src/execution_plans/shuffle_writer.rs
@@ -75,7 +75,7 @@ pub struct ShuffleWriterExec {
     /// Optional shuffle output partitioning
     shuffle_output_partitioning: Option<Partitioning>,
     /// Execution metrics
-    metrics: Arc<SharedMetricsSet>,
+    metrics: SharedMetricsSet,
 }
 
 #[derive(Debug, Clone)]
@@ -117,7 +117,7 @@ impl ShuffleWriterExec {
             plan,
             work_dir,
             shuffle_output_partitioning,
-            metrics: Arc::new(SharedMetricsSet::new()),
+            metrics: SharedMetricsSet::new(),
         })
     }
 
diff --git a/datafusion/src/physical_optimizer/repartition.rs b/datafusion/src/physical_optimizer/repartition.rs
index 4b1a536b2f124..5c1315db73d70 100644
--- a/datafusion/src/physical_optimizer/repartition.rs
+++ b/datafusion/src/physical_optimizer/repartition.rs
@@ -117,7 +117,7 @@ mod tests {
     #[test]
     fn added_repartition_to_single_partition() -> Result<()> {
         let schema = Arc::new(Schema::empty());
-        let metrics = Arc::new(SharedMetricsSet::new());
+        let metrics = SharedMetricsSet::new();
         let parquet_project = ProjectionExec::try_new(
             vec![],
             Arc::new(ParquetExec::new(
@@ -155,7 +155,7 @@ mod tests {
     #[test]
     fn repartition_deepest_node() -> Result<()> {
         let schema = Arc::new(Schema::empty());
-        let metrics = Arc::new(SharedMetricsSet::new());
+        let metrics = SharedMetricsSet::new();
         let parquet_project = ProjectionExec::try_new(
             vec![],
             Arc::new(ProjectionExec::try_new(
diff --git a/datafusion/src/physical_plan/metrics.rs b/datafusion/src/physical_plan/metrics.rs
index 8c354781468bf..cdfee8c046f4e 100644
--- a/datafusion/src/physical_plan/metrics.rs
+++ b/datafusion/src/physical_plan/metrics.rs
@@ -395,16 +395,16 @@ impl Display for MetricsSet {
 
 /// A set of SQLMetrics that can be added to as partitions
 /// execute. Designed to be a convenience for operator implementation
-#[derive(Default, Debug)]
+#[derive(Default, Debug, Clone)]
 pub struct SharedMetricsSet {
-    inner: Mutex<MetricsSet>,
+    inner: Arc<Mutex<MetricsSet>>,
 }
 
 impl SharedMetricsSet {
     /// Create a new empty shared metrics set
     pub fn new() -> Self {
         Self {
-            inner: Mutex::new(MetricsSet::new()),
+            inner: Arc::new(Mutex::new(MetricsSet::new())),
         }
     }
 
diff --git a/datafusion/src/physical_plan/parquet.rs b/datafusion/src/physical_plan/parquet.rs
index a9ae92ff85fc4..a2e5c8e594188 100644
--- a/datafusion/src/physical_plan/parquet.rs
+++ b/datafusion/src/physical_plan/parquet.rs
@@ -75,7 +75,7 @@ pub struct ParquetExec {
     /// Statistics for the data set (sum of statistics for all partitions)
     statistics: Statistics,
     /// Execution metrics
-    metrics: Arc<SharedMetricsSet>,
+    metrics: SharedMetricsSet,
     /// Optional predicate builder
     predicate_builder: Option<PruningPredicate>,
     /// Optional limit of the number of rows
@@ -98,7 +98,7 @@ pub struct ParquetPartition {
     /// Statistics for this partition
     pub statistics: Statistics,
     /// Execution metrics
-    metrics: Arc<SharedMetricsSet>,
+    metrics: SharedMetricsSet,
 }
 
 /// Stores metrics about the parquet execution for a particular parquet file
@@ -159,7 +159,7 @@ impl ParquetExec {
                filenames, projection, predicate, limit);
         // build a list of Parquet partitions with statistics and gather all unique schemas
         // used in this data set
-        let metrics = Arc::new(SharedMetricsSet::new());
+        let metrics = SharedMetricsSet::new();
         let mut schemas: Vec<Schema> = vec![];
         let mut partitions = Vec::with_capacity(max_concurrency);
         let filenames: Vec<String> = filenames.iter().map(|s| s.to_string()).collect();
@@ -408,7 +408,7 @@ impl ParquetExec {
         }
         let schema = Arc::new(schemas.pop().unwrap());
 
-        let metrics = Arc::new(SharedMetricsSet::new());
+        let metrics = SharedMetricsSet::new();
         let predicate_creation_errors =
             MetricBuilder::new(&metrics).global_counter("numPredicateCreationErrors");
 
@@ -442,7 +442,7 @@ impl ParquetExec {
         partitions: Vec<ParquetPartition>,
         schema: SchemaRef,
         projection: Option<Vec<usize>>,
-        metrics: Arc<SharedMetricsSet>,
+        metrics: SharedMetricsSet,
         predicate_builder: Option<PruningPredicate>,
         batch_size: usize,
         limit: Option<usize>,
@@ -585,7 +585,7 @@ impl ParquetPartition {
     pub fn new(
         filenames: Vec<String>,
         statistics: Statistics,
-        metrics: Arc<SharedMetricsSet>,
+        metrics: SharedMetricsSet,
     ) -> Self {
         Self {
             filenames,
@@ -852,7 +852,7 @@ fn build_row_group_predicate(
 fn read_files(
     partition: usize,
     filenames: &[String],
-    metrics: Arc<SharedMetricsSet>,
+    metrics: SharedMetricsSet,
     projection: &[usize],
     predicate_builder: &Option<PruningPredicate>,
     batch_size: usize,

From abbd6cd1fae04480cb8ba30b4a054103422c343a Mon Sep 17 00:00:00 2001
From: Andrew Lamb <andrew@nerdnetworks.org>
Date: Wed, 18 Aug 2021 12:57:00 -0400
Subject: [PATCH 05/17] Change MetricKind::Custom to use static str

---
 datafusion/src/physical_plan/metrics.rs | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/datafusion/src/physical_plan/metrics.rs b/datafusion/src/physical_plan/metrics.rs
index cdfee8c046f4e..28a9083687dd9 100644
--- a/datafusion/src/physical_plan/metrics.rs
+++ b/datafusion/src/physical_plan/metrics.rs
@@ -95,17 +95,17 @@ impl<'a> MetricBuilder<'a> {
 
     /// Consumes self and creates a new Countr for recording
     /// some metric of an operators
-    pub fn counter(self, counter_name: impl Into<Arc<str>>, partition: usize) -> Count {
+    pub fn counter(self, counter_name: &'static str, partition: usize) -> Count {
         let metric = self
             .with_partition(partition)
-            .build(MetricKind::Custom(counter_name.into()));
+            .build(MetricKind::Custom(counter_name));
         Count::new(metric)
     }
 
     /// Consumes self and creates a new Counter for recording
     /// some metric of an overall operator (not per partition
-    pub fn global_counter(self, counter_name: impl Into<Arc<str>>) -> Count {
-        let metric = self.build(MetricKind::Custom(counter_name.into()));
+    pub fn global_counter(self, counter_name: &'static str) -> Count {
+        let metric = self.build(MetricKind::Custom(counter_name));
         Count::new(metric)
     }
 
@@ -118,10 +118,10 @@ impl<'a> MetricBuilder<'a> {
 
     /// Consumes self and creates a new Timer for recording some
     /// subset of of an operators execution time
-    pub fn subset_time(self, subset_name: impl Into<Arc<str>>, partition: usize) -> Time {
+    pub fn subset_time(self, subset_name: &'static str, partition: usize) -> Time {
         let metric = self
             .with_partition(partition)
-            .build(MetricKind::Custom(subset_name.into()));
+            .build(MetricKind::Custom(subset_name));
         Time::new(metric)
     }
 }
@@ -265,7 +265,7 @@ pub enum MetricKind {
     // TODO timestamp, etc
     // https://github.com/apache/arrow-datafusion/issues/866
     /// Arbitarary user defined type
-    Custom(Arc<str>),
+    Custom(&'static str),
 }
 
 impl Display for MetricKind {

From 098f07e1aadb484521c6b3259b8c9f5cf8767ec9 Mon Sep 17 00:00:00 2001
From: Andrew Lamb <andrew@nerdnetworks.org>
Date: Wed, 18 Aug 2021 12:58:11 -0400
Subject: [PATCH 06/17] MetricKind implements Copy

---
 datafusion/src/physical_plan/metrics.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/datafusion/src/physical_plan/metrics.rs b/datafusion/src/physical_plan/metrics.rs
index 28a9083687dd9..43b5cfeafdb90 100644
--- a/datafusion/src/physical_plan/metrics.rs
+++ b/datafusion/src/physical_plan/metrics.rs
@@ -255,7 +255,7 @@ impl SQLMetric {
     }
 }
 
-#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
 /// How should the value of the metric be interpreted?
 pub enum MetricKind {
     /// Number of output rows produced

From 0e1fce95134ffb86df40e8f99949ef93d127a0f5 Mon Sep 17 00:00:00 2001
From: Andrew Lamb <andrew@nerdnetworks.org>
Date: Wed, 18 Aug 2021 13:10:10 -0400
Subject: [PATCH 07/17] Apply suggestions from code review

Co-authored-by: Raphael Taylor-Davies <1781103+tustvold@users.noreply.github.com>
---
 datafusion/src/physical_plan/metrics.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/datafusion/src/physical_plan/metrics.rs b/datafusion/src/physical_plan/metrics.rs
index 43b5cfeafdb90..d5d84975633e5 100644
--- a/datafusion/src/physical_plan/metrics.rs
+++ b/datafusion/src/physical_plan/metrics.rs
@@ -353,7 +353,7 @@ impl MetricsSet {
                     accum.set(accum.value() + metric.value())
                 })
                 .or_insert_with(|| {
-                    // accumuate with no partition
+                    // accumulate with no partition
                     let partition = None;
                     let accum = SQLMetric::new_with_labels(
                         metric.kind().clone(),

From 4cff24443ef1429cce7edd31dadc547eafc0a7eb Mon Sep 17 00:00:00 2001
From: Andrew Lamb <andrew@nerdnetworks.org>
Date: Wed, 18 Aug 2021 14:56:14 -0400
Subject: [PATCH 08/17] Use Cow<str> rather than Arc<str>

---
 datafusion/src/physical_plan/metrics.rs | 24 +++++++++++++-----------
 datafusion/src/physical_plan/parquet.rs |  4 ++--
 2 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/datafusion/src/physical_plan/metrics.rs b/datafusion/src/physical_plan/metrics.rs
index d5d84975633e5..0504b28347f2f 100644
--- a/datafusion/src/physical_plan/metrics.rs
+++ b/datafusion/src/physical_plan/metrics.rs
@@ -20,6 +20,7 @@
 pub mod wrappers;
 
 use std::{
+    borrow::Cow,
     fmt::{Debug, Display},
     sync::{
         atomic::{AtomicUsize, Ordering},
@@ -62,8 +63,8 @@ impl<'a> MetricBuilder<'a> {
     /// Add a label to the metric being constructed
     pub fn with_new_label(
         self,
-        name: impl Into<Arc<str>>,
-        value: impl Into<Arc<str>>,
+        name: impl Into<Cow<'static, str>>,
+        value: impl Into<Cow<'static, str>>,
     ) -> Self {
         self.with_label(Label::new(name.into(), value.into()))
     }
@@ -151,12 +152,7 @@ impl Display for SQLMetric {
         let mut iter = self
             .partition
             .iter()
-            .map(|partition| {
-                Label::new(
-                    Arc::from("partition"),
-                    Arc::from(partition.to_string().as_str()),
-                )
-            })
+            .map(|partition| Label::new("partition", partition.to_string()))
             .chain(self.labels().iter().cloned())
             .peekable();
 
@@ -439,15 +435,21 @@ impl SharedMetricsSet {
 /// , "attributes" in [open
 /// telemetry](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/datamodel.md],
 /// etc.
+///
+/// As the name and value are expected to mostly be constant strings,
+/// use a `Cow` to avoid copying / allocations in this common case.
 #[derive(Debug, Clone, PartialEq, Eq, Hash)]
 pub struct Label {
-    name: Arc<str>,
-    value: Arc<str>,
+    name: Cow<'static, str>,
+    value: Cow<'static, str>,
 }
 
 impl Label {
     /// Create a new Label
-    pub fn new(name: impl Into<Arc<str>>, value: impl Into<Arc<str>>) -> Self {
+    pub fn new(
+        name: impl Into<Cow<'static, str>>,
+        value: impl Into<Cow<'static, str>>,
+    ) -> Self {
         let name = name.into();
         let value = value.into();
         Self { name, value }
diff --git a/datafusion/src/physical_plan/parquet.rs b/datafusion/src/physical_plan/parquet.rs
index a2e5c8e594188..6c1f250cae6d7 100644
--- a/datafusion/src/physical_plan/parquet.rs
+++ b/datafusion/src/physical_plan/parquet.rs
@@ -609,11 +609,11 @@ impl ParquetFileMetrics {
     /// Create new metrics
     pub fn new(partition: usize, filename: &str, metrics: &SharedMetricsSet) -> Self {
         let predicate_evaluation_errors = MetricBuilder::new(metrics)
-            .with_new_label("filename", filename)
+            .with_new_label("filename", filename.to_string())
             .counter("numPredicateEvaluationErrors", partition);
 
         let row_groups_pruned = MetricBuilder::new(metrics)
-            .with_new_label("filename", filename)
+            .with_new_label("filename", filename.to_string())
             .counter("numRowGroupsPruned", partition);
 
         Self {

From 337a6b0e992a12dc2505afcbab3b3fc25694d7ff Mon Sep 17 00:00:00 2001
From: Andrew Lamb <andrew@nerdnetworks.org>
Date: Wed, 18 Aug 2021 14:57:04 -0400
Subject: [PATCH 09/17] clippy

---
 datafusion/src/physical_plan/metrics.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/datafusion/src/physical_plan/metrics.rs b/datafusion/src/physical_plan/metrics.rs
index 0504b28347f2f..6b1a89fe72854 100644
--- a/datafusion/src/physical_plan/metrics.rs
+++ b/datafusion/src/physical_plan/metrics.rs
@@ -343,7 +343,7 @@ impl MetricsSet {
 
         // There are all sorts of ways to make this more efficient
         for metric in &self.metrics {
-            let key = (metric.kind.clone(), metric.labels.clone());
+            let key = (metric.kind, metric.labels.clone());
             map.entry(key)
                 .and_modify(|accum: &mut SQLMetric| {
                     accum.set(accum.value() + metric.value())
@@ -352,7 +352,7 @@ impl MetricsSet {
                     // accumulate with no partition
                     let partition = None;
                     let accum = SQLMetric::new_with_labels(
-                        metric.kind().clone(),
+                        metric.kind(),
                         partition,
                         metric.labels().to_vec(),
                     );

From 03ba0780e2fff048cd4883d8451c05e7a56e1905 Mon Sep 17 00:00:00 2001
From: Andrew Lamb <andrew@nerdnetworks.org>
Date: Wed, 18 Aug 2021 15:02:57 -0400
Subject: [PATCH 10/17] Rename SharedMetricsSet to ExecutionPlanMetricsSet

---
 .../src/execution_plans/shuffle_reader.rs     |  8 ++++---
 .../src/execution_plans/shuffle_writer.rs     |  8 +++----
 .../src/physical_optimizer/repartition.rs     |  6 ++---
 .../src/physical_plan/hash_aggregate.rs       |  6 ++---
 datafusion/src/physical_plan/hash_join.rs     |  8 +++----
 datafusion/src/physical_plan/metrics.rs       | 21 ++++++++++------
 datafusion/src/physical_plan/parquet.rs       | 24 +++++++++++--------
 datafusion/src/physical_plan/repartition.rs   |  8 +++----
 datafusion/src/physical_plan/sort.rs          |  6 ++---
 9 files changed, 54 insertions(+), 41 deletions(-)

diff --git a/ballista/rust/core/src/execution_plans/shuffle_reader.rs b/ballista/rust/core/src/execution_plans/shuffle_reader.rs
index cd5e014e32eff..15ccf11881f17 100644
--- a/ballista/rust/core/src/execution_plans/shuffle_reader.rs
+++ b/ballista/rust/core/src/execution_plans/shuffle_reader.rs
@@ -28,7 +28,9 @@ use async_trait::async_trait;
 use datafusion::arrow::datatypes::SchemaRef;
 use datafusion::arrow::error::Result as ArrowResult;
 use datafusion::arrow::record_batch::RecordBatch;
-use datafusion::physical_plan::metrics::{MetricBuilder, MetricsSet, SharedMetricsSet};
+use datafusion::physical_plan::metrics::{
+    ExecutionPlanMetricsSet, MetricBuilder, MetricsSet,
+};
 use datafusion::physical_plan::{
     DisplayFormatType, ExecutionPlan, Partitioning, SQLMetric,
 };
@@ -49,7 +51,7 @@ pub struct ShuffleReaderExec {
     pub(crate) partition: Vec<Vec<PartitionLocation>>,
     pub(crate) schema: SchemaRef,
     /// Execution metrics
-    metrics: SharedMetricsSet,
+    metrics: ExecutionPlanMetricsSet,
 }
 
 impl ShuffleReaderExec {
@@ -61,7 +63,7 @@ impl ShuffleReaderExec {
         Ok(Self {
             partition,
             schema,
-            metrics: SharedMetricsSet::new(),
+            metrics: ExecutionPlanMetricsSet::new(),
         })
     }
 }
diff --git a/ballista/rust/core/src/execution_plans/shuffle_writer.rs b/ballista/rust/core/src/execution_plans/shuffle_writer.rs
index ed5b35623bdde..9872cffeb96ee 100644
--- a/ballista/rust/core/src/execution_plans/shuffle_writer.rs
+++ b/ballista/rust/core/src/execution_plans/shuffle_writer.rs
@@ -46,7 +46,7 @@ use datafusion::arrow::record_batch::RecordBatch;
 use datafusion::error::{DataFusionError, Result};
 use datafusion::physical_plan::hash_utils::create_hashes;
 use datafusion::physical_plan::metrics::{
-    self, MetricBuilder, MetricsSet, SharedMetricsSet,
+    self, ExecutionPlanMetricsSet, MetricBuilder, MetricsSet,
 };
 use datafusion::physical_plan::repartition::RepartitionExec;
 use datafusion::physical_plan::Partitioning::RoundRobinBatch;
@@ -75,7 +75,7 @@ pub struct ShuffleWriterExec {
     /// Optional shuffle output partitioning
     shuffle_output_partitioning: Option<Partitioning>,
     /// Execution metrics
-    metrics: SharedMetricsSet,
+    metrics: ExecutionPlanMetricsSet,
 }
 
 #[derive(Debug, Clone)]
@@ -87,7 +87,7 @@ struct ShuffleWriteMetrics {
 }
 
 impl ShuffleWriteMetrics {
-    fn new(partition: usize, metrics: &SharedMetricsSet) -> Self {
+    fn new(partition: usize, metrics: &ExecutionPlanMetricsSet) -> Self {
         let write_time = MetricBuilder::new(metrics).subset_time("writeTime", partition);
 
         let input_rows = MetricBuilder::new(metrics).counter("inputRows", partition);
@@ -117,7 +117,7 @@ impl ShuffleWriterExec {
             plan,
             work_dir,
             shuffle_output_partitioning,
-            metrics: SharedMetricsSet::new(),
+            metrics: ExecutionPlanMetricsSet::new(),
         })
     }
 
diff --git a/datafusion/src/physical_optimizer/repartition.rs b/datafusion/src/physical_optimizer/repartition.rs
index 5c1315db73d70..984b234bfe4c4 100644
--- a/datafusion/src/physical_optimizer/repartition.rs
+++ b/datafusion/src/physical_optimizer/repartition.rs
@@ -110,14 +110,14 @@ mod tests {
 
     use super::*;
     use crate::datasource::datasource::Statistics;
-    use crate::physical_plan::metrics::SharedMetricsSet;
+    use crate::physical_plan::metrics::ExecutionPlanMetricsSet;
     use crate::physical_plan::parquet::{ParquetExec, ParquetPartition};
     use crate::physical_plan::projection::ProjectionExec;
 
     #[test]
     fn added_repartition_to_single_partition() -> Result<()> {
         let schema = Arc::new(Schema::empty());
-        let metrics = SharedMetricsSet::new();
+        let metrics = ExecutionPlanMetricsSet::new();
         let parquet_project = ProjectionExec::try_new(
             vec![],
             Arc::new(ParquetExec::new(
@@ -155,7 +155,7 @@ mod tests {
     #[test]
     fn repartition_deepest_node() -> Result<()> {
         let schema = Arc::new(Schema::empty());
-        let metrics = SharedMetricsSet::new();
+        let metrics = ExecutionPlanMetricsSet::new();
         let parquet_project = ProjectionExec::try_new(
             vec![],
             Arc::new(ProjectionExec::try_new(
diff --git a/datafusion/src/physical_plan/hash_aggregate.rs b/datafusion/src/physical_plan/hash_aggregate.rs
index 683edbb6ac0e2..009696004b848 100644
--- a/datafusion/src/physical_plan/hash_aggregate.rs
+++ b/datafusion/src/physical_plan/hash_aggregate.rs
@@ -51,7 +51,7 @@ use pin_project_lite::pin_project;
 use async_trait::async_trait;
 
 use super::metrics::wrappers::Count;
-use super::metrics::{MetricBuilder, MetricsSet, SharedMetricsSet};
+use super::metrics::{ExecutionPlanMetricsSet, MetricBuilder, MetricsSet};
 use super::{expressions::Column, RecordBatchStream, SendableRecordBatchStream};
 
 /// Hash aggregate modes
@@ -88,7 +88,7 @@ pub struct HashAggregateExec {
     /// to the partial aggregate
     input_schema: SchemaRef,
     /// Execution Metrics
-    metrics: SharedMetricsSet,
+    metrics: ExecutionPlanMetricsSet,
 }
 
 fn create_schema(
@@ -144,7 +144,7 @@ impl HashAggregateExec {
             input,
             schema,
             input_schema,
-            metrics: SharedMetricsSet::new(),
+            metrics: ExecutionPlanMetricsSet::new(),
         })
     }
 
diff --git a/datafusion/src/physical_plan/hash_join.rs b/datafusion/src/physical_plan/hash_join.rs
index eb607c17d05b2..5e7cc283965b3 100644
--- a/datafusion/src/physical_plan/hash_join.rs
+++ b/datafusion/src/physical_plan/hash_join.rs
@@ -57,7 +57,7 @@ use super::{
 };
 use super::{
     expressions::Column,
-    metrics::{self, MetricBuilder, MetricsSet, SharedMetricsSet},
+    metrics::{self, ExecutionPlanMetricsSet, MetricBuilder, MetricsSet},
 };
 use crate::error::{DataFusionError, Result};
 use crate::logical_plan::JoinType;
@@ -114,7 +114,7 @@ pub struct HashJoinExec {
     /// Partitioning mode to use
     mode: PartitionMode,
     /// Execution metrics
-    metrics: SharedMetricsSet,
+    metrics: ExecutionPlanMetricsSet,
 }
 
 /// Metrics for HashJoinExec
@@ -133,7 +133,7 @@ struct HashJoinMetrics {
 }
 
 impl HashJoinMetrics {
-    pub fn new(partition: usize, metrics: &SharedMetricsSet) -> Self {
+    pub fn new(partition: usize, metrics: &ExecutionPlanMetricsSet) -> Self {
         let join_time = MetricBuilder::new(metrics).subset_time("join_time", partition);
 
         let input_batches =
@@ -201,7 +201,7 @@ impl HashJoinExec {
             build_side: Arc::new(Mutex::new(None)),
             random_state,
             mode: partition_mode,
-            metrics: SharedMetricsSet::new(),
+            metrics: ExecutionPlanMetricsSet::new(),
         })
     }
 
diff --git a/datafusion/src/physical_plan/metrics.rs b/datafusion/src/physical_plan/metrics.rs
index 6b1a89fe72854..97edda4f5b43f 100644
--- a/datafusion/src/physical_plan/metrics.rs
+++ b/datafusion/src/physical_plan/metrics.rs
@@ -35,7 +35,7 @@ use self::wrappers::{Count, Time};
 /// Structure for constructing metrics, counters, timers, etc
 pub struct MetricBuilder<'a> {
     /// Location that the metric created by this builder will be added do
-    metrics: &'a SharedMetricsSet,
+    metrics: &'a ExecutionPlanMetricsSet,
 
     /// optional partition number
     partition: Option<usize>,
@@ -46,7 +46,7 @@ pub struct MetricBuilder<'a> {
 
 impl<'a> MetricBuilder<'a> {
     /// Create a new `MetricBuilder` that will register the result of `build()` with the `metrics`
-    pub fn new(metrics: &'a SharedMetricsSet) -> Self {
+    pub fn new(metrics: &'a ExecutionPlanMetricsSet) -> Self {
         Self {
             metrics,
             partition: None,
@@ -352,7 +352,7 @@ impl MetricsSet {
                     // accumulate with no partition
                     let partition = None;
                     let accum = SQLMetric::new_with_labels(
-                        metric.kind(),
+                        *metric.kind(),
                         partition,
                         metric.labels().to_vec(),
                     );
@@ -389,14 +389,21 @@ impl Display for MetricsSet {
     }
 }
 
-/// A set of SQLMetrics that can be added to as partitions
-/// execute. Designed to be a convenience for operator implementation
+/// A set of SQLMetrics for an individual "operator" (e.g. `&dyn
+/// ExecutionPlan`).
+///
+/// This structure is intended as a convenience for [`ExecutionPlan`]
+/// implementations so they can generate different streams for multiple
+/// partitions but easily report them together.
+///
+/// Each `clone()` of this structure will add metrics to the same
+/// underlying metrics set
 #[derive(Default, Debug, Clone)]
-pub struct SharedMetricsSet {
+pub struct ExecutionPlanMetricsSet {
     inner: Arc<Mutex<MetricsSet>>,
 }
 
-impl SharedMetricsSet {
+impl ExecutionPlanMetricsSet {
     /// Create a new empty shared metrics set
     pub fn new() -> Self {
         Self {
diff --git a/datafusion/src/physical_plan/parquet.rs b/datafusion/src/physical_plan/parquet.rs
index 6c1f250cae6d7..8416551e43cde 100644
--- a/datafusion/src/physical_plan/parquet.rs
+++ b/datafusion/src/physical_plan/parquet.rs
@@ -56,7 +56,7 @@ use tokio::{
 use crate::datasource::datasource::{ColumnStatistics, Statistics};
 use async_trait::async_trait;
 
-use super::metrics::{self, MetricBuilder, MetricsSet, SharedMetricsSet};
+use super::metrics::{self, ExecutionPlanMetricsSet, MetricBuilder, MetricsSet};
 use super::stream::RecordBatchReceiverStream;
 use crate::physical_plan::expressions::{MaxAccumulator, MinAccumulator};
 use crate::physical_plan::Accumulator;
@@ -75,7 +75,7 @@ pub struct ParquetExec {
     /// Statistics for the data set (sum of statistics for all partitions)
     statistics: Statistics,
     /// Execution metrics
-    metrics: SharedMetricsSet,
+    metrics: ExecutionPlanMetricsSet,
     /// Optional predicate builder
     predicate_builder: Option<PruningPredicate>,
     /// Optional limit of the number of rows
@@ -98,7 +98,7 @@ pub struct ParquetPartition {
     /// Statistics for this partition
     pub statistics: Statistics,
     /// Execution metrics
-    metrics: SharedMetricsSet,
+    metrics: ExecutionPlanMetricsSet,
 }
 
 /// Stores metrics about the parquet execution for a particular parquet file
@@ -159,7 +159,7 @@ impl ParquetExec {
                filenames, projection, predicate, limit);
         // build a list of Parquet partitions with statistics and gather all unique schemas
         // used in this data set
-        let metrics = SharedMetricsSet::new();
+        let metrics = ExecutionPlanMetricsSet::new();
         let mut schemas: Vec<Schema> = vec![];
         let mut partitions = Vec::with_capacity(max_concurrency);
         let filenames: Vec<String> = filenames.iter().map(|s| s.to_string()).collect();
@@ -408,7 +408,7 @@ impl ParquetExec {
         }
         let schema = Arc::new(schemas.pop().unwrap());
 
-        let metrics = SharedMetricsSet::new();
+        let metrics = ExecutionPlanMetricsSet::new();
         let predicate_creation_errors =
             MetricBuilder::new(&metrics).global_counter("numPredicateCreationErrors");
 
@@ -442,7 +442,7 @@ impl ParquetExec {
         partitions: Vec<ParquetPartition>,
         schema: SchemaRef,
         projection: Option<Vec<usize>>,
-        metrics: SharedMetricsSet,
+        metrics: ExecutionPlanMetricsSet,
         predicate_builder: Option<PruningPredicate>,
         batch_size: usize,
         limit: Option<usize>,
@@ -585,7 +585,7 @@ impl ParquetPartition {
     pub fn new(
         filenames: Vec<String>,
         statistics: Statistics,
-        metrics: SharedMetricsSet,
+        metrics: ExecutionPlanMetricsSet,
     ) -> Self {
         Self {
             filenames,
@@ -607,7 +607,11 @@ impl ParquetPartition {
 
 impl ParquetFileMetrics {
     /// Create new metrics
-    pub fn new(partition: usize, filename: &str, metrics: &SharedMetricsSet) -> Self {
+    pub fn new(
+        partition: usize,
+        filename: &str,
+        metrics: &ExecutionPlanMetricsSet,
+    ) -> Self {
         let predicate_evaluation_errors = MetricBuilder::new(metrics)
             .with_new_label("filename", filename.to_string())
             .counter("numPredicateEvaluationErrors", partition);
@@ -852,7 +856,7 @@ fn build_row_group_predicate(
 fn read_files(
     partition: usize,
     filenames: &[String],
-    metrics: SharedMetricsSet,
+    metrics: ExecutionPlanMetricsSet,
     projection: &[usize],
     predicate_builder: &Option<PruningPredicate>,
     batch_size: usize,
@@ -1003,7 +1007,7 @@ mod tests {
     }
 
     fn parquet_file_metrics() -> ParquetFileMetrics {
-        let metrics = Arc::new(SharedMetricsSet::new());
+        let metrics = Arc::new(ExecutionPlanMetricsSet::new());
         ParquetFileMetrics::new(0, "file.parquet", &metrics)
     }
 
diff --git a/datafusion/src/physical_plan/repartition.rs b/datafusion/src/physical_plan/repartition.rs
index 7181dce550750..75429efba0190 100644
--- a/datafusion/src/physical_plan/repartition.rs
+++ b/datafusion/src/physical_plan/repartition.rs
@@ -31,7 +31,7 @@ use arrow::{array::Array, error::Result as ArrowResult};
 use arrow::{compute::take, datatypes::SchemaRef};
 use tokio_stream::wrappers::UnboundedReceiverStream;
 
-use super::metrics::{self, MetricBuilder, MetricsSet, SharedMetricsSet};
+use super::metrics::{self, ExecutionPlanMetricsSet, MetricBuilder, MetricsSet};
 use super::{RecordBatchStream, SendableRecordBatchStream};
 use async_trait::async_trait;
 
@@ -63,7 +63,7 @@ pub struct RepartitionExec {
     >,
 
     /// Execution metrics
-    metrics: SharedMetricsSet,
+    metrics: ExecutionPlanMetricsSet,
 }
 
 #[derive(Debug, Clone)]
@@ -80,7 +80,7 @@ impl RepartitionMetrics {
     pub fn new(
         output_partition: usize,
         input_partition: usize,
-        metrics: &SharedMetricsSet,
+        metrics: &ExecutionPlanMetricsSet,
     ) -> Self {
         let label = metrics::Label::new("inputPartition", input_partition.to_string());
 
@@ -240,7 +240,7 @@ impl RepartitionExec {
             input,
             partitioning,
             channels: Arc::new(Mutex::new(HashMap::new())),
-            metrics: SharedMetricsSet::new(),
+            metrics: ExecutionPlanMetricsSet::new(),
         })
     }
 
diff --git a/datafusion/src/physical_plan/sort.rs b/datafusion/src/physical_plan/sort.rs
index 0c6a3d83e5841..959b71fa392dd 100644
--- a/datafusion/src/physical_plan/sort.rs
+++ b/datafusion/src/physical_plan/sort.rs
@@ -18,7 +18,7 @@
 //! Defines the SORT plan
 
 use super::metrics::wrappers::{Count, Time};
-use super::metrics::{MetricBuilder, MetricsSet, SharedMetricsSet};
+use super::metrics::{ExecutionPlanMetricsSet, MetricBuilder, MetricsSet};
 use super::{RecordBatchStream, SendableRecordBatchStream};
 use crate::error::{DataFusionError, Result};
 use crate::physical_plan::expressions::PhysicalSortExpr;
@@ -48,7 +48,7 @@ pub struct SortExec {
     /// Sort expressions
     expr: Vec<PhysicalSortExpr>,
     /// Execution metrics
-    metrics: SharedMetricsSet,
+    metrics: ExecutionPlanMetricsSet,
     /// Preserve partitions of input plan
     preserve_partitioning: bool,
 }
@@ -72,7 +72,7 @@ impl SortExec {
         Self {
             expr,
             input,
-            metrics: SharedMetricsSet::new(),
+            metrics: ExecutionPlanMetricsSet::new(),
             preserve_partitioning,
         }
     }

From 0d7bde5ad1b976569c4d5631c5cd150fb0632d3d Mon Sep 17 00:00:00 2001
From: Andrew Lamb <andrew@nerdnetworks.org>
Date: Wed, 18 Aug 2021 15:45:13 -0400
Subject: [PATCH 11/17] Idomatic Rust: Use typed values rather than MetricKind

---
 .../src/execution_plans/shuffle_writer.rs     |   6 +-
 .../src/physical_plan/hash_aggregate.rs       |   7 +-
 datafusion/src/physical_plan/hash_join.rs     |  10 +-
 datafusion/src/physical_plan/metrics.rs       | 321 ++++++++++++------
 .../src/physical_plan/metrics/wrappers.rs     | 105 ------
 datafusion/src/physical_plan/parquet.rs       |   4 +-
 datafusion/src/physical_plan/repartition.rs   |   6 +-
 datafusion/src/physical_plan/sort.rs          |   9 +-
 8 files changed, 229 insertions(+), 239 deletions(-)
 delete mode 100644 datafusion/src/physical_plan/metrics/wrappers.rs

diff --git a/ballista/rust/core/src/execution_plans/shuffle_writer.rs b/ballista/rust/core/src/execution_plans/shuffle_writer.rs
index 9872cffeb96ee..3b8160d9d692d 100644
--- a/ballista/rust/core/src/execution_plans/shuffle_writer.rs
+++ b/ballista/rust/core/src/execution_plans/shuffle_writer.rs
@@ -81,9 +81,9 @@ pub struct ShuffleWriterExec {
 #[derive(Debug, Clone)]
 struct ShuffleWriteMetrics {
     /// Time spend writing batches to shuffle files
-    write_time: metrics::wrappers::Time,
-    input_rows: metrics::wrappers::Count,
-    output_rows: metrics::wrappers::Count,
+    write_time: metrics::Time,
+    input_rows: metrics::Count,
+    output_rows: metrics::Count,
 }
 
 impl ShuffleWriteMetrics {
diff --git a/datafusion/src/physical_plan/hash_aggregate.rs b/datafusion/src/physical_plan/hash_aggregate.rs
index 009696004b848..5a2ec9ee2d88f 100644
--- a/datafusion/src/physical_plan/hash_aggregate.rs
+++ b/datafusion/src/physical_plan/hash_aggregate.rs
@@ -50,8 +50,7 @@ use pin_project_lite::pin_project;
 
 use async_trait::async_trait;
 
-use super::metrics::wrappers::Count;
-use super::metrics::{ExecutionPlanMetricsSet, MetricBuilder, MetricsSet};
+use super::metrics::{self, ExecutionPlanMetricsSet, MetricBuilder, MetricsSet};
 use super::{expressions::Column, RecordBatchStream, SendableRecordBatchStream};
 
 /// Hash aggregate modes
@@ -316,7 +315,7 @@ pin_project! {
         #[pin]
         output: futures::channel::oneshot::Receiver<ArrowResult<RecordBatch>>,
         finished: bool,
-        output_rows: Count,
+        output_rows: metrics::Count,
     }
 }
 
@@ -525,7 +524,7 @@ impl GroupedHashAggregateStream {
         group_expr: Vec<Arc<dyn PhysicalExpr>>,
         aggr_expr: Vec<Arc<dyn AggregateExpr>>,
         input: SendableRecordBatchStream,
-        output_rows: Count,
+        output_rows: metrics::Count,
     ) -> Self {
         let (tx, rx) = futures::channel::oneshot::channel();
 
diff --git a/datafusion/src/physical_plan/hash_join.rs b/datafusion/src/physical_plan/hash_join.rs
index 5e7cc283965b3..3a0a3f206b65f 100644
--- a/datafusion/src/physical_plan/hash_join.rs
+++ b/datafusion/src/physical_plan/hash_join.rs
@@ -121,15 +121,15 @@ pub struct HashJoinExec {
 #[derive(Debug)]
 struct HashJoinMetrics {
     /// Total time for joining probe-side batches to the build-side batches
-    join_time: metrics::wrappers::Time,
+    join_time: metrics::Time,
     /// Number of batches consumed by this operator
-    input_batches: metrics::wrappers::Count,
+    input_batches: metrics::Count,
     /// Number of rows consumed by this operator
-    input_rows: metrics::wrappers::Count,
+    input_rows: metrics::Count,
     /// Number of batches produced by this operator
-    output_batches: metrics::wrappers::Count,
+    output_batches: metrics::Count,
     /// Number of rows produced by this operator
-    output_rows: metrics::wrappers::Count,
+    output_rows: metrics::Count,
 }
 
 impl HashJoinMetrics {
diff --git a/datafusion/src/physical_plan/metrics.rs b/datafusion/src/physical_plan/metrics.rs
index 97edda4f5b43f..58831396721db 100644
--- a/datafusion/src/physical_plan/metrics.rs
+++ b/datafusion/src/physical_plan/metrics.rs
@@ -17,22 +17,21 @@
 
 //! Metrics for recording information about execution
 
-pub mod wrappers;
-
-use std::{
-    borrow::Cow,
-    fmt::{Debug, Display},
-    sync::{
+use std::{borrow::Cow, fmt::{Debug, Display}, sync::{
         atomic::{AtomicUsize, Ordering},
         Arc, Mutex,
-    },
-};
+    }, time::Instant};
 
 use hashbrown::HashMap;
 
-use self::wrappers::{Count, Time};
-
-/// Structure for constructing metrics, counters, timers, etc
+/// Structure for constructing metrics, counters, timers, etc.
+///
+/// Note the use of `Cow<..>` is to avoid allocations in the common
+/// case of constant strings
+///
+/// ```rust
+/// TODO doc example
+/// ```
 pub struct MetricBuilder<'a> {
     /// Location that the metric created by this builder will be added do
     metrics: &'a ExecutionPlanMetricsSet,
@@ -75,64 +74,225 @@ impl<'a> MetricBuilder<'a> {
         self
     }
 
-    /// Consume self and create a metric of the specified kind
+    /// Consume self and create a metric of the specified value
     /// registered with the MetricsSet
-    pub fn build(self, kind: MetricKind) -> Arc<SQLMetric> {
+    pub fn build(self, value: MetricValue) {
         let Self {
             labels,
             partition,
             metrics,
         } = self;
-        let metric = Arc::new(SQLMetric::new_with_labels(kind, partition, labels));
+        let metric = Arc::new(SQLMetric::new_with_labels(value, partition, labels));
         metrics.register(metric.clone());
-        metric
     }
 
     /// Consume self and create a new counter for recording output rows
     pub fn output_rows(self, partition: usize) -> Count {
-        let metric = self.with_partition(partition).build(MetricKind::OutputRows);
-        Count::new(metric)
+        let count = Count::new();
+        self.with_partition(partition).build(MetricValue::OutputRows(count.clone()));
+        count
     }
 
-    /// Consumes self and creates a new Countr for recording
-    /// some metric of an operators
-    pub fn counter(self, counter_name: &'static str, partition: usize) -> Count {
-        let metric = self
+    /// Consumes self and creates a new [`Count`] for recording some
+    /// arbitrary metric of an operator.
+    pub fn counter(self, counter_name: impl Into<Cow<'static, str>>, partition: usize) -> Count {
+        self
             .with_partition(partition)
-            .build(MetricKind::Custom(counter_name));
-        Count::new(metric)
+            .global_counter(counter_name)
     }
 
-    /// Consumes self and creates a new Counter for recording
-    /// some metric of an overall operator (not per partition
-    pub fn global_counter(self, counter_name: &'static str) -> Count {
-        let metric = self.build(MetricKind::Custom(counter_name));
-        Count::new(metric)
+    /// Consumes self and creates a new [`Count`] for recording a
+    /// metric of an overall operator (not per partition)
+    pub fn global_counter(self, counter_name: impl Into<Cow<'static, str>>) -> Count {
+        let count = Count::new();
+        self.build(MetricValue::Count {
+            name: counter_name.into(),
+            value: count.clone(),
+        });
+        count
     }
 
     /// Consume self and create a new Timer for recording the overall cpu time
     /// spent by an operator
     pub fn cpu_time(self, partition: usize) -> Time {
-        let metric = self.with_partition(partition).build(MetricKind::CPUTime);
-        Time::new(metric)
+        let time = Time::new();
+        self.with_partition(partition)
+            .build(MetricValue::CPUTime(time.clone()));
+        time
     }
 
     /// Consumes self and creates a new Timer for recording some
-    /// subset of of an operators execution time
-    pub fn subset_time(self, subset_name: &'static str, partition: usize) -> Time {
-        let metric = self
-            .with_partition(partition)
-            .build(MetricKind::Custom(subset_name));
-        Time::new(metric)
+    /// subset of of an operators execution time.
+    pub fn subset_time(self, subset_name: impl Into<Cow<'static, str>>, partition: usize) -> Time {
+        let time = Time::new();
+        self.with_partition(partition)
+            .build(MetricValue::Time {
+                name: subset_name.into(),
+                value: time.clone()
+            });
+        time
+    }
+}
+
+
+/// A counter to record things such as number of input or output rows
+///
+/// Note `clone`ing counters update the same underlying metrics
+#[derive(Debug, Clone)]
+pub struct Count {
+    /// value of the metric counter
+    value: Arc<AtomicUsize>,
+}
+
+impl Count {
+    /// create a new counter
+    pub fn new() -> Self {
+        Self {
+            value : Arc::new(AtomicUsize::new(0))
+        }
+    }
+
+    /// Add `n` to the metric's value
+    pub fn add(&self, n: usize) {
+        // relaxed ordering for operations on `value` poses no issues
+        // we're purely using atomic ops with no associated memory ops
+        self.value.fetch_add(n, Ordering::Relaxed);
+    }
+
+    /// Get the current value
+    pub fn value(&self) -> usize {
+        self.value.load(Ordering::Relaxed)
     }
 }
 
+/// a SQLMetric for CPU timing information
+#[derive(Debug, Clone)]
+pub struct Time {
+    /// elapsed time, in nanoseconds
+    nanos: Arc<AtomicUsize>,
+}
+
+impl Time {
+    /// Create a new [`Time`] wrapper suitable for recording elapsed
+    /// times for operations.
+    pub fn new() -> Self {
+        Self {
+            nanos: Arc::new(AtomicUsize::new(0))
+        }
+    }
+
+    /// Add elapsed nanoseconds since `start`to self
+    pub fn add_elapsed(&self, start: Instant) {
+        let more_nanos = start.elapsed().as_nanos() as usize;
+        self.nanos.fetch_add(more_nanos, Ordering::Relaxed);
+    }
+
+    /// return a scoped guard that adds the amount of time elapsed
+    /// between its creation and its drop or call to `stop` to the
+    /// underlying metric.
+    pub fn timer(&self) -> ScopedTimerGuard<'_> {
+        ScopedTimerGuard {
+            inner: self,
+            start: Some(Instant::now()),
+        }
+    }
+
+    /// Get the number of nanoseconds record by this Time metric
+    pub fn value(&self) -> usize {
+        self.nanos.load(Ordering::Relaxed)
+    }
+}
+
+/// RAAI structure that adds all time between its construction and
+/// destruction to the CPU time or the first call to `stop` whichever
+/// comes first
+pub struct ScopedTimerGuard<'a> {
+    inner: &'a Time,
+    start: Option<Instant>,
+}
+
+impl<'a> ScopedTimerGuard<'a> {
+    /// Stop the timer timing and record the time taken
+    pub fn stop(&mut self) {
+        if let Some(start) = self.start.take() {
+            self.inner.add_elapsed(start)
+        }
+    }
+
+    /// Stop the timer, record the time taken and consume self
+    pub fn done(mut self) {
+        self.stop()
+    }
+}
+
+impl<'a> Drop for ScopedTimerGuard<'a> {
+    fn drop(&mut self) {
+        self.stop()
+    }
+}
+
+/// Possible values for each metric. Among other differences, the
+/// metric types have various ways to display their values and some
+/// metrics are so common they are given special treatment.
+#[derive(Debug, Clone)]
+pub enum MetricValue {
+    /// Number of output rows produced
+    OutputRows(Count),
+    /// CPU time
+    CPUTime(Time),
+    // Operator defined count
+    Count {
+        name: Cow<'static, str>,
+        value: Count
+    },
+    // Operator defined time
+    Time {
+        name: Cow<'static, str>,
+        value: Time
+    }
+    // TODO timestamp, etc
+    // https://github.com/apache/arrow-datafusion/issues/866
+}
+
+
+impl MetricValue {
+    pub fn name(&self) -> &str {
+        // match self {
+        //     MetricKind::OutputRows => "outputRows",
+        //     MetricKind::Custom(name) => name,
+        //     MetricKind::CPUTime => "cpuTime",
+        // }
+        todo!();
+    }
+}
+
+impl Display for MetricValue {
+    /// Prints the value of this metric
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        todo!()
+        // // and now the value
+        // let format_as_duration = match &self.kind {
+        //     MetricKind::OutputRows => false,
+        //     MetricKind::CPUTime => true,
+        //     MetricKind::Custom(name) => name.contains("Time") || name.contains("time"),
+        // };
+
+        // if format_as_duration {
+        //     let duration = std::time::Duration::from_nanos(self.value() as u64);
+        //     write!(f, "={:?}", duration)
+        // } else {
+        //     write!(f, "={}", self.value())
+        // }
+    }
+}
+
+
 /// Something that tracks the metrics of an execution using an atomic
 /// usize
 #[derive(Debug)]
 pub struct SQLMetric {
     /// value of the metric
-    value: AtomicUsize,
+    value: MetricValue,
 
     /// arbitrary name=value pairs identifiying this metric
     labels: Vec<Label>,
@@ -140,14 +300,11 @@ pub struct SQLMetric {
     /// To which partition of an operators output did this metric
     /// apply? If None means all partitions.
     partition: Option<usize>,
-
-    /// The kind of metric (how to logically interpret the value)
-    kind: MetricKind,
 }
 
 impl Display for SQLMetric {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        write!(f, "{}", self.kind)?;
+        write!(f, "{}", self.value.name())?;
 
         let mut iter = self
             .partition
@@ -175,43 +332,32 @@ impl Display for SQLMetric {
         }
 
         // and now the value
-        let format_as_duration = match &self.kind {
-            MetricKind::OutputRows => false,
-            MetricKind::CPUTime => true,
-            MetricKind::Custom(name) => name.contains("Time") || name.contains("time"),
-        };
-
-        if format_as_duration {
-            let duration = std::time::Duration::from_nanos(self.value() as u64);
-            write!(f, "={:?}", duration)
-        } else {
-            write!(f, "={}", self.value())
-        }
+        write!(f, "{}", self.value)
     }
 }
 
 impl SQLMetric {
-    /// Create a new SQLMetric
-    pub fn new(kind: MetricKind, partition: Option<usize>) -> Self {
+    /// Create a new SQLMetric. Consider using [`MetricBuilder`]
+    /// rather than this function directly.
+    pub fn new(value: MetricValue, partition: Option<usize>) -> Self {
         Self {
-            value: 0.into(),
+            value,
             labels: vec![],
             partition,
-            kind,
         }
     }
 
-    /// Add a new label to this metric
+    /// Create a new SQLMetric. Consider using [`MetricBuilder`]
+    /// rather than this function directly.
     pub fn new_with_labels(
-        kind: MetricKind,
+        value: MetricValue,
         partition: Option<usize>,
         labels: Vec<Label>,
     ) -> Self {
         Self {
-            value: 0.into(),
+            value,
             labels,
             partition,
-            kind,
         }
     }
 
@@ -221,65 +367,16 @@ impl SQLMetric {
         self
     }
 
-    /// Add the standard name for output rows
-
-    /// Get the current value
-    pub fn value(&self) -> usize {
-        self.value.load(Ordering::Relaxed)
-    }
-
-    /// get the kind of this metric
-    pub fn kind(&self) -> &MetricKind {
-        &self.kind
-    }
-
-    /// Add `n` to the metric's value
-    pub fn add(&self, n: usize) {
-        // relaxed ordering for operations on `value` poses no issues
-        // we're purely using atomic ops with no associated memory ops
-        self.value.fetch_add(n, Ordering::Relaxed);
-    }
-
-    /// Set the metric's value to `n`
-    pub fn set(&self, n: usize) {
-        self.value.store(n, Ordering::Relaxed);
-    }
-
     /// What labels are present for this metric?
     fn labels(&self) -> &[Label] {
         &self.labels
     }
-}
 
-#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
-/// How should the value of the metric be interpreted?
-pub enum MetricKind {
-    /// Number of output rows produced
-    OutputRows,
-    /// CPU time
-    CPUTime,
-    // TODO timestamp, etc
-    // https://github.com/apache/arrow-datafusion/issues/866
-    /// Arbitarary user defined type
-    Custom(&'static str),
-}
-
-impl Display for MetricKind {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        write!(f, "{}", self.name())
+    /// return the value of this metric
+    pub fn value(&self) -> &MetricValue {
     }
 }
 
-impl MetricKind {
-    /// return a user displayable name of this kind of metric
-    pub fn name(&self) -> &str {
-        match self {
-            MetricKind::OutputRows => "outputRows",
-            MetricKind::Custom(name) => name,
-            MetricKind::CPUTime => "cpuTime",
-        }
-    }
-}
 
 /// A set of SQLMetrics for a particular operator
 #[derive(Default, Debug, Clone)]
diff --git a/datafusion/src/physical_plan/metrics/wrappers.rs b/datafusion/src/physical_plan/metrics/wrappers.rs
deleted file mode 100644
index 0a023713494f3..0000000000000
--- a/datafusion/src/physical_plan/metrics/wrappers.rs
+++ /dev/null
@@ -1,105 +0,0 @@
-// Licensed to the Apache Software Foundation (ASF) under one
-// or more contributor license agreements.  See the NOTICE file
-// distributed with this work for additional information
-// regarding copyright ownership.  The ASF licenses this file
-// to you under the Apache License, Version 2.0 (the
-// "License"); you may not use this file except in compliance
-// with the License.  You may obtain a copy of the License at
-//
-//   http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing,
-// software distributed under the License is distributed on an
-// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
-// KIND, either express or implied.  See the License for the
-// specific language governing permissions and limitations
-// under the License.
-
-//!  wrappers for `SQLMetrics` for more conveniently recording execution metrics
-
-use std::{sync::Arc, time::Instant};
-
-use super::SQLMetric;
-
-// pub trait MetricSource {
-//     /// Return the underlying metric
-//     pub fn metric(&self) -> Arc<SQLMetric>;
-// }
-
-/// a SQLMetric wrapper for a counter (number of input or output rows)
-///
-/// Note `clone` counters update the same underlying metrics
-#[derive(Debug, Clone)]
-pub struct Count {
-    inner: Arc<SQLMetric>,
-}
-
-impl Count {
-    /// create a new counter wrapper around this metric
-    pub fn new(inner: Arc<SQLMetric>) -> Self {
-        Self { inner }
-    }
-
-    /// Add `n` to the counter's value
-    pub fn add(&self, n: usize) {
-        self.inner.add(n)
-    }
-}
-
-/// a SQLMetric wrapper for CPU timing information
-#[derive(Debug, Clone)]
-pub struct Time {
-    inner: Arc<SQLMetric>,
-}
-
-impl Time {
-    /// Create a new [`Time`] wrapper suitable for recording elapsed
-    /// times for operations.
-    pub fn new(inner: Arc<SQLMetric>) -> Self {
-        Self { inner }
-    }
-
-    /// Add elapsed nanoseconds since `start`to self
-    pub fn add_elapsed(&self, start: Instant) {
-        let nanos = start.elapsed().as_nanos() as usize;
-        self.inner.add(nanos)
-    }
-
-    /// return a scoped guard that adds the amount of time elapsed
-    /// between its creation and its drop or call to `stop` to the
-    /// underlying metric.
-    pub fn timer(&self) -> ScopedTimerGuard<'_> {
-        ScopedTimerGuard {
-            inner: self,
-            start: Some(Instant::now()),
-        }
-    }
-}
-
-/// RAAI structure that adds all time between its construction and
-/// destruction to the CPU time or the first call to `stop` whichever
-/// comes first
-pub struct ScopedTimerGuard<'a> {
-    inner: &'a Time,
-    start: Option<Instant>,
-}
-
-impl<'a> ScopedTimerGuard<'a> {
-    /// Stop the timer timing and record the time taken
-    pub fn stop(&mut self) {
-        if let Some(start) = self.start.take() {
-            self.inner.add_elapsed(start)
-        }
-    }
-
-    /// Stop the timer, record the time taken and consume self
-    pub fn done(mut self) {
-        self.stop()
-    }
-}
-
-impl<'a> Drop for ScopedTimerGuard<'a> {
-    fn drop(&mut self) {
-        self.stop()
-    }
-}
diff --git a/datafusion/src/physical_plan/parquet.rs b/datafusion/src/physical_plan/parquet.rs
index 8416551e43cde..ff00b6b4e0853 100644
--- a/datafusion/src/physical_plan/parquet.rs
+++ b/datafusion/src/physical_plan/parquet.rs
@@ -105,9 +105,9 @@ pub struct ParquetPartition {
 #[derive(Debug, Clone)]
 struct ParquetFileMetrics {
     /// Numer of times the predicate could not be evaluated
-    pub predicate_evaluation_errors: metrics::wrappers::Count,
+    pub predicate_evaluation_errors: metrics::Count,
     /// Number of row groups pruned using
-    pub row_groups_pruned: metrics::wrappers::Count,
+    pub row_groups_pruned: metrics::Count,
 }
 
 impl ParquetExec {
diff --git a/datafusion/src/physical_plan/repartition.rs b/datafusion/src/physical_plan/repartition.rs
index 75429efba0190..6587bd4f79488 100644
--- a/datafusion/src/physical_plan/repartition.rs
+++ b/datafusion/src/physical_plan/repartition.rs
@@ -69,11 +69,11 @@ pub struct RepartitionExec {
 #[derive(Debug, Clone)]
 struct RepartitionMetrics {
     /// Time in nanos to execute child operator and fetch batches
-    fetch_time: metrics::wrappers::Time,
+    fetch_time: metrics::Time,
     /// Time in nanos to perform repartitioning
-    repart_time: metrics::wrappers::Time,
+    repart_time: metrics::Time,
     /// Time in nanos for sending resulting batches to channels
-    send_time: metrics::wrappers::Time,
+    send_time: metrics::Time,
 }
 
 impl RepartitionMetrics {
diff --git a/datafusion/src/physical_plan/sort.rs b/datafusion/src/physical_plan/sort.rs
index 959b71fa392dd..f1346b5b1d715 100644
--- a/datafusion/src/physical_plan/sort.rs
+++ b/datafusion/src/physical_plan/sort.rs
@@ -17,8 +17,7 @@
 
 //! Defines the SORT plan
 
-use super::metrics::wrappers::{Count, Time};
-use super::metrics::{ExecutionPlanMetricsSet, MetricBuilder, MetricsSet};
+use super::metrics::{self, ExecutionPlanMetricsSet, MetricBuilder, MetricsSet};
 use super::{RecordBatchStream, SendableRecordBatchStream};
 use crate::error::{DataFusionError, Result};
 use crate::physical_plan::expressions::PhysicalSortExpr;
@@ -227,7 +226,7 @@ pin_project! {
         output: futures::channel::oneshot::Receiver<ArrowResult<Option<RecordBatch>>>,
         finished: bool,
         schema: SchemaRef,
-        output_rows: Count
+        output_rows: metrics::Count
     }
 }
 
@@ -235,8 +234,8 @@ impl SortStream {
     fn new(
         input: SendableRecordBatchStream,
         expr: Vec<PhysicalSortExpr>,
-        output_rows: Count,
-        sort_time: Time,
+        output_rows: metrics::Count,
+        sort_time: metrics::Time,
     ) -> Self {
         let (tx, rx) = futures::channel::oneshot::channel();
         let schema = input.schema();

From cdf7a0d66ce58ba8fec1fa521c3ce839e34774ce Mon Sep 17 00:00:00 2001
From: Andrew Lamb <andrew@nerdnetworks.org>
Date: Wed, 18 Aug 2021 16:14:00 -0400
Subject: [PATCH 12/17] checkpoint for compiling

---
 datafusion/src/physical_plan/metrics.rs | 135 ++++++++++++++++--------
 1 file changed, 90 insertions(+), 45 deletions(-)

diff --git a/datafusion/src/physical_plan/metrics.rs b/datafusion/src/physical_plan/metrics.rs
index 58831396721db..ba8546734155e 100644
--- a/datafusion/src/physical_plan/metrics.rs
+++ b/datafusion/src/physical_plan/metrics.rs
@@ -17,10 +17,15 @@
 
 //! Metrics for recording information about execution
 
-use std::{borrow::Cow, fmt::{Debug, Display}, sync::{
+use std::{
+    borrow::Cow,
+    fmt::{Debug, Display},
+    sync::{
         atomic::{AtomicUsize, Ordering},
         Arc, Mutex,
-    }, time::Instant};
+    },
+    time::Instant,
+};
 
 use hashbrown::HashMap;
 
@@ -89,16 +94,19 @@ impl<'a> MetricBuilder<'a> {
     /// Consume self and create a new counter for recording output rows
     pub fn output_rows(self, partition: usize) -> Count {
         let count = Count::new();
-        self.with_partition(partition).build(MetricValue::OutputRows(count.clone()));
+        self.with_partition(partition)
+            .build(MetricValue::OutputRows(count.clone()));
         count
     }
 
     /// Consumes self and creates a new [`Count`] for recording some
     /// arbitrary metric of an operator.
-    pub fn counter(self, counter_name: impl Into<Cow<'static, str>>, partition: usize) -> Count {
-        self
-            .with_partition(partition)
-            .global_counter(counter_name)
+    pub fn counter(
+        self,
+        counter_name: impl Into<Cow<'static, str>>,
+        partition: usize,
+    ) -> Count {
+        self.with_partition(partition).global_counter(counter_name)
     }
 
     /// Consumes self and creates a new [`Count`] for recording a
@@ -107,7 +115,7 @@ impl<'a> MetricBuilder<'a> {
         let count = Count::new();
         self.build(MetricValue::Count {
             name: counter_name.into(),
-            value: count.clone(),
+            count: count.clone(),
         });
         count
     }
@@ -123,18 +131,20 @@ impl<'a> MetricBuilder<'a> {
 
     /// Consumes self and creates a new Timer for recording some
     /// subset of of an operators execution time.
-    pub fn subset_time(self, subset_name: impl Into<Cow<'static, str>>, partition: usize) -> Time {
+    pub fn subset_time(
+        self,
+        subset_name: impl Into<Cow<'static, str>>,
+        partition: usize,
+    ) -> Time {
         let time = Time::new();
-        self.with_partition(partition)
-            .build(MetricValue::Time {
-                name: subset_name.into(),
-                value: time.clone()
-            });
+        self.with_partition(partition).build(MetricValue::Time {
+            name: subset_name.into(),
+            time: time.clone(),
+        });
         time
     }
 }
 
-
 /// A counter to record things such as number of input or output rows
 ///
 /// Note `clone`ing counters update the same underlying metrics
@@ -148,7 +158,7 @@ impl Count {
     /// create a new counter
     pub fn new() -> Self {
         Self {
-            value : Arc::new(AtomicUsize::new(0))
+            value: Arc::new(AtomicUsize::new(0)),
         }
     }
 
@@ -177,7 +187,7 @@ impl Time {
     /// times for operations.
     pub fn new() -> Self {
         Self {
-            nanos: Arc::new(AtomicUsize::new(0))
+            nanos: Arc::new(AtomicUsize::new(0)),
         }
     }
 
@@ -240,21 +250,16 @@ pub enum MetricValue {
     OutputRows(Count),
     /// CPU time
     CPUTime(Time),
-    // Operator defined count
+    /// Operator defined count
     Count {
         name: Cow<'static, str>,
-        value: Count
+        count: Count,
     },
-    // Operator defined time
-    Time {
-        name: Cow<'static, str>,
-        value: Time
-    }
-    // TODO timestamp, etc
-    // https://github.com/apache/arrow-datafusion/issues/866
+    /// Operator defined time
+    Time { name: Cow<'static, str>, time: Time }, // TODO timestamp, etc
+                                                  // https://github.com/apache/arrow-datafusion/issues/866
 }
 
-
 impl MetricValue {
     pub fn name(&self) -> &str {
         // match self {
@@ -264,6 +269,34 @@ impl MetricValue {
         // }
         todo!();
     }
+
+    /// Return the value of the metric as a usize value
+    pub fn as_usize(&self) -> usize {
+        match self {
+            MetricValue::OutputRows(count) => count.value(),
+            MetricValue::CPUTime(time) => time.value(),
+            MetricValue::Count { count, .. } => count.value(),
+            MetricValue::Time { time, .. } => time.value(),
+        }
+    }
+
+    /// create a new MetricValue with the same type as `self` suitable
+    /// for accumulating
+    pub fn new_empty(&self) -> Self {
+        todo!()
+    }
+
+    /// Add the value of other to this. panic's if the type is mismatched or
+    /// aggregating does not make sense for this value
+    ///
+    /// Note this is purposely marked `mut` (even though atomics are
+    /// used) so Rust's type system can be used to ensure the
+    /// appropriate API access. `MetricValues` should be modified
+    /// using the original [`Count`] or [`Time`] they were created
+    /// from.
+    pub fn add(&mut self, other: &Self) {
+        todo!()
+    }
 }
 
 impl Display for MetricValue {
@@ -286,7 +319,6 @@ impl Display for MetricValue {
     }
 }
 
-
 /// Something that tracks the metrics of an execution using an atomic
 /// usize
 #[derive(Debug)]
@@ -372,11 +404,16 @@ impl SQLMetric {
         &self.labels
     }
 
-    /// return the value of this metric
+    /// return a reference to the value of this metric
     pub fn value(&self) -> &MetricValue {
+        &self.value
     }
-}
 
+    /// return a mutable reference to the value of this metric
+    pub fn value_mut(&mut self) -> &mut MetricValue {
+        &mut self.value
+    }
+}
 
 /// A set of SQLMetrics for a particular operator
 #[derive(Default, Debug, Clone)]
@@ -403,19 +440,21 @@ impl MetricsSet {
     /// convenience: return the number of rows produced, aggregated
     /// across partitions or None if no metric is present
     pub fn output_rows(&self) -> Option<usize> {
-        self.sum(|metric| matches!(metric.kind(), MetricKind::OutputRows))
+        self.sum(|metric| matches!(metric.value(), MetricValue::OutputRows(_)))
+            .map(|v| v.as_usize())
     }
 
     /// convenience: return the amount of CPU time spent, aggregated
     /// across partitions or None if no metric is present
     pub fn cpu_time(&self) -> Option<usize> {
-        self.sum(|metric| matches!(metric.kind(), MetricKind::CPUTime))
+        self.sum(|metric| matches!(metric.value(), MetricValue::CPUTime(_)))
+            .map(|v| v.as_usize())
     }
 
     /// Sums the values for metrics for which `f(metric)` returns
     /// true, and returns the value. Returns None if no metrics match
     /// the predicate.
-    pub fn sum<F>(&self, mut f: F) -> Option<usize>
+    pub fn sum<F>(&self, mut f: F) -> Option<MetricValue>
     where
         F: FnMut(&SQLMetric) -> bool,
     {
@@ -425,35 +464,41 @@ impl MetricsSet {
             .filter(|metric| f(metric.as_ref()))
             .peekable();
 
-        if iter.peek().is_none() {
-            None
-        } else {
-            Some(iter.map(|metric| metric.value()).sum())
-        }
+        let mut accum = match iter.peek() {
+            None => {
+                return None;
+            }
+            Some(metric) => metric.value().new_empty(),
+        };
+
+        iter.for_each(|metric| accum.add(metric.value()));
+
+        Some(accum)
     }
 
     /// Returns returns a new derived `MetricsSet` where all metrics
-    /// that had partition=`Some(..)` have been aggregated
-    /// together. The resulting `MetricsSet` has all metrics with `Partition=None`
+    /// that had the same name and partition=`Some(..)` have been
+    /// aggregated together. The resulting `MetricsSet` has all
+    /// metrics with `Partition=None`
     pub fn aggregate_by_partition(&self) -> Self {
         let mut map = HashMap::new();
 
         // There are all sorts of ways to make this more efficient
         for metric in &self.metrics {
-            let key = (metric.kind, metric.labels.clone());
+            let key = (metric.value.name(), metric.labels.clone());
             map.entry(key)
                 .and_modify(|accum: &mut SQLMetric| {
-                    accum.set(accum.value() + metric.value())
+                    accum.value_mut().add(metric.value());
                 })
                 .or_insert_with(|| {
                     // accumulate with no partition
                     let partition = None;
-                    let accum = SQLMetric::new_with_labels(
-                        *metric.kind(),
+                    let mut accum = SQLMetric::new_with_labels(
+                        metric.value().new_empty(),
                         partition,
                         metric.labels().to_vec(),
                     );
-                    accum.set(metric.value());
+                    accum.value_mut().add(metric.value());
                     accum
                 });
         }

From 8b464f307ad1d30319ed8e9e73c8ea88b206cb41 Mon Sep 17 00:00:00 2001
From: Andrew Lamb <andrew@nerdnetworks.org>
Date: Wed, 18 Aug 2021 16:36:11 -0400
Subject: [PATCH 13/17] Implement last bits

---
 datafusion/src/physical_plan/metrics.rs | 110 ++++++++++++++++--------
 datafusion/tests/parquet_pruning.rs     |   3 +-
 datafusion/tests/sql.rs                 |   2 +
 3 files changed, 80 insertions(+), 35 deletions(-)

diff --git a/datafusion/src/physical_plan/metrics.rs b/datafusion/src/physical_plan/metrics.rs
index ba8546734155e..16c926a494209 100644
--- a/datafusion/src/physical_plan/metrics.rs
+++ b/datafusion/src/physical_plan/metrics.rs
@@ -18,7 +18,7 @@
 //! Metrics for recording information about execution
 
 use std::{
-    borrow::Cow,
+    borrow::{Borrow, Cow},
     fmt::{Debug, Display},
     sync::{
         atomic::{AtomicUsize, Ordering},
@@ -197,6 +197,11 @@ impl Time {
         self.nanos.fetch_add(more_nanos, Ordering::Relaxed);
     }
 
+    /// Add the number of nanoseconds of other `Time` to self
+    pub fn add(&self, other: &Time) {
+        self.nanos.fetch_add(other.value(), Ordering::Relaxed);
+    }
+
     /// return a scoped guard that adds the amount of time elapsed
     /// between its creation and its drop or call to `stop` to the
     /// underlying metric.
@@ -246,47 +251,67 @@ impl<'a> Drop for ScopedTimerGuard<'a> {
 /// metrics are so common they are given special treatment.
 #[derive(Debug, Clone)]
 pub enum MetricValue {
-    /// Number of output rows produced
+    /// Number of output rows produced: "output_rows" metric
     OutputRows(Count),
-    /// CPU time
+    /// CPU time: the "cpu_time" metric
     CPUTime(Time),
-    /// Operator defined count
+    /// Operator defined count.
     Count {
+        /// The provided name of this metric
         name: Cow<'static, str>,
+        /// The value of the metric
         count: Count,
     },
     /// Operator defined time
-    Time { name: Cow<'static, str>, time: Time }, // TODO timestamp, etc
-                                                  // https://github.com/apache/arrow-datafusion/issues/866
+    Time {
+        /// The provided name of this metric
+        name: Cow<'static, str>,
+        /// The value of the metric
+        time: Time,
+    },
+    // TODO timestamp, etc
+    // https://github.com/apache/arrow-datafusion/issues/866
 }
 
 impl MetricValue {
+    /// Return the name of this SQL metric
     pub fn name(&self) -> &str {
-        // match self {
-        //     MetricKind::OutputRows => "outputRows",
-        //     MetricKind::Custom(name) => name,
-        //     MetricKind::CPUTime => "cpuTime",
-        // }
-        todo!();
+        match self {
+            Self::OutputRows(_) => "output_rows",
+            Self::CPUTime(_) => "cpu_time",
+            Self::Count { name, .. } => name.borrow(),
+            Self::Time { name, .. } => name.borrow(),
+        }
     }
 
     /// Return the value of the metric as a usize value
     pub fn as_usize(&self) -> usize {
         match self {
-            MetricValue::OutputRows(count) => count.value(),
-            MetricValue::CPUTime(time) => time.value(),
-            MetricValue::Count { count, .. } => count.value(),
-            MetricValue::Time { time, .. } => time.value(),
+            Self::OutputRows(count) => count.value(),
+            Self::CPUTime(time) => time.value(),
+            Self::Count { count, .. } => count.value(),
+            Self::Time { time, .. } => time.value(),
         }
     }
 
     /// create a new MetricValue with the same type as `self` suitable
     /// for accumulating
     pub fn new_empty(&self) -> Self {
-        todo!()
+        match self {
+            Self::OutputRows(_) => Self::OutputRows(Count::new()),
+            Self::CPUTime(_) => Self::CPUTime(Time::new()),
+            Self::Count { name, .. } => Self::Count {
+                name: name.clone(),
+                count: Count::new(),
+            },
+            Self::Time { name, .. } => Self::Time {
+                name: name.clone(),
+                time: Time::new(),
+            },
+        }
     }
 
-    /// Add the value of other to this. panic's if the type is mismatched or
+    /// Add the value of other to `self`. panic's if the type is mismatched or
     /// aggregating does not make sense for this value
     ///
     /// Note this is purposely marked `mut` (even though atomics are
@@ -295,27 +320,44 @@ impl MetricValue {
     /// using the original [`Count`] or [`Time`] they were created
     /// from.
     pub fn add(&mut self, other: &Self) {
-        todo!()
+        match (self, other) {
+            (Self::OutputRows(count), Self::OutputRows(other_count))
+            | (
+                Self::Count { count, .. },
+                Self::Count {
+                    count: other_count, ..
+                },
+            ) => count.add(other_count.value()),
+            (Self::CPUTime(time), Self::CPUTime(other_time))
+            | (
+                Self::Time { time, .. },
+                Self::Time {
+                    time: other_time, ..
+                },
+            ) => time.add(other_time),
+            m @ (_, _) => {
+                panic!(
+                    "Mismatched types aggregating metric values. Expected {:?} got {:?}",
+                    m.0.name(),
+                    m.1.name()
+                )
+            }
+        }
     }
 }
 
 impl Display for MetricValue {
     /// Prints the value of this metric
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        todo!()
-        // // and now the value
-        // let format_as_duration = match &self.kind {
-        //     MetricKind::OutputRows => false,
-        //     MetricKind::CPUTime => true,
-        //     MetricKind::Custom(name) => name.contains("Time") || name.contains("time"),
-        // };
-
-        // if format_as_duration {
-        //     let duration = std::time::Duration::from_nanos(self.value() as u64);
-        //     write!(f, "={:?}", duration)
-        // } else {
-        //     write!(f, "={}", self.value())
-        // }
+        match self {
+            Self::OutputRows(count) | Self::Count { count, .. } => {
+                write!(f, "{}", count.value())
+            }
+            Self::CPUTime(time) | Self::Time { time, .. } => {
+                let duration = std::time::Duration::from_nanos(time.value() as u64);
+                write!(f, "{:?}", duration)
+            }
+        }
     }
 }
 
@@ -364,7 +406,7 @@ impl Display for SQLMetric {
         }
 
         // and now the value
-        write!(f, "{}", self.value)
+        write!(f, "={}", self.value)
     }
 }
 
diff --git a/datafusion/tests/parquet_pruning.rs b/datafusion/tests/parquet_pruning.rs
index edcaeeef68cfc..a682fc7647557 100644
--- a/datafusion/tests/parquet_pruning.rs
+++ b/datafusion/tests/parquet_pruning.rs
@@ -445,7 +445,8 @@ impl TestOutput {
     /// retrieve the value of the named metric, if any
     fn metric_value(&self, metric_name: &str) -> Option<usize> {
         self.parquet_metrics
-            .sum(|metric| metric.kind().name() == metric_name)
+            .sum(|metric| metric.value().name() == metric_name)
+            .map(|v| v.as_usize())
     }
 
     /// The number of times the pruning predicate evaluation errors
diff --git a/datafusion/tests/sql.rs b/datafusion/tests/sql.rs
index 0f385680deed4..40da631379695 100644
--- a/datafusion/tests/sql.rs
+++ b/datafusion/tests/sql.rs
@@ -2150,6 +2150,8 @@ async fn csv_explain_analyze() {
     let formatted = arrow::util::pretty::pretty_format_batches(&actual).unwrap();
     let formatted = normalize_for_explain(&formatted);
 
+    println!("ANALYZE EXPLAIN:\n{}", formatted);
+
     // Only test basic plumbing and try to avoid having to change too
     // many things
     let needle = "RepartitionExec: partitioning=RoundRobinBatch(NUM_CORES), metrics=[";

From 128c6c1d9f91d81a114ab97026f71d5d1042e15a Mon Sep 17 00:00:00 2001
From: Andrew Lamb <andrew@nerdnetworks.org>
Date: Thu, 19 Aug 2021 07:06:11 -0400
Subject: [PATCH 14/17] Break into modules

---
 datafusion/src/physical_plan/metrics.rs       | 356 +-----------------
 .../src/physical_plan/metrics/builder.rs      | 138 +++++++
 datafusion/src/physical_plan/metrics/value.rs | 245 ++++++++++++
 datafusion/src/physical_plan/mod.rs           |   7 +-
 4 files changed, 404 insertions(+), 342 deletions(-)
 create mode 100644 datafusion/src/physical_plan/metrics/builder.rs
 create mode 100644 datafusion/src/physical_plan/metrics/value.rs

diff --git a/datafusion/src/physical_plan/metrics.rs b/datafusion/src/physical_plan/metrics.rs
index 16c926a494209..ae0975cc16e24 100644
--- a/datafusion/src/physical_plan/metrics.rs
+++ b/datafusion/src/physical_plan/metrics.rs
@@ -17,355 +17,30 @@
 
 //! Metrics for recording information about execution
 
+mod builder;
+mod value;
+
 use std::{
-    borrow::{Borrow, Cow},
+    borrow::Cow,
     fmt::{Debug, Display},
-    sync::{
-        atomic::{AtomicUsize, Ordering},
-        Arc, Mutex,
-    },
-    time::Instant,
+    sync::{Arc, Mutex},
 };
 
 use hashbrown::HashMap;
 
-/// Structure for constructing metrics, counters, timers, etc.
-///
-/// Note the use of `Cow<..>` is to avoid allocations in the common
-/// case of constant strings
-///
-/// ```rust
-/// TODO doc example
-/// ```
-pub struct MetricBuilder<'a> {
-    /// Location that the metric created by this builder will be added do
-    metrics: &'a ExecutionPlanMetricsSet,
-
-    /// optional partition number
-    partition: Option<usize>,
-
-    /// arbitrary name=value pairs identifiying this metric
-    labels: Vec<Label>,
-}
-
-impl<'a> MetricBuilder<'a> {
-    /// Create a new `MetricBuilder` that will register the result of `build()` with the `metrics`
-    pub fn new(metrics: &'a ExecutionPlanMetricsSet) -> Self {
-        Self {
-            metrics,
-            partition: None,
-            labels: vec![],
-        }
-    }
-
-    /// Add a label to the metric being constructed
-    pub fn with_label(mut self, label: Label) -> Self {
-        self.labels.push(label);
-        self
-    }
-
-    /// Add a label to the metric being constructed
-    pub fn with_new_label(
-        self,
-        name: impl Into<Cow<'static, str>>,
-        value: impl Into<Cow<'static, str>>,
-    ) -> Self {
-        self.with_label(Label::new(name.into(), value.into()))
-    }
-
-    /// Set the partition of the metric being constructed
-    pub fn with_partition(mut self, partition: usize) -> Self {
-        self.partition = Some(partition);
-        self
-    }
-
-    /// Consume self and create a metric of the specified value
-    /// registered with the MetricsSet
-    pub fn build(self, value: MetricValue) {
-        let Self {
-            labels,
-            partition,
-            metrics,
-        } = self;
-        let metric = Arc::new(SQLMetric::new_with_labels(value, partition, labels));
-        metrics.register(metric.clone());
-    }
-
-    /// Consume self and create a new counter for recording output rows
-    pub fn output_rows(self, partition: usize) -> Count {
-        let count = Count::new();
-        self.with_partition(partition)
-            .build(MetricValue::OutputRows(count.clone()));
-        count
-    }
-
-    /// Consumes self and creates a new [`Count`] for recording some
-    /// arbitrary metric of an operator.
-    pub fn counter(
-        self,
-        counter_name: impl Into<Cow<'static, str>>,
-        partition: usize,
-    ) -> Count {
-        self.with_partition(partition).global_counter(counter_name)
-    }
-
-    /// Consumes self and creates a new [`Count`] for recording a
-    /// metric of an overall operator (not per partition)
-    pub fn global_counter(self, counter_name: impl Into<Cow<'static, str>>) -> Count {
-        let count = Count::new();
-        self.build(MetricValue::Count {
-            name: counter_name.into(),
-            count: count.clone(),
-        });
-        count
-    }
-
-    /// Consume self and create a new Timer for recording the overall cpu time
-    /// spent by an operator
-    pub fn cpu_time(self, partition: usize) -> Time {
-        let time = Time::new();
-        self.with_partition(partition)
-            .build(MetricValue::CPUTime(time.clone()));
-        time
-    }
-
-    /// Consumes self and creates a new Timer for recording some
-    /// subset of of an operators execution time.
-    pub fn subset_time(
-        self,
-        subset_name: impl Into<Cow<'static, str>>,
-        partition: usize,
-    ) -> Time {
-        let time = Time::new();
-        self.with_partition(partition).build(MetricValue::Time {
-            name: subset_name.into(),
-            time: time.clone(),
-        });
-        time
-    }
-}
+// public exports
+pub use builder::MetricBuilder;
+pub use value::{Count, MetricValue, ScopedTimerGuard, Time};
 
-/// A counter to record things such as number of input or output rows
+/// Something that tracks a value of interest (metric) of a DataFusion
+/// [`ExecutionPlan`] execution.
 ///
-/// Note `clone`ing counters update the same underlying metrics
-#[derive(Debug, Clone)]
-pub struct Count {
-    /// value of the metric counter
-    value: Arc<AtomicUsize>,
-}
-
-impl Count {
-    /// create a new counter
-    pub fn new() -> Self {
-        Self {
-            value: Arc::new(AtomicUsize::new(0)),
-        }
-    }
-
-    /// Add `n` to the metric's value
-    pub fn add(&self, n: usize) {
-        // relaxed ordering for operations on `value` poses no issues
-        // we're purely using atomic ops with no associated memory ops
-        self.value.fetch_add(n, Ordering::Relaxed);
-    }
-
-    /// Get the current value
-    pub fn value(&self) -> usize {
-        self.value.load(Ordering::Relaxed)
-    }
-}
-
-/// a SQLMetric for CPU timing information
-#[derive(Debug, Clone)]
-pub struct Time {
-    /// elapsed time, in nanoseconds
-    nanos: Arc<AtomicUsize>,
-}
-
-impl Time {
-    /// Create a new [`Time`] wrapper suitable for recording elapsed
-    /// times for operations.
-    pub fn new() -> Self {
-        Self {
-            nanos: Arc::new(AtomicUsize::new(0)),
-        }
-    }
-
-    /// Add elapsed nanoseconds since `start`to self
-    pub fn add_elapsed(&self, start: Instant) {
-        let more_nanos = start.elapsed().as_nanos() as usize;
-        self.nanos.fetch_add(more_nanos, Ordering::Relaxed);
-    }
-
-    /// Add the number of nanoseconds of other `Time` to self
-    pub fn add(&self, other: &Time) {
-        self.nanos.fetch_add(other.value(), Ordering::Relaxed);
-    }
-
-    /// return a scoped guard that adds the amount of time elapsed
-    /// between its creation and its drop or call to `stop` to the
-    /// underlying metric.
-    pub fn timer(&self) -> ScopedTimerGuard<'_> {
-        ScopedTimerGuard {
-            inner: self,
-            start: Some(Instant::now()),
-        }
-    }
-
-    /// Get the number of nanoseconds record by this Time metric
-    pub fn value(&self) -> usize {
-        self.nanos.load(Ordering::Relaxed)
-    }
-}
-
-/// RAAI structure that adds all time between its construction and
-/// destruction to the CPU time or the first call to `stop` whichever
-/// comes first
-pub struct ScopedTimerGuard<'a> {
-    inner: &'a Time,
-    start: Option<Instant>,
-}
-
-impl<'a> ScopedTimerGuard<'a> {
-    /// Stop the timer timing and record the time taken
-    pub fn stop(&mut self) {
-        if let Some(start) = self.start.take() {
-            self.inner.add_elapsed(start)
-        }
-    }
-
-    /// Stop the timer, record the time taken and consume self
-    pub fn done(mut self) {
-        self.stop()
-    }
-}
-
-impl<'a> Drop for ScopedTimerGuard<'a> {
-    fn drop(&mut self) {
-        self.stop()
-    }
-}
-
-/// Possible values for each metric. Among other differences, the
-/// metric types have various ways to display their values and some
-/// metrics are so common they are given special treatment.
-#[derive(Debug, Clone)]
-pub enum MetricValue {
-    /// Number of output rows produced: "output_rows" metric
-    OutputRows(Count),
-    /// CPU time: the "cpu_time" metric
-    CPUTime(Time),
-    /// Operator defined count.
-    Count {
-        /// The provided name of this metric
-        name: Cow<'static, str>,
-        /// The value of the metric
-        count: Count,
-    },
-    /// Operator defined time
-    Time {
-        /// The provided name of this metric
-        name: Cow<'static, str>,
-        /// The value of the metric
-        time: Time,
-    },
-    // TODO timestamp, etc
-    // https://github.com/apache/arrow-datafusion/issues/866
-}
-
-impl MetricValue {
-    /// Return the name of this SQL metric
-    pub fn name(&self) -> &str {
-        match self {
-            Self::OutputRows(_) => "output_rows",
-            Self::CPUTime(_) => "cpu_time",
-            Self::Count { name, .. } => name.borrow(),
-            Self::Time { name, .. } => name.borrow(),
-        }
-    }
-
-    /// Return the value of the metric as a usize value
-    pub fn as_usize(&self) -> usize {
-        match self {
-            Self::OutputRows(count) => count.value(),
-            Self::CPUTime(time) => time.value(),
-            Self::Count { count, .. } => count.value(),
-            Self::Time { time, .. } => time.value(),
-        }
-    }
-
-    /// create a new MetricValue with the same type as `self` suitable
-    /// for accumulating
-    pub fn new_empty(&self) -> Self {
-        match self {
-            Self::OutputRows(_) => Self::OutputRows(Count::new()),
-            Self::CPUTime(_) => Self::CPUTime(Time::new()),
-            Self::Count { name, .. } => Self::Count {
-                name: name.clone(),
-                count: Count::new(),
-            },
-            Self::Time { name, .. } => Self::Time {
-                name: name.clone(),
-                time: Time::new(),
-            },
-        }
-    }
-
-    /// Add the value of other to `self`. panic's if the type is mismatched or
-    /// aggregating does not make sense for this value
-    ///
-    /// Note this is purposely marked `mut` (even though atomics are
-    /// used) so Rust's type system can be used to ensure the
-    /// appropriate API access. `MetricValues` should be modified
-    /// using the original [`Count`] or [`Time`] they were created
-    /// from.
-    pub fn add(&mut self, other: &Self) {
-        match (self, other) {
-            (Self::OutputRows(count), Self::OutputRows(other_count))
-            | (
-                Self::Count { count, .. },
-                Self::Count {
-                    count: other_count, ..
-                },
-            ) => count.add(other_count.value()),
-            (Self::CPUTime(time), Self::CPUTime(other_time))
-            | (
-                Self::Time { time, .. },
-                Self::Time {
-                    time: other_time, ..
-                },
-            ) => time.add(other_time),
-            m @ (_, _) => {
-                panic!(
-                    "Mismatched types aggregating metric values. Expected {:?} got {:?}",
-                    m.0.name(),
-                    m.1.name()
-                )
-            }
-        }
-    }
-}
-
-impl Display for MetricValue {
-    /// Prints the value of this metric
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        match self {
-            Self::OutputRows(count) | Self::Count { count, .. } => {
-                write!(f, "{}", count.value())
-            }
-            Self::CPUTime(time) | Self::Time { time, .. } => {
-                let duration = std::time::Duration::from_nanos(time.value() as u64);
-                write!(f, "{:?}", duration)
-            }
-        }
-    }
-}
-
-/// Something that tracks the metrics of an execution using an atomic
-/// usize
+/// Typically these `SQLMetric`s are not created directly, but instead
+/// are created using [`MetricBuilder`] or methods on
+/// [`ExecutionPlanMetricsSet`].
 #[derive(Debug)]
 pub struct SQLMetric {
-    /// value of the metric
+    /// The value the metric
     value: MetricValue,
 
     /// arbitrary name=value pairs identifiying this metric
@@ -457,7 +132,8 @@ impl SQLMetric {
     }
 }
 
-/// A set of SQLMetrics for a particular operator
+/// A snapshot of the metrics for a particular operator (`dyn
+/// ExecutionPlan`).
 #[derive(Default, Debug, Clone)]
 pub struct MetricsSet {
     metrics: Vec<Arc<SQLMetric>>,
diff --git a/datafusion/src/physical_plan/metrics/builder.rs b/datafusion/src/physical_plan/metrics/builder.rs
new file mode 100644
index 0000000000000..c6f89ddc66a34
--- /dev/null
+++ b/datafusion/src/physical_plan/metrics/builder.rs
@@ -0,0 +1,138 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Builder for creating arbitrary metrics
+
+use std::{borrow::Cow, sync::Arc};
+
+use super::{Count, ExecutionPlanMetricsSet, Label, MetricValue, SQLMetric, Time};
+
+/// Structure for constructing metrics, counters, timers, etc.
+///
+/// Note the use of `Cow<..>` is to avoid allocations in the common
+/// case of constant strings
+///
+/// ```rust
+/// TODO doc example
+/// ```
+pub struct MetricBuilder<'a> {
+    /// Location that the metric created by this builder will be added do
+    metrics: &'a ExecutionPlanMetricsSet,
+
+    /// optional partition number
+    partition: Option<usize>,
+
+    /// arbitrary name=value pairs identifiying this metric
+    labels: Vec<Label>,
+}
+
+impl<'a> MetricBuilder<'a> {
+    /// Create a new `MetricBuilder` that will register the result of `build()` with the `metrics`
+    pub fn new(metrics: &'a ExecutionPlanMetricsSet) -> Self {
+        Self {
+            metrics,
+            partition: None,
+            labels: vec![],
+        }
+    }
+
+    /// Add a label to the metric being constructed
+    pub fn with_label(mut self, label: Label) -> Self {
+        self.labels.push(label);
+        self
+    }
+
+    /// Add a label to the metric being constructed
+    pub fn with_new_label(
+        self,
+        name: impl Into<Cow<'static, str>>,
+        value: impl Into<Cow<'static, str>>,
+    ) -> Self {
+        self.with_label(Label::new(name.into(), value.into()))
+    }
+
+    /// Set the partition of the metric being constructed
+    pub fn with_partition(mut self, partition: usize) -> Self {
+        self.partition = Some(partition);
+        self
+    }
+
+    /// Consume self and create a metric of the specified value
+    /// registered with the MetricsSet
+    pub fn build(self, value: MetricValue) {
+        let Self {
+            labels,
+            partition,
+            metrics,
+        } = self;
+        let metric = Arc::new(SQLMetric::new_with_labels(value, partition, labels));
+        metrics.register(metric.clone());
+    }
+
+    /// Consume self and create a new counter for recording output rows
+    pub fn output_rows(self, partition: usize) -> Count {
+        let count = Count::new();
+        self.with_partition(partition)
+            .build(MetricValue::OutputRows(count.clone()));
+        count
+    }
+
+    /// Consumes self and creates a new [`Count`] for recording some
+    /// arbitrary metric of an operator.
+    pub fn counter(
+        self,
+        counter_name: impl Into<Cow<'static, str>>,
+        partition: usize,
+    ) -> Count {
+        self.with_partition(partition).global_counter(counter_name)
+    }
+
+    /// Consumes self and creates a new [`Count`] for recording a
+    /// metric of an overall operator (not per partition)
+    pub fn global_counter(self, counter_name: impl Into<Cow<'static, str>>) -> Count {
+        let count = Count::new();
+        self.build(MetricValue::Count {
+            name: counter_name.into(),
+            count: count.clone(),
+        });
+        count
+    }
+
+    /// Consume self and create a new Timer for recording the overall cpu time
+    /// spent by an operator
+    pub fn cpu_time(self, partition: usize) -> Time {
+        let time = Time::new();
+        self.with_partition(partition)
+            .build(MetricValue::CPUTime(time.clone()));
+        time
+    }
+
+    /// Consumes self and creates a new Timer for recording some
+    /// subset of of an operators execution time.
+    pub fn subset_time(
+        self,
+        subset_name: impl Into<Cow<'static, str>>,
+        partition: usize,
+    ) -> Time {
+        let time = Time::new();
+        self.with_partition(partition).build(MetricValue::Time {
+            name: subset_name.into(),
+            time: time.clone(),
+        });
+        time
+    }
+}
diff --git a/datafusion/src/physical_plan/metrics/value.rs b/datafusion/src/physical_plan/metrics/value.rs
new file mode 100644
index 0000000000000..60e90f97e521d
--- /dev/null
+++ b/datafusion/src/physical_plan/metrics/value.rs
@@ -0,0 +1,245 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Value representation of metrics
+
+use std::{
+    borrow::{Borrow, Cow},
+    sync::{
+        atomic::{AtomicUsize, Ordering},
+        Arc,
+    },
+    time::Instant,
+};
+
+/// A counter to record things such as number of input or output rows
+///
+/// Note `clone`ing counters update the same underlying metrics
+#[derive(Debug, Clone)]
+pub struct Count {
+    /// value of the metric counter
+    value: std::sync::Arc<AtomicUsize>,
+}
+
+impl Count {
+    /// create a new counter
+    pub fn new() -> Self {
+        Self {
+            value: Arc::new(AtomicUsize::new(0)),
+        }
+    }
+
+    /// Add `n` to the metric's value
+    pub fn add(&self, n: usize) {
+        // relaxed ordering for operations on `value` poses no issues
+        // we're purely using atomic ops with no associated memory ops
+        self.value.fetch_add(n, Ordering::Relaxed);
+    }
+
+    /// Get the current value
+    pub fn value(&self) -> usize {
+        self.value.load(Ordering::Relaxed)
+    }
+}
+
+/// a SQLMetric for CPU timing information
+#[derive(Debug, Clone)]
+pub struct Time {
+    /// elapsed time, in nanoseconds
+    nanos: Arc<AtomicUsize>,
+}
+
+impl Time {
+    /// Create a new [`Time`] wrapper suitable for recording elapsed
+    /// times for operations.
+    pub fn new() -> Self {
+        Self {
+            nanos: Arc::new(AtomicUsize::new(0)),
+        }
+    }
+
+    /// Add elapsed nanoseconds since `start`to self
+    pub fn add_elapsed(&self, start: Instant) {
+        let more_nanos = start.elapsed().as_nanos() as usize;
+        self.nanos.fetch_add(more_nanos, Ordering::Relaxed);
+    }
+
+    /// Add the number of nanoseconds of other `Time` to self
+    pub fn add(&self, other: &Time) {
+        self.nanos.fetch_add(other.value(), Ordering::Relaxed);
+    }
+
+    /// return a scoped guard that adds the amount of time elapsed
+    /// between its creation and its drop or call to `stop` to the
+    /// underlying metric.
+    pub fn timer(&self) -> ScopedTimerGuard<'_> {
+        ScopedTimerGuard {
+            inner: self,
+            start: Some(Instant::now()),
+        }
+    }
+
+    /// Get the number of nanoseconds record by this Time metric
+    pub fn value(&self) -> usize {
+        self.nanos.load(Ordering::Relaxed)
+    }
+}
+
+/// RAAI structure that adds all time between its construction and
+/// destruction to the CPU time or the first call to `stop` whichever
+/// comes first
+pub struct ScopedTimerGuard<'a> {
+    inner: &'a Time,
+    start: Option<Instant>,
+}
+
+impl<'a> ScopedTimerGuard<'a> {
+    /// Stop the timer timing and record the time taken
+    pub fn stop(&mut self) {
+        if let Some(start) = self.start.take() {
+            self.inner.add_elapsed(start)
+        }
+    }
+
+    /// Stop the timer, record the time taken and consume self
+    pub fn done(mut self) {
+        self.stop()
+    }
+}
+
+impl<'a> Drop for ScopedTimerGuard<'a> {
+    fn drop(&mut self) {
+        self.stop()
+    }
+}
+
+/// Possible values for a metric.
+///
+/// Among other differences, the metric types have different ways to
+/// logically interpret their underlying values and some metrics are
+/// so common they are given special treatment.
+#[derive(Debug, Clone)]
+pub enum MetricValue {
+    /// Number of output rows produced: "output_rows" metric
+    OutputRows(Count),
+    /// CPU time: the "cpu_time" metric
+    CPUTime(Time),
+    /// Operator defined count.
+    Count {
+        /// The provided name of this metric
+        name: Cow<'static, str>,
+        /// The value of the metric
+        count: Count,
+    },
+    /// Operator defined time
+    Time {
+        /// The provided name of this metric
+        name: Cow<'static, str>,
+        /// The value of the metric
+        time: Time,
+    },
+    // TODO timestamp, etc
+    // https://github.com/apache/arrow-datafusion/issues/866
+}
+
+impl MetricValue {
+    /// Return the name of this SQL metric
+    pub fn name(&self) -> &str {
+        match self {
+            Self::OutputRows(_) => "output_rows",
+            Self::CPUTime(_) => "cpu_time",
+            Self::Count { name, .. } => name.borrow(),
+            Self::Time { name, .. } => name.borrow(),
+        }
+    }
+
+    /// Return the value of the metric as a usize value
+    pub fn as_usize(&self) -> usize {
+        match self {
+            Self::OutputRows(count) => count.value(),
+            Self::CPUTime(time) => time.value(),
+            Self::Count { count, .. } => count.value(),
+            Self::Time { time, .. } => time.value(),
+        }
+    }
+
+    /// create a new MetricValue with the same type as `self` suitable
+    /// for accumulating
+    pub fn new_empty(&self) -> Self {
+        match self {
+            Self::OutputRows(_) => Self::OutputRows(Count::new()),
+            Self::CPUTime(_) => Self::CPUTime(Time::new()),
+            Self::Count { name, .. } => Self::Count {
+                name: name.clone(),
+                count: Count::new(),
+            },
+            Self::Time { name, .. } => Self::Time {
+                name: name.clone(),
+                time: Time::new(),
+            },
+        }
+    }
+
+    /// Add the value of other to `self`. panic's if the type is mismatched or
+    /// aggregating does not make sense for this value
+    ///
+    /// Note this is purposely marked `mut` (even though atomics are
+    /// used) so Rust's type system can be used to ensure the
+    /// appropriate API access. `MetricValues` should be modified
+    /// using the original [`Count`] or [`Time`] they were created
+    /// from.
+    pub fn add(&mut self, other: &Self) {
+        match (self, other) {
+            (Self::OutputRows(count), Self::OutputRows(other_count))
+            | (
+                Self::Count { count, .. },
+                Self::Count {
+                    count: other_count, ..
+                },
+            ) => count.add(other_count.value()),
+            (Self::CPUTime(time), Self::CPUTime(other_time))
+            | (
+                Self::Time { time, .. },
+                Self::Time {
+                    time: other_time, ..
+                },
+            ) => time.add(other_time),
+            m @ (_, _) => {
+                panic!(
+                    "Mismatched types aggregating metric values. Expected {:?} got {:?}",
+                    m.0.name(),
+                    m.1.name()
+                )
+            }
+        }
+    }
+}
+
+impl std::fmt::Display for MetricValue {
+    /// Prints the value of this metric
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            Self::OutputRows(count) | Self::Count { count, .. } => {
+                write!(f, "{}", count.value())
+            }
+            Self::CPUTime(time) | Self::Time { time, .. } => {
+                let duration = std::time::Duration::from_nanos(time.value() as u64);
+                write!(f, "{:?}", duration)
+            }
+        }
+    }
+}
diff --git a/datafusion/src/physical_plan/mod.rs b/datafusion/src/physical_plan/mod.rs
index 9ebde71d9e596..c17615d6432eb 100644
--- a/datafusion/src/physical_plan/mod.rs
+++ b/datafusion/src/physical_plan/mod.rs
@@ -129,8 +129,11 @@ pub trait ExecutionPlan: Debug + Send + Sync {
     /// Return a copy of the set of [`SQLMetric`]s for this
     /// `ExecutionPlan`.
     ///
-    /// The [`MetricsSet`]s returned may not be the complete list of
-    /// metrics. Once `self.execute()` has returned (aka the future is
+    /// While the values of the metrics in the returned
+    /// [`MetricsSet`]s may change as execution progresses, the
+    /// specific metrics will not.
+    ///
+    /// Once `self.execute()` has returned (technically the future is
     /// resolved) for all available partitions, the set of metrics
     /// should be complete. If this function is called prior to
     /// `execute()` new metrics may appear in subsequent calls.

From b3b4aa09fdd2382a0a5b6d601c2f92276a017881 Mon Sep 17 00:00:00 2001
From: Andrew Lamb <andrew@nerdnetworks.org>
Date: Thu, 19 Aug 2021 07:10:34 -0400
Subject: [PATCH 15/17] Rename SQLMetric -> Metric

---
 .../src/execution_plans/shuffle_reader.rs     |  4 +-
 .../src/execution_plans/shuffle_writer.rs     |  2 +-
 ballista/rust/core/src/utils.rs               |  2 +-
 datafusion/src/physical_plan/metrics.rs       | 42 +++++++------------
 .../src/physical_plan/metrics/builder.rs      |  4 +-
 datafusion/src/physical_plan/metrics/value.rs |  2 +-
 datafusion/src/physical_plan/mod.rs           |  6 +--
 7 files changed, 23 insertions(+), 39 deletions(-)

diff --git a/ballista/rust/core/src/execution_plans/shuffle_reader.rs b/ballista/rust/core/src/execution_plans/shuffle_reader.rs
index 15ccf11881f17..ab7e2a91b59f1 100644
--- a/ballista/rust/core/src/execution_plans/shuffle_reader.rs
+++ b/ballista/rust/core/src/execution_plans/shuffle_reader.rs
@@ -31,9 +31,7 @@ use datafusion::arrow::record_batch::RecordBatch;
 use datafusion::physical_plan::metrics::{
     ExecutionPlanMetricsSet, MetricBuilder, MetricsSet,
 };
-use datafusion::physical_plan::{
-    DisplayFormatType, ExecutionPlan, Partitioning, SQLMetric,
-};
+use datafusion::physical_plan::{DisplayFormatType, ExecutionPlan, Metric, Partitioning};
 use datafusion::{
     error::{DataFusionError, Result},
     physical_plan::RecordBatchStream,
diff --git a/ballista/rust/core/src/execution_plans/shuffle_writer.rs b/ballista/rust/core/src/execution_plans/shuffle_writer.rs
index 3b8160d9d692d..746e701643ad7 100644
--- a/ballista/rust/core/src/execution_plans/shuffle_writer.rs
+++ b/ballista/rust/core/src/execution_plans/shuffle_writer.rs
@@ -51,7 +51,7 @@ use datafusion::physical_plan::metrics::{
 use datafusion::physical_plan::repartition::RepartitionExec;
 use datafusion::physical_plan::Partitioning::RoundRobinBatch;
 use datafusion::physical_plan::{
-    DisplayFormatType, ExecutionPlan, Partitioning, RecordBatchStream, SQLMetric,
+    DisplayFormatType, ExecutionPlan, Metric, Partitioning, RecordBatchStream,
 };
 use futures::StreamExt;
 use hashbrown::HashMap;
diff --git a/ballista/rust/core/src/utils.rs b/ballista/rust/core/src/utils.rs
index 92429e83d1fcc..a79dbd79ffca8 100644
--- a/ballista/rust/core/src/utils.rs
+++ b/ballista/rust/core/src/utils.rs
@@ -61,7 +61,7 @@ use datafusion::physical_plan::parquet::ParquetExec;
 use datafusion::physical_plan::projection::ProjectionExec;
 use datafusion::physical_plan::sort::SortExec;
 use datafusion::physical_plan::{
-    metrics, AggregateExpr, ExecutionPlan, PhysicalExpr, RecordBatchStream, SQLMetric,
+    metrics, AggregateExpr, ExecutionPlan, Metric, PhysicalExpr, RecordBatchStream,
 };
 use futures::{future, Stream, StreamExt};
 use std::time::Instant;
diff --git a/datafusion/src/physical_plan/metrics.rs b/datafusion/src/physical_plan/metrics.rs
index ae0975cc16e24..d47a4868fa7e6 100644
--- a/datafusion/src/physical_plan/metrics.rs
+++ b/datafusion/src/physical_plan/metrics.rs
@@ -35,11 +35,11 @@ pub use value::{Count, MetricValue, ScopedTimerGuard, Time};
 /// Something that tracks a value of interest (metric) of a DataFusion
 /// [`ExecutionPlan`] execution.
 ///
-/// Typically these `SQLMetric`s are not created directly, but instead
+/// Typically [`Metric`]s are not created directly, but instead
 /// are created using [`MetricBuilder`] or methods on
 /// [`ExecutionPlanMetricsSet`].
 #[derive(Debug)]
-pub struct SQLMetric {
+pub struct Metric {
     /// The value the metric
     value: MetricValue,
 
@@ -51,7 +51,7 @@ pub struct SQLMetric {
     partition: Option<usize>,
 }
 
-impl Display for SQLMetric {
+impl Display for Metric {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         write!(f, "{}", self.value.name())?;
 
@@ -85,8 +85,8 @@ impl Display for SQLMetric {
     }
 }
 
-impl SQLMetric {
-    /// Create a new SQLMetric. Consider using [`MetricBuilder`]
+impl Metric {
+    /// Create a new [`Metric`]. Consider using [`MetricBuilder`]
     /// rather than this function directly.
     pub fn new(value: MetricValue, partition: Option<usize>) -> Self {
         Self {
@@ -96,7 +96,7 @@ impl SQLMetric {
         }
     }
 
-    /// Create a new SQLMetric. Consider using [`MetricBuilder`]
+    /// Create a new [`Metric`]. Consider using [`MetricBuilder`]
     /// rather than this function directly.
     pub fn new_with_labels(
         value: MetricValue,
@@ -136,7 +136,7 @@ impl SQLMetric {
 /// ExecutionPlan`).
 #[derive(Default, Debug, Clone)]
 pub struct MetricsSet {
-    metrics: Vec<Arc<SQLMetric>>,
+    metrics: Vec<Arc<Metric>>,
 }
 
 impl MetricsSet {
@@ -146,15 +146,10 @@ impl MetricsSet {
     }
 
     /// Add the specified metric
-    pub fn push(&mut self, metric: Arc<SQLMetric>) {
+    pub fn push(&mut self, metric: Arc<Metric>) {
         self.metrics.push(metric)
     }
 
-    /// Add all [`SQLMetric`]s in this set to the specified array.
-    fn extend_other(&mut self, metrics: &mut Vec<Arc<SQLMetric>>) {
-        metrics.extend(self.metrics.iter().cloned())
-    }
-
     /// convenience: return the number of rows produced, aggregated
     /// across partitions or None if no metric is present
     pub fn output_rows(&self) -> Option<usize> {
@@ -174,7 +169,7 @@ impl MetricsSet {
     /// the predicate.
     pub fn sum<F>(&self, mut f: F) -> Option<MetricValue>
     where
-        F: FnMut(&SQLMetric) -> bool,
+        F: FnMut(&Metric) -> bool,
     {
         let mut iter = self
             .metrics
@@ -205,13 +200,13 @@ impl MetricsSet {
         for metric in &self.metrics {
             let key = (metric.value.name(), metric.labels.clone());
             map.entry(key)
-                .and_modify(|accum: &mut SQLMetric| {
+                .and_modify(|accum: &mut Metric| {
                     accum.value_mut().add(metric.value());
                 })
                 .or_insert_with(|| {
                     // accumulate with no partition
                     let partition = None;
-                    let mut accum = SQLMetric::new_with_labels(
+                    let mut accum = Metric::new_with_labels(
                         metric.value().new_empty(),
                         partition,
                         metric.labels().to_vec(),
@@ -249,7 +244,7 @@ impl Display for MetricsSet {
     }
 }
 
-/// A set of SQLMetrics for an individual "operator" (e.g. `&dyn
+/// A set of [`Metric`] for an individual "operator" (e.g. `&dyn
 /// ExecutionPlan`).
 ///
 /// This structure is intended as a convenience for [`ExecutionPlan`]
@@ -271,20 +266,11 @@ impl ExecutionPlanMetricsSet {
         }
     }
 
-    /// Add the specified metric
-    pub fn register(&self, metric: Arc<SQLMetric>) {
+    /// Add the specified metric to the underlying metric set
+    pub fn register(&self, metric: Arc<Metric>) {
         self.inner.lock().expect("not poisoned").push(metric)
     }
 
-    /// Add all [`SQLMetric`]s for this `ExecutionPlan` to the
-    /// specified array.
-    pub fn extend_other(&self, metrics: &mut Vec<Arc<SQLMetric>>) {
-        self.inner
-            .lock()
-            .expect("not poisoned")
-            .extend_other(metrics)
-    }
-
     /// Return a clone of the inner MetricsSet
     pub fn clone_inner(&self) -> MetricsSet {
         let guard = self.inner.lock().expect("not poisoned");
diff --git a/datafusion/src/physical_plan/metrics/builder.rs b/datafusion/src/physical_plan/metrics/builder.rs
index c6f89ddc66a34..5f07b49b09c5f 100644
--- a/datafusion/src/physical_plan/metrics/builder.rs
+++ b/datafusion/src/physical_plan/metrics/builder.rs
@@ -19,7 +19,7 @@
 
 use std::{borrow::Cow, sync::Arc};
 
-use super::{Count, ExecutionPlanMetricsSet, Label, MetricValue, SQLMetric, Time};
+use super::{Count, ExecutionPlanMetricsSet, Label, Metric, MetricValue, Time};
 
 /// Structure for constructing metrics, counters, timers, etc.
 ///
@@ -79,7 +79,7 @@ impl<'a> MetricBuilder<'a> {
             partition,
             metrics,
         } = self;
-        let metric = Arc::new(SQLMetric::new_with_labels(value, partition, labels));
+        let metric = Arc::new(Metric::new_with_labels(value, partition, labels));
         metrics.register(metric.clone());
     }
 
diff --git a/datafusion/src/physical_plan/metrics/value.rs b/datafusion/src/physical_plan/metrics/value.rs
index 60e90f97e521d..495a46c137697 100644
--- a/datafusion/src/physical_plan/metrics/value.rs
+++ b/datafusion/src/physical_plan/metrics/value.rs
@@ -56,7 +56,7 @@ impl Count {
     }
 }
 
-/// a SQLMetric for CPU timing information
+/// Measure a potentially non contiguous duration of time
 #[derive(Debug, Clone)]
 pub struct Time {
     /// elapsed time, in nanoseconds
diff --git a/datafusion/src/physical_plan/mod.rs b/datafusion/src/physical_plan/mod.rs
index c17615d6432eb..82c297c8e40ed 100644
--- a/datafusion/src/physical_plan/mod.rs
+++ b/datafusion/src/physical_plan/mod.rs
@@ -17,8 +17,8 @@
 
 //! Traits for physical query plan, supporting parallel execution for partitioned relations.
 
+pub use self::metrics::Metric;
 use self::metrics::MetricsSet;
-pub use self::metrics::SQLMetric;
 use self::{
     coalesce_partitions::CoalescePartitionsExec, display::DisplayableExecutionPlan,
 };
@@ -126,8 +126,8 @@ pub trait ExecutionPlan: Debug + Send + Sync {
     /// creates an iterator
     async fn execute(&self, partition: usize) -> Result<SendableRecordBatchStream>;
 
-    /// Return a copy of the set of [`SQLMetric`]s for this
-    /// `ExecutionPlan`.
+    /// Return a snapshot of the set of [`Metric`]s for this
+    /// [`ExecutionPlan`].
     ///
     /// While the values of the metrics in the returned
     /// [`MetricsSet`]s may change as execution progresses, the

From f0e4c629b47dcc543c3c9754c07155f63e5e391e Mon Sep 17 00:00:00 2001
From: Andrew Lamb <andrew@nerdnetworks.org>
Date: Thu, 19 Aug 2021 07:13:27 -0400
Subject: [PATCH 16/17] fixup test

---
 datafusion/tests/sql.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/datafusion/tests/sql.rs b/datafusion/tests/sql.rs
index 40da631379695..23859cf17a1e6 100644
--- a/datafusion/tests/sql.rs
+++ b/datafusion/tests/sql.rs
@@ -2161,7 +2161,7 @@ async fn csv_explain_analyze() {
         needle,
         formatted
     );
-    let verbose_needle = "Output Rows       | 5";
+    let verbose_needle = "Output Rows            | 5";
     assert!(
         !formatted.contains(verbose_needle),
         "found unexpected '{}' in\n{}",

From c07de69c58f11772f2b92f0d2ebb91bd302405e5 Mon Sep 17 00:00:00 2001
From: Andrew Lamb <andrew@nerdnetworks.org>
Date: Thu, 19 Aug 2021 07:22:20 -0400
Subject: [PATCH 17/17] use snake case

---
 ballista/rust/core/src/execution_plans/shuffle_reader.rs | 2 +-
 ballista/rust/core/src/execution_plans/shuffle_writer.rs | 4 ++--
 ballista/rust/core/src/utils.rs                          | 2 +-
 datafusion/src/physical_plan/parquet.rs                  | 6 +++---
 datafusion/src/physical_plan/repartition.rs              | 6 +++---
 datafusion/tests/parquet_pruning.rs                      | 4 ++--
 datafusion/tests/sql.rs                                  | 4 ++--
 7 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/ballista/rust/core/src/execution_plans/shuffle_reader.rs b/ballista/rust/core/src/execution_plans/shuffle_reader.rs
index ab7e2a91b59f1..bc5dbc175c7ad 100644
--- a/ballista/rust/core/src/execution_plans/shuffle_reader.rs
+++ b/ballista/rust/core/src/execution_plans/shuffle_reader.rs
@@ -102,7 +102,7 @@ impl ExecutionPlan for ShuffleReaderExec {
         info!("ShuffleReaderExec::execute({})", partition);
 
         let fetch_time =
-            MetricBuilder::new(&self.metrics).subset_time("fetchTime", partition);
+            MetricBuilder::new(&self.metrics).subset_time("fetch_time", partition);
         let timer = fetch_time.timer();
 
         let partition_locations = &self.partition[partition];
diff --git a/ballista/rust/core/src/execution_plans/shuffle_writer.rs b/ballista/rust/core/src/execution_plans/shuffle_writer.rs
index 746e701643ad7..36e445bc4ead0 100644
--- a/ballista/rust/core/src/execution_plans/shuffle_writer.rs
+++ b/ballista/rust/core/src/execution_plans/shuffle_writer.rs
@@ -88,9 +88,9 @@ struct ShuffleWriteMetrics {
 
 impl ShuffleWriteMetrics {
     fn new(partition: usize, metrics: &ExecutionPlanMetricsSet) -> Self {
-        let write_time = MetricBuilder::new(metrics).subset_time("writeTime", partition);
+        let write_time = MetricBuilder::new(metrics).subset_time("write_time", partition);
 
-        let input_rows = MetricBuilder::new(metrics).counter("inputRows", partition);
+        let input_rows = MetricBuilder::new(metrics).counter("input_rows", partition);
 
         let output_rows = MetricBuilder::new(metrics).output_rows(partition);
 
diff --git a/ballista/rust/core/src/utils.rs b/ballista/rust/core/src/utils.rs
index a79dbd79ffca8..bf0d152631ed1 100644
--- a/ballista/rust/core/src/utils.rs
+++ b/ballista/rust/core/src/utils.rs
@@ -71,7 +71,7 @@ use std::time::Instant;
 pub async fn write_stream_to_disk(
     stream: &mut Pin<Box<dyn RecordBatchStream + Send + Sync>>,
     path: &str,
-    disk_write_metric: &metrics::wrappers::Time,
+    disk_write_metric: &metrics::Time,
 ) -> Result<PartitionStats> {
     let file = File::create(&path).map_err(|e| {
         BallistaError::General(format!(
diff --git a/datafusion/src/physical_plan/parquet.rs b/datafusion/src/physical_plan/parquet.rs
index ff00b6b4e0853..f85495cee1e91 100644
--- a/datafusion/src/physical_plan/parquet.rs
+++ b/datafusion/src/physical_plan/parquet.rs
@@ -410,7 +410,7 @@ impl ParquetExec {
 
         let metrics = ExecutionPlanMetricsSet::new();
         let predicate_creation_errors =
-            MetricBuilder::new(&metrics).global_counter("numPredicateCreationErrors");
+            MetricBuilder::new(&metrics).global_counter("num_predicate_creation_errors");
 
         let predicate_builder = predicate.and_then(|predicate_expr| {
             match PruningPredicate::try_new(&predicate_expr, schema.clone()) {
@@ -614,11 +614,11 @@ impl ParquetFileMetrics {
     ) -> Self {
         let predicate_evaluation_errors = MetricBuilder::new(metrics)
             .with_new_label("filename", filename.to_string())
-            .counter("numPredicateEvaluationErrors", partition);
+            .counter("predicate_evaluation_errors", partition);
 
         let row_groups_pruned = MetricBuilder::new(metrics)
             .with_new_label("filename", filename.to_string())
-            .counter("numRowGroupsPruned", partition);
+            .counter("row_groups_pruned", partition);
 
         Self {
             predicate_evaluation_errors,
diff --git a/datafusion/src/physical_plan/repartition.rs b/datafusion/src/physical_plan/repartition.rs
index 6587bd4f79488..8ba9a4f3ad47d 100644
--- a/datafusion/src/physical_plan/repartition.rs
+++ b/datafusion/src/physical_plan/repartition.rs
@@ -87,17 +87,17 @@ impl RepartitionMetrics {
         // Time in nanos to execute child operator and fetch batches
         let fetch_time = MetricBuilder::new(metrics)
             .with_label(label.clone())
-            .subset_time("fetchTime", output_partition);
+            .subset_time("fetch_time", output_partition);
 
         // Time in nanos to perform repartitioning
         let repart_time = MetricBuilder::new(metrics)
             .with_label(label.clone())
-            .subset_time("repartTime", output_partition);
+            .subset_time("repart_time", output_partition);
 
         // Time in nanos for sending resulting batches to channels
         let send_time = MetricBuilder::new(metrics)
             .with_label(label)
-            .subset_time("sendTime", output_partition);
+            .subset_time("send_time", output_partition);
 
         Self {
             fetch_time,
diff --git a/datafusion/tests/parquet_pruning.rs b/datafusion/tests/parquet_pruning.rs
index a682fc7647557..14f5dd20f470c 100644
--- a/datafusion/tests/parquet_pruning.rs
+++ b/datafusion/tests/parquet_pruning.rs
@@ -451,12 +451,12 @@ impl TestOutput {
 
     /// The number of times the pruning predicate evaluation errors
     fn predicate_evaluation_errors(&self) -> Option<usize> {
-        self.metric_value("numPredicateEvaluationErrors")
+        self.metric_value("predicate_evaluation_errors")
     }
 
     /// The number of times the pruning predicate evaluation errors
     fn row_groups_pruned(&self) -> Option<usize> {
-        self.metric_value("numRowGroupsPruned")
+        self.metric_value("row_groups_pruned")
     }
 
     fn description(&self) -> String {
diff --git a/datafusion/tests/sql.rs b/datafusion/tests/sql.rs
index 23859cf17a1e6..03da79e39be6c 100644
--- a/datafusion/tests/sql.rs
+++ b/datafusion/tests/sql.rs
@@ -2161,7 +2161,7 @@ async fn csv_explain_analyze() {
         needle,
         formatted
     );
-    let verbose_needle = "Output Rows            | 5";
+    let verbose_needle = "Output Rows";
     assert!(
         !formatted.contains(verbose_needle),
         "found unexpected '{}' in\n{}",
@@ -2181,7 +2181,7 @@ async fn csv_explain_analyze_verbose() {
     let formatted = arrow::util::pretty::pretty_format_batches(&actual).unwrap();
     let formatted = normalize_for_explain(&formatted);
 
-    let verbose_needle = "Output Rows       | 5";
+    let verbose_needle = "Output Rows";
     assert!(
         formatted.contains(verbose_needle),
         "did not find '{}' in\n{}",