gabotechs commented on code in PR #16195:
URL: https://github.com/apache/datafusion/pull/16195#discussion_r2109611063


##########
datafusion/physical-plan/src/metrics/value.rs:
##########
@@ -443,6 +460,9 @@ impl MetricValue {
                 .and_then(|ts| ts.timestamp_nanos_opt())
                 .map(|nanos| nanos as usize)
                 .unwrap_or(0),
+            Self::Custom { name, value } => {
+                panic!("MetricValue::as_usize isn't supported for custom 
metric values. ({name}: {value:?})")
+            }

Review Comment:
   Maybe some `CustomMetricValue` implementations would like to implement 
`as_usize(&self)`? 



##########
datafusion/physical-plan/src/metrics/value.rs:
##########
@@ -401,6 +401,22 @@ pub enum MetricValue {
     StartTimestamp(Timestamp),
     /// The time at which execution ended
     EndTimestamp(Timestamp),
+    Custom {
+        /// The provided name of this metric
+        name: Cow<'static, str>,
+        /// A custom implementation of the metric value.
+        value: Arc<dyn CustomMetricValue>,
+    },
+}
+
+/// A custom metric value implementation.
+/// The only constraint is that it should be aggregatable.
+pub trait CustomMetricValue: Display + Debug + Send + Sync {

Review Comment:
   Traits that are part of public DataFusion's API tends to have extensive 
documentation in each method about how users are supposed to implement them, 
take the `TableProvider` as an example:
   
   
https://github.com/apache/datafusion/blob/14beb79a21f827c5990f4429db61c486ec260d63/datafusion/catalog/src/table.rs#L51
   
   It would be nice to ship something similar, and once people agree on the 
actual API of this, it would also be nice to add an example in the same way 
there's one for the other public facing traits, for example
   
   
https://github.com/apache/datafusion/blob/f3b1141d0f417e9d9e6c0ada03592c9d9ec60cd4/datafusion-examples/examples/custom_datasource.rs#L162



##########
datafusion/physical-plan/src/metrics/value.rs:
##########
@@ -344,7 +344,7 @@ impl Drop for ScopedTimerGuard<'_> {
 /// 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, PartialEq)]
+#[derive(Debug, Clone)]

Review Comment:
   Is there any way of keeping this? as the `MetricValue` is exposed to the 
public, this will introduce a non backwards compatible breaking change in the 
API.
   
   I imagine that forcing `PartialEq` in `CustomMetricValue` gets messy because 
I don't think you'll be able to use a dynamic `CustomMetricValue` here, but it 
might be better to try it in order to not introduce the breaking change



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

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

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


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

Reply via email to