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