mailmindlin opened a new pull request, #22136:
URL: https://github.com/apache/datafusion/pull/22136
## Which issue does this PR close?
- Closes #22135
## Rationale for this change
`FFI_ExecutionPlan` exposes most of the `ExecutionPlan` trait but does not
expose `metrics()`. As a result, `ForeignExecutionPlan::metrics()` falls
through to the trait default (`None`), so anything downstream of an FFI
boundary loses metrics. The most visible breakage is `EXPLAIN ANALYZE`,
which renders empty metric blocks for foreign plans; anything calling
`DisplayableExecutionPlan::with_metrics(...)` on a plan tree containing
foreign nodes is similarly affected.
This PR makes foreign plans behave the same as local plans for metric
reporting. Metrics are passed as a snapshot, and all atomic-backed
counters/gauges/timers are read into plain integer fields at marshal time.
Correct because none of the in-tree consumers (`AnalyzeExec`,
`DisplayableExecutionPlan`) poll metrics
during streaming.
## What changes are included in this PR?
- New module `datafusion/ffi/src/metrics.rs` with FFI-stable mirrors of
`MetricsSet`, `Metric`, `MetricValue` (all 16 variants), `Label`,
`MetricType`, `MetricCategory`, `PruningMetrics`, `RatioMetrics`, and
`RatioMergeStrategy`, plus bidirectional `From` conversions.
- `MetricValue::Custom { value: Arc<dyn CustomMetricValue> }` is marshalled
as `(name, Display output, as_usize())`. On the consumer side it is
reconstructed as a small `FfiCustomMetricValue` shim that preserves
`Display` and `as_usize()`. `aggregate` becomes a no-op (snapshots are
not mergeable) and `as_any` only downcasts to the shim — this is the
documented compromise.
- `FFI_ExecutionPlan` gains a new `metrics` function pointer (appended
after `repartitioned`). `ForeignExecutionPlan::metrics()` is implemented
to call through it.
- Two trivial accessors added to `RatioMetrics`: `merge_strategy()` and
`display_raw_values()` — needed to marshal these otherwise-private fields.
- `chrono` added as a direct dependency of `datafusion-ffi` (used for
`Timestamp` ↔ unix-nanos conversion).
## Are these changes tested?
Yes. New tests, all passing:
- 7 unit tests in `datafusion/ffi/src/metrics.rs` round-trip every
`MetricValue` variant individually, plus a full `Metric`
(value + labels + partition + type + category) and a `MetricsSet`.
- `test_ffi_execution_plan_metrics_round_trip` in
`datafusion/ffi/src/execution_plan.rs` exercises the full FFI path:
builds an `ExecutionPlan` with a `MetricsSet`, wraps it in
`FFI_ExecutionPlan`, retrieves metrics via
`ForeignExecutionPlan::metrics()`
through `mock_foreign_marker_id`, and asserts the aggregated value matches.
- `EmptyExec` test helper extended with `with_metrics(MetricsSet)`.
Existing test suites still pass: `cargo test -p datafusion-ffi
--all-features` and `cargo test -p datafusion-ffi --features
integration-tests`.
## Are there any user-facing changes?
Yes — this PR adds public API and makes a binary-incompatible change to
`FFI_ExecutionPlan`. Please add the `api change` label.
- **New public types** in `datafusion_ffi::metrics`: `FFI_MetricsSet`,
`FFI_Metric`, `FFI_MetricValue`, `FFI_Label`, `FFI_MetricType`,
`FFI_MetricCategory`, `FFI_PruningMetrics`, `FFI_RatioMetrics`,
`FFI_RatioMergeStrategy`, and `FfiCustomMetricValue`.
- **ABI break for `FFI_ExecutionPlan`**: a new `metrics` function pointer
field is appended. Producers and consumers must be rebuilt together, as
is already enforced by the major-version check via
`datafusion_ffi::version()`.
- **New public accessors** on `RatioMetrics`: `merge_strategy()` and
`display_raw_values()`. Non-breaking additions.
- **`MetricValue::Custom` across FFI is lossy by design**: the underlying
`dyn CustomMetricValue` is not preserved; only its `Display` output and
`as_usize()` snapshot survive. Documented on `FfiCustomMetricValue`.
--
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]
