kosiew commented on code in PR #24218:
URL: https://github.com/apache/datafusion/pull/24218#discussion_r3810571245
##########
datafusion/ffi/src/physical_expr/metrics.rs:
##########
@@ -177,6 +177,14 @@ pub enum FFI_MetricValue {
name: SString,
gauge: u64,
},
+ BytesCount {
Review 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.
##########
datafusion/physical-expr-common/src/metrics/mod.rs:
##########
@@ -773,6 +777,61 @@ mod tests {
assert_eq!(metrics.sum(|_| true), Some(expected_sum));
}
+ #[test]
+ fn test_bytes_counter_and_gauge_use_byte_units() {
+ let metrics = ExecutionPlanMetricsSet::new();
+
+ // A dedicated byte counter/gauge (like `bytes_scanned` or
+ // `stream_memory_usage`) must render with human_readable_size's
+ // 1024-based units (KB/MB/GB), not human_readable_count's 1000-based
+ // units (K/M/B) - see #24203. 3 GiB, chosen to clear
+ // 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 =
Review 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.
--
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]
