DevShiba opened a new pull request, #24218: URL: https://github.com/apache/datafusion/pull/24218
## Which issue does this PR close? - Closes https://github.com/apache/datafusion/issues/24203 ## Rationale for this change ``` output_bytes=7.5 GB bytes_scanned=1.26 B ``` `bytes_scanned` is actually 1.26 GB, but reads as "1.26 billion" because it's formatted with `human_readable_count` instead of `human_readable_size`. Root cause: `MetricValue::Count`/`Gauge` are the generic variants used by operator-defined metrics with no dedicated variant of their own (`bytes_scanned`, `stream_memory_usage`, `bytes_written`). Unlike the metrics with dedicated variants (`OutputBytes`, `SpilledBytes`, `CurrentMemoryUsage`, `PeakMemoryUsage`), `MetricValue` itself does not carry the metric's category - only the wrapping `Metric` struct does. So `Display` for the generic `Count`/`Gauge` arms has no way to know it's holding a byte measurement and always falls back to `human_readable_count`'s 1000-based K/M/B/T units, even when the metric was declared with `.with_category(MetricCategory::Bytes)`. This also affects `stream_memory_usage` (a `Gauge`) and `bytes_written` (a `Count`), which share the same root cause but weren't mentioned in the original report - found by grepping for every `.with_category(MetricCategory::Bytes)` call site paired with a generic `.counter()`/`.gauge()`/`.global_counter()` builder. ## What changes are included in this PR? Moves the format decision into `Display for Metric`, which does have both the value and the category, instead of `Display for MetricValue`, which doesn't. A generic `Count`/`Gauge` tagged `Bytes` now uses `human_readable_size` (1024-based KB/MB/GB/TB) like the dedicated byte variants already do. Rows/Timing-category and uncategorized generic metrics are untouched - `human_readable_count` was already correct for them. Updated the 14 hardcoded `bytes_scanned` expected values across two sqllogictest files to match the corrected format. Did this by hand rather than via `--complete`, since `--complete` also baked in non-deterministic timing/path values that those tests intentionally wildcard with `<slt:ignore>` - `--complete`'s output would have made the tests flaky. ## Are these changes tested? Yes. Added two unit tests (`test_display_generic_count_respects_bytes_category`, `test_display_generic_gauge_respects_bytes_category`) covering both the `Count` and `Gauge` cases, plus confirming a `Rows`-category generic counter is untouched. Ran the full sqllogictest suite (502/502 files) to confirm no other fixture was missed, and `cargo check --workspace --all-targets`. ## Are there any user-facing changes? Yes: `EXPLAIN ANALYZE` output for `bytes_scanned`, `stream_memory_usage`, and `bytes_written` now shows correct byte units (e.g. `1.26 GB`) instead of count units (e.g. `1.26 B`, misleadingly meaning "1.26 billion"). -- 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]
