Skip to content

fix(ffi): route partition statistics through statistics_with_args#2

Open
alamb wants to merge 1 commit into
asolimando:asolimando/partition-statistics-contextfrom
alamb:fix/ffi-statistics-with-args
Open

fix(ffi): route partition statistics through statistics_with_args#2
alamb wants to merge 1 commit into
asolimando:asolimando/partition-statistics-contextfrom
alamb:fix/ffi-statistics-with-args

Conversation

@alamb

@alamb alamb commented Jun 8, 2026

Copy link
Copy Markdown

Which issue does this PR close?

Targets apache#21815 (the StatisticsContext / statistics_with_args PR). This is a follow-up fix to land on that branch before it merges.

Rationale for this change

apache#21815 deprecates ExecutionPlan::partition_statistics and migrates every in-tree operator (DataSourceExec, FilterExec, UnionExec, joins, aggregates, …) to override only the new statistics_with_args.

The FFI wrapper (partition_statistics_fn_wrapper) still called the deprecated partition_statistics. So if anyone upgraded their nodes to use the new function, the FFI bindings still would call the deprecated version

What changes are included in this PR?

  • Fix bug
  • Add test
  • Add note to upgrade guide

Are these changes tested?

Yes locally

Are there any user-facing changes?

No public API change beyond the upgrade-guide note. Fixes a silent statistics regression for FFI-exported plans.


Disclaimer: I used AI to assist in preparing this change; I have reviewed the output and it matches my intention.

@github-actions github-actions Bot added documentation Improvements or additions to documentation ffi labels Jun 8, 2026
@alamb alamb marked this pull request as ready for review June 8, 2026 20:05
fn test_ffi_execution_plan_partition_statistics_round_trip() -> Result<()> {
/// Build an `EmptyExec` carrying `statistics`, then export it across the
/// (mock) FFI boundary and return the resulting foreign plan.
#[cfg(test)]

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored out into a common fixture so I could avoid a copy/pasta version of the test that was hard to understand what was different

let with_stats =
export_empty_exec_over_ffi(&schema, Some(original_stats.clone()))?;
assert_eq!(
with_stats.partition_statistics(None)?.as_ref(),

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

old API

export_empty_exec_over_ffi(&schema, Some(original_stats.clone()))?;
assert_eq!(
with_stats
.statistics_with_args(&StatisticsArgs::new(None))?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same test, with new API

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation ffi

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant