kosiew commented on code in PR #24218:
URL: https://github.com/apache/datafusion/pull/24218#discussion_r3842749853
##########
datafusion/physical-expr-common/src/metrics/value.rs:
##########
@@ -665,13 +665,35 @@ pub enum MetricValue {
/// The value of the metric
count: Count,
},
+ /// Operator defined count representing a size in bytes (e.g.
+ /// `bytes_scanned`, `bytes_written`). Like [`Self::Count`], but always
+ /// displayed with [`human_readable_size`]'s 1024-based byte units
+ /// (KB/MB/GB/TB) instead of [`human_readable_count`]'s 1000-based units.
+ BytesCount {
Review Comment:
I think this is still a SemVer-breaking change. `MetricValue` is public and
exhaustive, so adding `BytesCount` and `BytesGauge` breaks downstream
exhaustive matches. `cargo-semver-checks` reports this as `enum_variant_added`.
Because this PR targets `main`, where the workspace version is still
`55.0.0`, and `55.0.0` has already been released from `branch-55` in #24385, we
should avoid adding new variants to this enum here.
Could we keep `MetricValue::Count` and `MetricValue::Gauge`, retain
`MetricCategory::Bytes`, and restore the category-aware display branch so
generic byte metrics use `human_readable_size`? That should preserve the
byte-unit behavior without changing the public Rust enum or the
`FFI_MetricValue` ABI.
The cdylib test could then check the transported `Bytes` category, the
generic variant/name/value, and the byte-formatted display output.
--
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]
