DevShiba commented on PR #24218:
URL: https://github.com/apache/datafusion/pull/24218#issuecomment-5243402608
Good call, thanks for the pointer to #24203's discussion - pushed a rework.
`bytes_scanned`, `bytes_written`, and `stream_memory_usage` are now
registered as two new dedicated `MetricValue` variants instead of being
reinterpreted at `Display` time based on category:
- `BytesCount { name, count }` - a named, byte-formatted `Count` (for
`bytes_scanned`, `bytes_written`), used via a new
`MetricBuilder::bytes_counter`/`global_bytes_counter`.
- `BytesGauge { name, gauge }` - a named, byte-formatted `Gauge` (for
`stream_memory_usage`), used via a new `MetricBuilder::bytes_gauge`.
This follows the exact shape `OutputBytes`/`SpilledBytes`/`PeakMemoryUsage`
already use, and `Display for Metric` goes back to the plain `write!(f, "{}",
self.value)` it had before - `MetricValue` no longer needs help from `Metric`'s
category to know it's holding bytes.
One design note: I didn't fold `stream_memory_usage` into the existing
`PeakMemoryUsage` variant, even though they're structurally identical.
`PeakMemoryUsage`'s current call sites (`peak_mem_used`, `max_mem_used`,
`build_mem_used`) are all monotonically-increasing accumulators, so "peak" is
accurate for them. `stream_memory_usage` is `.set()` to a capacity that can
shrink as a sliding window prunes rows - it's a live/current value, not a peak,
so reusing that variant's name for it would be misleading to anyone reading the
enum or debugging via `EXPLAIN`. Happy to fold them together if you'd rather
keep the surface area smaller and treat "peak" loosely, but figured I'd flag
the reasoning rather than silently pick one.
Also had to update two other places that matched on `MetricValue`
exhaustively, since the compiler doesn't let a match silently ignore new
variants:
- `MetricsSet::sum_by_name`, which `bytes_scanned`'s own tests rely on via
`sum_by_name("bytes_scanned")`.
- `display.rs`'s JSON `EXPLAIN` metric serialization - without an explicit
arm there, the new variants would've fallen through to a string fallback,
turning `bytes_scanned`'s JSON output from a number into a string like `"3.0
GB"`, which would've been a real regression for any JSON `EXPLAIN` consumer.
- The `datafusion-ffi` crate's `FFI_MetricValue` also mirrors `MetricValue`
across an ABI boundary and documents that new variants must be appended at the
end (variant order is part of its stable ABI) - done that, with matching
conversions and round-trip test coverage added in both directions.
Reran the full sqllogictest suite - the rendered `EXPLAIN ANALYZE` text is
byte-for-byte unchanged, so none of the `.slt` fixture updates from the
previous commit needed to 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]
