Add metrics to `FFI_ExecutionPlan` by mailmindlin · Pull Request #22136 · apache/datafusion

mailmindlin · 2026-05-12T21:15:07Z

Which issue does this PR close?

Closes Expose ExecutionPlan::metrics() across the FFI boundary #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.

github-actions · 2026-05-13T12:19:36Z

Thank you for opening this pull request!

Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).

Details

     Cloning apache/main
    Building datafusion-ffi v53.1.0 (current)
       Built [  57.069s] (current)
     Parsing datafusion-ffi v53.1.0 (current)
      Parsed [   0.063s] (current)
    Building datafusion-ffi v53.1.0 (baseline)
       Built [  56.899s] (baseline)
     Parsing datafusion-ffi v53.1.0 (baseline)
      Parsed [   0.059s] (baseline)
    Checking datafusion-ffi v53.1.0 -> v53.1.0 (no change; assume patch)
     Checked [   0.320s] 222 checks: 220 pass, 1 fail, 1 warn, 30 skip

--- failure constructible_struct_adds_field: externally-constructible struct adds field ---

Description:
A pub struct constructible with a struct literal has a new pub field. Existing struct literals must be updated to include the new field.
        ref: https://doc.rust-lang.org/reference/expressions/struct-expr.html
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.47.0/src/lints/constructible_struct_adds_field.ron

Failed in:
  field FFI_ExecutionPlan.metrics in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/execution_plan.rs:75

--- warning repr_c_plain_struct_fields_reordered: struct fields reordered in repr(C) struct ---

Description:
A public repr(C) struct had its fields reordered. This can change the struct's memory layout, possibly breaking FFI use cases that depend on field position and order.
        ref: https://doc.rust-lang.org/reference/type-layout.html#reprc-structs
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.47.0/src/lints/repr_c_plain_struct_fields_reordered.ron

Failed in:
  FFI_ExecutionPlan.clone moved from position 7 to 8, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/execution_plan.rs:79
  FFI_ExecutionPlan.release moved from position 8 to 9, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/execution_plan.rs:82
  FFI_ExecutionPlan.private_data moved from position 9 to 10, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/execution_plan.rs:86
  FFI_ExecutionPlan.library_marker_id moved from position 10 to 11, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/execution_plan.rs:91

     Summary semver requires new major version: 1 major and 0 minor checks failed
     Warning produced 1 major and 0 minor level warnings
    Finished [ 116.386s] datafusion-ffi
    Building datafusion-physical-expr-common v53.1.0 (current)
       Built [  19.399s] (current)
     Parsing datafusion-physical-expr-common v53.1.0 (current)
      Parsed [   0.020s] (current)
    Building datafusion-physical-expr-common v53.1.0 (baseline)
       Built [  19.783s] (baseline)
     Parsing datafusion-physical-expr-common v53.1.0 (baseline)
      Parsed [   0.022s] (baseline)
    Checking datafusion-physical-expr-common v53.1.0 -> v53.1.0 (no change; assume patch)
     Checked [   0.258s] 222 checks: 222 pass, 30 skip
     Summary no semver update required
    Finished [  40.483s] datafusion-physical-expr-common

Copilot

Pull request overview

This PR extends datafusion-ffi so ExecutionPlan::metrics() can be accessed across the FFI boundary, preserving execution metrics for foreign plans (e.g., for EXPLAIN ANALYZE and DisplayableExecutionPlan::with_metrics(...)) by marshalling metrics as a snapshot.

Changes:

