Expose `ExecutionPlan` statistics across the FFI boundary by mailmindlin · Pull Request #22157 · apache/datafusion

mailmindlin · 2026-05-13T20:34:49Z

Which issue does this PR close?

Closes Expose ExecutionPlan statistics across the FFI boundary #22152

Rationale for this change

ExecutionPlan::partition_statistics and TableProvider::statistics are not currently transported across the DataFusion FFI boundary, so foreign plans and providers always report Statistics::new_unknown / None. This blocks optimizer rules that depend on statistics (e.g. join reordering, partition pruning) from working with out-of-process plugins, which defeats the point of exposing those hooks to plugin authors.

Statistics contains Precision<ScalarValue> for column min/max/sum. ScalarValue is a large enum that's impractical to mirror in #[repr(C)], so I reuse the existing datafusion_proto_common::Statistics prost encoding — the same pattern this crate already uses for filter expressions.

What changes are included in this PR?

New datafusion_ffi::statistics module with [de]serialize_statistics helpers wrapping thedatafusion_proto_common::Statistics round-trip.
New partition_statistics field on FFI_ExecutionPlan and corresponding ExecutionPlan::partition_statistics impl on ForeignExecutionPlan
New statistics field on FFI_TableProvider and corresponding TableProvider::statistics impl on ForeignTableProvider. Since the trait returns Option<Statistics>, the implementation cannot propagate decode errors, it logs a log::warn! and triggers a debug_assert!.

This PR is expected to be merged after #22136 so it includes those changes.

Are these changes tested?

Yes:

Unit tests in statistics.rs cover three round-trip cases: Statistics::new_unknown, fully-exact statistics with ScalarValue::Int32/Int64/Utf8 min/max/sum, and mixed Precision::Exact/Inexact/Absent values.
A new round-trip integration test in execution_plan.rs exercises ForeignExecutionPlan::partition_statistics with both None and Some(idx) partitions, against a plan with no statistics (returns Statistics::new_unknown) and a plan with concrete statistics.
A new round-trip integration test in table_provider.rs uses a thin TableWithStats wrapper over MemTable to verify both the None path and the concrete Statistics path through ForeignTableProvider::statistics.

Are there any user-facing changes?

This is a breaking ABI change for the datafusion-ffi crate:

FFI_ExecutionPlan gains a partition_statistics field.
FFI_TableProvider gains a statistics field.

Plugins compiled against earlier versions of datafusion-ffi will need to be recompiled. There are no breaking changes to the Rust trait surface or to Statistics itself; downstream ExecutionPlan / TableProvider implementations require no changes.

github-actions · 2026-05-13T21:36:21Z

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 [  58.694s] (current)
     Parsing datafusion-ffi v53.1.0 (current)
      Parsed [   0.058s] (current)
    Building datafusion-ffi v53.1.0 (baseline)
       Built [  58.172s] (baseline)
     Parsing datafusion-ffi v53.1.0 (baseline)
      Parsed [   0.058s] (baseline)
    Checking datafusion-ffi v53.1.0 -> v53.1.0 (no change; assume patch)
     Checked [   0.228s] 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.partition_statistics in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/execution_plan.rs:82

--- 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 8 to 9, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/execution_plan.rs:89
  FFI_ExecutionPlan.release moved from position 9 to 10, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/execution_plan.rs:92
  FFI_ExecutionPlan.private_data moved from position 10 to 11, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/execution_plan.rs:96
  FFI_ExecutionPlan.library_marker_id moved from position 11 to 12, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/execution_plan.rs:101
  FFI_TableProvider.logical_codec moved from position 6 to 7, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/table_provider.rs:145
  FFI_TableProvider.version moved from position 9 to 10, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/table_provider.rs:155
  FFI_TableProvider.library_marker_id moved from position 11 to 12, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/table_provider.rs:164

     Summary semver requires new major version: 1 major and 0 minor checks failed
     Warning produced 1 major and 0 minor level warnings
    Finished [ 120.349s] datafusion-ffi

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

Copilot

Pull request overview

This PR plumbs ExecutionPlan::partition_statistics and TableProvider::statistics across the datafusion-ffi boundary so out-of-process plugins can report real statistics to the planner instead of always returning Statistics::new_unknown/None. It reuses the existing datafusion_proto_common::Statistics prost encoding (the same approach already used for filter expressions) rather than mirroring ScalarValue in #[repr(C)].

