LiaCastaneda commented on code in PR #16195:
URL: https://github.com/apache/datafusion/pull/16195#discussion_r2109164412
##########
datafusion/physical-plan/src/metrics/value.rs:
##########
@@ -589,6 +632,79 @@ mod tests {
use super::*;
+ #[derive(Debug, Default)]
+ pub struct CustomCounter {
+ count: AtomicUsize,
+ }
+
+ impl CustomMetricValue for CustomCounter {
+ fn new_empty(self: &Self) -> Arc<dyn CustomMetricValue> {
+ Arc::new(CustomCounter::default())
+ }
+
+ fn aggregate(self: &Self, other: Arc<dyn CustomMetricValue + 'static>)
{
+ let other = other.as_any().downcast_ref::<Self>().unwrap();
+ self.count
+ .fetch_add(other.count.load(Ordering::Relaxed),
Ordering::Relaxed);
+ }
+
+ fn as_any(self: &Self) -> &dyn Any {
+ self
+ }
+ }
+
+ #[test]
+ #[should_panic(expected = "Unsupported metric aggregation between Hi and
Hello")]
+ fn test_custom_metric_with_mismatching_names() {
+ let custom_counter = CustomCounter::default();
+ let mut custom_val = MetricValue::Custom {
+ name: Cow::Borrowed("Hi"),
+ value: Arc::new(custom_counter),
+ };
+
+ let other_custom_counter = CustomCounter::default();
+ let other_custom_val = MetricValue::Custom {
+ name: Cow::Borrowed("Hello"),
+ value: Arc::new(other_custom_counter),
+ };
+
+ // Should panic
+ custom_val.aggregate(&other_custom_val);
+ }
+
+ #[test]
+ fn test_custom_metric() {
+ let custom_counter = CustomCounter::default();
+ custom_counter.count.fetch_add(11, Ordering::Relaxed);
+ let mut custom_val = MetricValue::Custom {
+ name: Cow::Borrowed("Hi"),
+ value: Arc::new(custom_counter),
+ };
+
+ let other_custom_counter = CustomCounter::default();
+ other_custom_counter.count.fetch_add(20, Ordering::Relaxed);
+ let other_custom_val = MetricValue::Custom {
+ name: Cow::Borrowed("Hi"),
+ value: Arc::new(other_custom_counter),
+ };
+
+ custom_val.aggregate(&other_custom_val);
+
+ if let MetricValue::Custom { value, .. } = custom_val {
Review Comment:
nit: I think this can also be written as
if let MetricValue::Custom { value, .. } = custom_val {
let counter = value
.as_any()
.downcast_ref::<CustomCounter>()
.expect("Expected CustomCounter");
assert_eq!(counter.count.load(Ordering::Relaxed), 31);
}. since custom_val will always be `MetricValue::Custom`
##########
datafusion/physical-plan/src/metrics/value.rs:
##########
@@ -578,6 +618,9 @@ impl Display for MetricValue {
Self::StartTimestamp(timestamp) | Self::EndTimestamp(timestamp) =>
{
write!(f, "{timestamp}")
}
+ Self::Custom { name, value } => {
+ write!(f, "name:{name} {value:?}")
Review Comment:
I think we can also require a `Display` for `Custom`?
--
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