Add FFI-stable mirrors of MetricsSet, Metric, and all MetricValue variants (plus related metric types) with bidirectional conversions.
Add a metrics function pointer to FFI_ExecutionPlan and implement ForeignExecutionPlan::metrics() via that callback.
Expose RatioMetrics internals needed for marshalling via new accessors; add chrono as a direct datafusion-ffi dependency for timestamp conversion.

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
datafusion/physical-expr-common/src/metrics/value.rs	Adds `RatioMetrics` accessors needed to snapshot/serialize merge strategy and display options.
datafusion/ffi/src/metrics.rs	Introduces FFI-stable metric types and snapshot conversions, plus round-trip tests.
datafusion/ffi/src/lib.rs	Exposes the new `metrics` module publicly.
datafusion/ffi/src/execution_plan.rs	Adds `FFI_ExecutionPlan.metrics` callback and wires it into `ForeignExecutionPlan::metrics()`; adds an integration-style test.
datafusion/ffi/Cargo.toml	Adds direct `chrono` dependency for timestamp marshalling.
Cargo.lock	Records the new `chrono` dependency for `datafusion-ffi`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

mailmindlin · 2026-05-13T17:24:50Z

+    /// Snapshot the plan's execution metrics. Returns `None` when the
+    /// underlying [`ExecutionPlan::metrics`] returned `None`.
+    pub metrics: unsafe extern "C" fn(plan: &Self) -> FFI_Option<FFI_MetricsSet>,
+


This is expected.

timsaucer · 2026-05-13T16:40:18Z

+fn timestamp_from_ffi(nanos: FFI_Option<i64>) -> Timestamp {
+    let ts = Timestamp::new();
+    if let Some(n) = nanos.into_option() {
+        ts.set(DateTime::<Utc>::from_timestamp_nanos(n));
+    }
+    ts
+}


Will we lose something if we round trip and the original is not UTC?

That shouldn't be possible. Timestamp is composed of Arc<Mutex<Option<DateTime<Utc>>>>, and is documented as only storing UTC-nanoseconds.

Not sure why it shows up on the FFI enum but not the original

Add metrics to FFI_ExecutionPlan

5652d4d

github-actions Bot added physical-expr Changes to the physical-expr crates ffi Changes to the ffi crate labels May 12, 2026

timsaucer self-requested a review May 13, 2026 12:11

github-actions Bot added the auto detected api change Auto detected API change label May 13, 2026

timsaucer requested a review from Copilot May 13, 2026 12:23

Copilot started reviewing on behalf of timsaucer May 13, 2026 12:24 View session

Copilot AI reviewed May 13, 2026

View reviewed changes

timsaucer added api change Changes the API exposed to users of the crate labels May 13, 2026

mailmindlin added 2 commits May 13, 2026 12:37

Fix off-by-one bug

3f39376

Fix roundtrip tests

454cae9

timsaucer reviewed May 13, 2026

View reviewed changes

mailmindlin added 6 commits May 13, 2026 13:10

Remove awkward docs

a8e3f42

Explicit units

75269d8

Fix clippy lint

70f3ab3

Not sure why it shows up on the FFI enum but not the original

fmt

56e85e3

move metrics.rs

c2589d6

Merge branch 'main' into feature/ffi-executionplan-metrics

32f472d

mailmindlin mentioned this pull request May 13, 2026

Expose ExecutionPlan statistics across the FFI boundary #22152

Open

cargo fmt

5931124

mailmindlin mentioned this pull request May 13, 2026

Expose ExecutionPlan statistics across the FFI boundary #22157

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add metrics to `FFI_ExecutionPlan`#22136

Add metrics to `FFI_ExecutionPlan`#22136
mailmindlin wants to merge 10 commits into
apache:mainfrom
mailmindlin:feature/ffi-executionplan-metrics

mailmindlin commented May 12, 2026

Uh oh!

github-actions Bot commented May 13, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mailmindlin May 13, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

timsaucer May 13, 2026

Uh oh!

mailmindlin May 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

mailmindlin commented May 12, 2026

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions Bot commented May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mailmindlin May 13, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

timsaucer May 13, 2026

Choose a reason for hiding this comment

Uh oh!

mailmindlin May 13, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions Bot commented May 13, 2026 •

edited

Loading