Changes:

New datafusion_ffi::statistics module with serialize_statistics/deserialize_statistics helpers backed by datafusion-proto-common.
New partition_statistics function pointer on FFI_ExecutionPlan and corresponding ForeignExecutionPlan::partition_statistics implementation (Result-propagating).
New statistics function pointer on FFI_TableProvider and corresponding ForeignTableProvider::statistics implementation (logs + debug_assert! on decode error since the trait can only return Option).

Reviewed changes

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

File	Description
`datafusion/ffi/src/lib.rs`	Registers the new `statistics` module.
`datafusion/ffi/src/statistics.rs`	New helpers wrapping prost round-trip with three unit round-trip tests.
`datafusion/ffi/src/execution_plan.rs`	Adds `partition_statistics` FFI field, wrapper, and `ForeignExecutionPlan` impl; extends `EmptyExec` test helper with `with_statistics`; adds a round-trip integration test.
`datafusion/ffi/src/table_provider.rs`	Adds `statistics` FFI field, wrapper, and `ForeignTableProvider::statistics` impl with `log::warn!`/`debug_assert!` on decode failure; adds a `TableWithStats`-based round-trip test.

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

timsaucer

Looking good, but a few things worth addressing

timsaucer · 2026-05-14T19:31:59Z

+    /// `datafusion_proto_common::Statistics`.
+    pub partition_statistics: unsafe extern "C" fn(
+        plan: &Self,
+        partition: FFI_Option<u64>,


why FFI_Option instead of FFI_Option?

I'm not sure I understand what you mean

timsaucer · 2026-05-14T19:33:45Z

+    #[cfg(test)]
+    use datafusion_common::stats::Precision;
+    #[cfg(test)]
+    use datafusion_common::{ColumnStatistics, ScalarValue};


If this is here because you get clippy warnings otherwise, then it's marginally cleaner to remove the use statements and just fully qualify these below.

timsaucer · 2026-05-14T19:36:28Z

+) -> FFI_Result<FFI_Option<SVec<u8>>> {
+    let serialized: Option<SVec<u8>> = provider
+        .inner()
+        .statistics()
+        .map(|s| serialize_statistics(&s).into_iter().collect());
+    FFI_Result::Ok(serialized.into())


Why return a result? It looks infallible unless I missed something

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

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

mailmindlin added 9 commits May 14, 2026 09:38

Add metrics to FFI_ExecutionPlan

7b0e037

Fix off-by-one bug

c4fa489

Fix roundtrip tests

7b66b0b

Remove awkward docs

1f6dcb0

Explicit units

73a495c

Fix clippy lint

1b00bbd

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

fmt

e828d7d

move metrics.rs

0c21413

FFI statistics

a171b9e

mailmindlin force-pushed the feature/ffi-statistics branch from 97fb89e to a171b9e Compare May 14, 2026 13:40

github-actions Bot removed the physical-expr Changes to the physical-expr crates label May 14, 2026

mailmindlin mentioned this pull request May 14, 2026

Rerun PRs rerun-io/opensource#2

Open

timsaucer requested review from Copilot and timsaucer May 14, 2026 19:30

timsaucer added the api change Changes the API exposed to users of the crate label May 14, 2026

Copilot started reviewing on behalf of timsaucer May 14, 2026 19:30 View session

Copilot AI reviewed May 14, 2026

View reviewed changes

timsaucer reviewed May 14, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose `ExecutionPlan` statistics across the FFI boundary#22157

Expose `ExecutionPlan` statistics across the FFI boundary#22157
mailmindlin wants to merge 9 commits into
apache:mainfrom
mailmindlin:feature/ffi-statistics

mailmindlin commented May 13, 2026

Uh oh!

github-actions Bot commented May 13, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

timsaucer left a comment

Uh oh!

timsaucer May 14, 2026

Uh oh!

mailmindlin May 14, 2026 •

edited

Loading

Uh oh!

timsaucer May 14, 2026

Uh oh!

timsaucer May 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

mailmindlin commented May 13, 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!

timsaucer left a comment

Choose a reason for hiding this comment

Uh oh!

timsaucer May 14, 2026

Choose a reason for hiding this comment

Uh oh!

mailmindlin May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timsaucer May 14, 2026

Choose a reason for hiding this comment

Uh oh!

timsaucer May 14, 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

mailmindlin May 14, 2026 •

edited

Loading