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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
