fix: format Bytes-category custom metrics with byte units - #24218
fix: format Bytes-category custom metrics with byte units#24218DevShiba wants to merge 14 commits into
Conversation
MetricValue::Count/Gauge are the generic variants used by operator-defined metrics with no dedicated enum variant of their own (e.g. bytes_scanned, stream_memory_usage, bytes_written). Unlike the few 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 MetricValue's generic Count/Gauge arms had no way to know they held a byte measurement and always fell back to human_readable_count's 1000-based K/M/B/T units. Example from EXPLAIN ANALYZE before this change: output_bytes=7.5 GB bytes_scanned=1.26 B <- actually 1.26 GB, using count units (billion) Move the decision into Display for Metric, which does have both the value and the category, so a Bytes-category Count/Gauge 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, since human_readable_count was already correct for them. Also fixes the same bug for stream_memory_usage and bytes_written, which share the same root cause but weren't mentioned in the original report. Updated the 14 hardcoded bytes_scanned expected values across two sqllogictest files to match the corrected format. Deliberately did this by hand rather than via `--complete`, which would have also baked in non-deterministic timing values that the tests intentionally wildcard with <slt:ignore>. Closes apache#24203
github-actions
Bot
added
physical-expr
getChan
left a comment
getChan
left a comment
There was a problem hiding this comment.
Request changes: please consider fixing the metric registrations instead of changing the formatting of all generic Count/Gauge values based on MetricCategory::Bytes.
As discussed in issue #24203 (#24203 (comment)), bytes_scanned appears to be using the wrong metric type and should be represented by a dedicated byte-oriented metric. The same consideration likely applies to bytes_written and stream_memory_usage.
Changing Display for Metric to reinterpret generic Count/Gauge values based on the category broadens the behavior of operator-defined metrics and makes the metric type/category relationship less explicit. Could we instead add or use dedicated byte-oriented MetricValue/builder variants for these metrics, while leaving generic Count/Gauge formatting unchanged?
…cated byte-typed metrics
Address review feedback: instead of reinterpreting a Bytes-category
generic Count/Gauge at Display time, register the three affected
metrics as their own dedicated MetricValue variants, matching the
existing OutputBytes/SpilledBytes/PeakMemoryUsage precedent.
Adds two new MetricValue variants:
- BytesCount { name, count }: a named, byte-formatted Count, for
bytes_scanned and bytes_written.
- BytesGauge { name, gauge }: a named, byte-formatted Gauge, for
stream_memory_usage.
BytesGauge is deliberately not folded into the existing
PeakMemoryUsage variant even though they're structurally identical.
PeakMemoryUsage's existing 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 old rows - it is
not a peak, and mislabeling it as one would be misleading to anyone
reading EXPLAIN output or the enum variant itself.
Also updates the two places that previously matched on MetricValue
exhaustively and would otherwise have silently mis-handled the new
variants:
- MetricsSet::sum_by_name(), used by bytes_scanned's own tests via
sum_by_name("bytes_scanned").
- display.rs's JSON EXPLAIN metric_value_to_json(): without an
explicit arm, the new variants would have fallen through to the
string-fallback branch, turning bytes_scanned's JSON output from a
number into a string like "3.0 GB" - a real behavior change for any
JSON EXPLAIN consumer.
- The FFI crate's FFI_MetricValue, which documents that new variants
must be appended at the end since variant order is part of its
stable ABI - done here, with matching conversions and round-trip
test coverage in both directions.
Display for Metric goes back to the plain `write!(f, "{}", self.value)`
it had before the previous commit - MetricValue no longer needs help
from Metric's category to know it's holding bytes.
Rendered EXPLAIN ANALYZE output is byte-for-byte unchanged (reran the
full 502-file sqllogictest suite to confirm), so none of the
previously-updated .slt fixtures needed further changes.
|
Good call, thanks for the pointer to #24203's discussion - pushed a rework.
This follows the exact shape One design note: I didn't fold Also had to update two other places that matched on
Reran the full sqllogictest suite - the rendered |
github-actions
Bot
added
datasource
|
hi @DevShiba |
…metric-display # Conflicts: # datafusion/sqllogictest/test_files/parquet_nested_schema_pruning.slt
|
@kosiew Done - merged upstream/main in and resolved the one real conflict in Also picked up the 3 CI failures that showed up on the first run ( Re-verified after merging: full |
datafusion/core/src/datasource/file_format/parquet.rs's own assert_bytes_scanned test helper pattern-matched MetricValue::Count directly (bypassing MetricsSet::sum_by_name, which was already updated for the new BytesCount variant), so it stopped finding the bytes_scanned metric once its registration moved to MetricValue::BytesCount, failing capture_bytes_scanned_metric in cargo test (amd64), cargo test hash collisions, and cargo test extended_tests - all three were the same helper, just exercised under different feature-flag runs.
Two more test helpers pattern-matched MetricValue::Count directly
instead of going through the already-fixed MetricsSet::sum_by_name,
each in a different crate:
- datafusion/core/tests/parquet/page_pruning.rs's cast_count_metric,
which caused without_pushdown_filter to panic on .unwrap() (the
actual CI failure).
- datafusion/datasource-parquet/src/opener/mod.rs's
counter_metric_value, currently only exercised with a plain Count
metric so it wasn't failing, but would have silently returned 0
instead of the real value the moment it's used for bytes_scanned.
Did a full repo-wide sweep this time, not just the one failing site:
grepped every MetricValue::Count and MetricValue::Gauge match plus
every "bytes_scanned"/"bytes_written"/"stream_memory_usage" string
reference, and manually vetted each one against the new BytesCount/
BytesGauge variants. These two were the only remaining gaps.
Before pushing, ran the exact three commands the failing CI jobs use
(from .github/workflows/rust.yml and extended.yml) locally, twice:
cargo test --profile ci --exclude datafusion-examples \
--exclude ffi_example_table_provider --exclude datafusion-cli \
--workspace --lib --tests --bins \
--features serde,avro,json,backtrace,integration-tests,parquet_encryption,substrait
(cd datafusion && cargo test --profile ci \
--exclude datafusion-examples --exclude datafusion-benchmarks \
--exclude datafusion-sqllogictest --exclude datafusion-cli \
--workspace --lib --tests --features=force_hash_collisions,avro)
cargo test --profile ci --exclude datafusion-examples \
--exclude datafusion-benchmarks --exclude datafusion-cli \
--workspace --lib --tests --bins \
--features avro,json,backtrace,extended_tests,recursive_protection,parquet_encryption
All three: zero failures. Also ran the exact CI clippy script
(ci/scripts/rust_clippy.sh, -D warnings) and cargo fmt --check clean,
and confirmed the working tree has no stray files after the test runs
(the same check CI itself runs).
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24218 +/- ##
=======================================
Coverage 81.61% 81.61%
=======================================
Files 1124 1124
Lines 412077 412160 +83
Branches 412077 412160 +83
=======================================
+ Hits 336318 336392 +74
- Misses 55949 55952 +3
- Partials 19810 19816 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kosiew
left a comment
kosiew
left a comment
There was a problem hiding this comment.
Thanks for working on this. The byte-specific metric variants look like the right direction, and the changes cover the main metric formatting paths well.
I found one FFI coverage gap that I think we should address before merging. Since the new variants extend the stable ABI representation, it would be good to verify them through a real cross-library round trip, not only the in-process conversion tests.
I also left one small non-blocking suggestion to cover the global byte counter builder used by ParquetSink.
| name: SString, | ||
| gauge: u64, | ||
| }, | ||
| BytesCount { |
There was a problem hiding this comment.
BytesCount and BytesGauge extend FFI_MetricValue, whose layout is documented as a stable ABI. The existing conversion tests and mock-foreign ExecutionPlan::metrics() round trip are useful, but I think we should also cover this through a real cross-library test.
Could you add a cdylib integration test that gets an ExecutionPlan containing both byte metric variants from the test library, then calls metrics() and verifies the variant, metric name, and value?
The existing ffi_execution_plan integration tests do not currently call metrics(), so they would not catch an ABI layout or transport issue specifically affecting these new variants.
| // human_readable_size's >= 2x-tier threshold for GB (below that it | ||
| // falls back to a large MB value). | ||
| let three_gib = 3 * 1024 * 1024 * 1024; | ||
| let bytes_scanned = |
There was a problem hiding this comment.
Small suggestion: could we add global_bytes_counter("bytes_written") to this display regression test as well? ParquetSink uses the global builder, while the current coverage exercises the partitioned bytes_counter path. It would be nice to have both builders covered here.
Addresses kosiew's review on PR apache#24218: - Add a real cdylib integration test (test_ffi_execution_plan_byte_metrics_cross_library) that registers genuine BytesCount/BytesGauge metrics (via the same MetricBuilder::bytes_counter/bytes_gauge production code uses) on a plan served from a separately loaded copy of the datafusion-ffi cdylib, then calls .metrics() through the real ForeignExecutionPlan vtable to confirm both variants survive an actual FFI_MetricValue round trip across the dylib boundary - not just the in-process From conversions already covered by physical_expr::metrics's roundtrip tests. - New create_exec_with_byte_metrics factory function and ForeignLibraryModule entry, following the existing create_exec_with_statistics pattern. - Also cover global_bytes_counter("bytes_written") in test_bytes_counter_and_gauge_use_byte_units, since ParquetSink uses the global (non-partitioned) builder while the existing coverage only exercised the partitioned bytes_counter path. Verified: the exact three commands the previously-failing CI jobs run (cargo test (amd64), hash collisions, extended_tests), the exact CI clippy script, cargo fmt --check, and the full datafusion-ffi integration-tests suite - all clean.
|
@kosiew Thanks for the detailed review — pushed both. Cross-library FFI round trip: added
Verified locally before pushing: the exact commands the three previously-failing CI jobs run, the exact |
kosiew
left a comment
kosiew
left a comment
There was a problem hiding this comment.
@DevShiba, thanks for working on this. The byte metric round-trip coverage is heading in the right direction, and the global bytes_written builder display case looks addressed.
I found two things that still need changes before this is ready. The new field on ForeignLibraryModule changes a public #[repr(C)] layout and matches the SemVer failure reported by the bot. Also, the cdylib test currently checks only rendered strings, so it does not verify that the metric variants themselves survive the ABI round trip.
I left inline comments with more detail.
|
|
||
| pub create_exec_with_statistics: extern "C" fn() -> FFI_ExecutionPlan, | ||
|
|
||
| pub create_exec_with_byte_metrics: extern "C" fn() -> FFI_ExecutionPlan, |
There was a problem hiding this comment.
ForeignLibraryModule is public and #[repr(C)], so adding a field here shifts every field that follows it. A consumer compiled against the previous layout could then call the wrong factory function pointers. This also appears to be exactly what the constructible_struct_adds_field SemVer check is reporting.
Even though this is test support, it is exposed through the public integration-tests feature and is the cross-library ABI surface these tests rely on. Could we avoid extending this struct in the current release API? A separate exported symbol or loading path for this test factory would keep the existing layout intact.
| // that boundary - not just the in-process From conversions covered | ||
| // by physical_expr::metrics's roundtrip tests. | ||
| let metrics = plan.metrics().expect("plan should report metrics"); | ||
| let rendered: Vec<String> = metrics.iter().map(|m| m.to_string()).collect(); |
There was a problem hiding this comment.
Could we verify the metric variants explicitly here as well? Comparing the rendered strings confirms the display output, but it does not prove that the BytesCount and BytesGauge discriminants survived the ABI round trip.
I think this test should match metric.value() against MetricValue::BytesCount { name, count } and MetricValue::BytesGauge { name, gauge }, then assert both the names and numeric values. Keeping the display assertions too would still be useful.
Two follow-ups from review on PR apache#24218: 1. The previous commit added create_exec_with_byte_metrics as a new field on ForeignLibraryModule. That struct is public, #[repr(C)], and has no private/gated constructor, so every field is part of its exhaustive-construction ABI surface even though the struct only exists behind the integration-tests feature - exactly what cargo-semver-checks's constructible_struct_adds_field lint flagged. Reworked the new test factory as its own top-level exported symbol (datafusion_ffi_test_create_exec_with_byte_metrics, loaded via a new get_byte_metrics_exec() in tests/utils.rs, mirroring how get_module() already loads datafusion_ffi_get_module) instead of extending the struct. ForeignLibraryModule's fields are now byte-for-byte identical to the pre-fix commit. 2. test_ffi_execution_plan_byte_metrics_cross_library previously only compared the Display-rendered strings, which doesn't prove the BytesCount/BytesGauge discriminants themselves - as opposed to some other variant that happens to render the same text - survived the FFI round trip. Now matches metric.value() against MetricValue::BytesCount{name, count}/BytesGauge{name, gauge} explicitly, asserting both the name and the numeric value, in addition to (not instead of) the existing Display assertions. Verified: full datafusion-ffi --features integration-tests suite, the exact three commands the CI jobs run (cargo test (amd64), hash collisions, extended_tests), the exact CI clippy script, and cargo fmt --check - all clean. Confirmed via git diff against the pre-fix commit that ForeignLibraryModule has zero field additions/removals.
|
@kosiew Good catches on both — pushed fixes. SemVer/ABI: reworked Discriminant coverage: the test now Verified locally before pushing: full |
|
@DevShiba |
…metric-display # Conflicts: # datafusion/sqllogictest/test_files/dynamic_filter_pushdown_config.slt # datafusion/sqllogictest/test_files/parquet_nested_schema_pruning.slt
…rge commit The previous merge commit still had the literal PLACEHOLDER text in place of the two empirically-confirmed values (bytes_processed=1147.0 B and bytes_scanned=75.0 B) - staged before running sed to fill them in, so the merge commit captured the stale index instead of the corrected working tree. Caught by re-running git status/diff against HEAD before pushing, not by CI.
|
@kosiew Done — resolved. Also found something worth flagging: Verified after merging (twice, since I initially staged the sqllogictest fixture fix before running the sed that filled in the confirmed values, so my first merge commit was wrong until I caught it against |
kosiew
left a comment
kosiew
left a comment
There was a problem hiding this comment.
Thanks for working through the earlier feedback. The cross-library FFI coverage, explicit variant assertions, global_bytes_counter("bytes_written") coverage, and ForeignLibraryModule ABI layout concerns all look addressed now.
There is still one blocking issue around SemVer compatibility. MetricValue is a public exhaustive enum, so adding BytesCount and BytesGauge is a breaking change for downstream users that exhaustively match on it. cargo-semver-checks reports this as enum_variant_added.
Since 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, I think we need to avoid changing this public enum in the current release line.
A compatible approach would be to keep the existing MetricValue::Count and MetricValue::Gauge variants, keep MetricCategory::Bytes, and restore the category-aware display handling so byte metrics render through human_readable_size. The cdylib test can then assert the transported Bytes category, the generic variant/name/value, and the byte-formatted display output.
Requesting changes for that remaining issue. Thanks!
| /// `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 { |
There was a problem hiding this 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.
…splay MetricValue is a public, already-released exhaustive enum (55.0.0), so adding BytesCount/BytesGauge as new variants is a SemVer-breaking change (cargo-semver-checks: enum_variant_added) for any downstream crate that exhaustively matches on it. Reverts to the original design: bytes_counter/bytes_gauge/ global_bytes_counter still tag their metric with MetricCategory::Bytes, but construct generic Count/Gauge variants instead of dedicated ones. Display for Metric is restored to check the category and render through human_readable_size when it's Bytes, falling back to the plain Display for MetricValue otherwise. FFI_MetricValue is reverted to its exact pre-existing shape, and the cross-library FFI test now asserts the transported Bytes category, the generic variant/name/value, and the byte-formatted display output separately instead of matching on a dedicated variant discriminant.
|
@kosiew Done — reverted the
Verified: |
github-actions
Bot
removed
core
kosiew
left a comment
kosiew
left a comment
There was a problem hiding this comment.
Thanks for the follow-up. I reviewed the latest changes and all of my previous comments have been addressed.
The SemVer and ABI concern looks resolved. MetricValue and FFI_MetricValue keep their released enum shapes, and ForeignLibraryModule remains layout-compatible.
I also like the additional cross-library coverage. The cdylib test now checks the metric category, generic variant, name, value, and byte-formatted display after metrics() crosses the library boundary. The global bytes_written builder path is covered as well.
I did not find any new issues in the follow-up changes.
Validation reviewed:
cargo test -p datafusion-ffi --features integration-tests ffi_execution_plan_byte_metrics_cross_librarycargo test -p datafusion-physical-expr-common test_bytes_counter_and_gauge_use_byte_units
Thanks again for working through the feedback. This looks good to me.
…metric-display # Conflicts: # datafusion/physical-expr-common/src/metrics/mod.rs
4 participants
Which issue does this PR close?
Rationale for this change
bytes_scannedis actually 1.26 GB, but reads as "1.26 billion" because it's formatted withhuman_readable_countinstead ofhuman_readable_size.Root cause:
MetricValue::Count/Gaugeare 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),MetricValueitself does not carry the metric's category - only the wrappingMetricstruct does. SoDisplayfor the genericCount/Gaugearms has no way to know it's holding a byte measurement and always falls back tohuman_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(aGauge) andbytes_written(aCount), 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 ofDisplay for MetricValue, which doesn't. A genericCount/GaugetaggedBytesnow useshuman_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_countwas already correct for them.Updated the 14 hardcoded
bytes_scannedexpected values across two sqllogictest files to match the corrected format. Did this by hand rather than via--complete, since--completealso 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 theCountandGaugecases, plus confirming aRows-category generic counter is untouched. Ran the full sqllogictest suite (502/502 files) to confirm no other fixture was missed, andcargo check --workspace --all-targets.Are there any user-facing changes?
Yes:
EXPLAIN ANALYZEoutput forbytes_scanned,stream_memory_usage, andbytes_writtennow shows correct byte units (e.g.1.26 GB) instead of count units (e.g.1.26 B, misleadingly meaning "1.26 billion").