Skip to content

Add optimize_with_context to FFI_PhysicalOptimizerRule#22584

Merged
kosiew merged 3 commits into
apache:mainfrom
nathanb9:ffi-optimize-with-context
Jun 5, 2026
Merged

Add optimize_with_context to FFI_PhysicalOptimizerRule#22584
kosiew merged 3 commits into
apache:mainfrom
nathanb9:ffi-optimize-with-context

Conversation

@nathanb9
Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes #22334

Rationale for this change

FFI_PhysicalOptimizerRule only plumbed optimize, name, and schema_check — not optimize_with_context. Foreign rules that override the context-aware variant had their override silently discarded.

What changes are included in this PR?

  • Added FFI_PhysicalOptimizerContext struct to pass optimizer context (config + statistics registry) across FFI
  • Added optimize_with_context function pointer to FFI_PhysicalOptimizerRule
  • ForeignPhysicalOptimizerRule now overrides optimize_with_context to route through FFI
  • Unit tests for context-aware round-trip (with and without statistics registry)

Are these changes tested?

Yes — two new tests (test_optimize_with_context_round_trip, test_optimize_with_context_with_registry) plus all existing tests continue to pass.

Are there any user-facing changes?

API change: FFI_PhysicalOptimizerRule gains a new field (optimize_with_context). This is a layout change for any external consumer of this struct.

Plumb `optimize_with_context` through the FFI boundary so that foreign
physical optimizer rules can access the PhysicalOptimizerContext
(including the statistics registry). Previously only the context-free
`optimize` was exposed, silently discarding overrides of the
context-aware variant.

Closes apache#22334
@github-actions github-actions Bot added the ffi Changes to the ffi crate label May 28, 2026
@nathanb9 nathanb9 marked this pull request as ready for review May 30, 2026 19:56
Copy link
Copy Markdown
Contributor

@kosiew kosiew left a comment

Choose a reason for hiding this comment

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

@nathanb9
Thanks for working on this.

I think this needs a few changes before merge, mainly around the new context plumbing and ABI coverage.

Comment thread datafusion/ffi/src/physical_optimizer.rs Outdated
Comment thread datafusion/ffi/src/physical_optimizer.rs Outdated
Comment thread datafusion/ffi/src/physical_optimizer.rs Outdated
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 1, 2026

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 [  62.896s] (current)
     Parsing datafusion-ffi v53.1.0 (current)
      Parsed [   0.059s] (current)
    Building datafusion-ffi v53.1.0 (baseline)
       Built [  58.721s] (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.238s] 223 checks: 221 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.48.0/src/lints/constructible_struct_adds_field.ron

Failed in:
  field FFI_PhysicalOptimizerRule.optimize_with_context in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/physical_optimizer.rs:136
  field ForeignLibraryModule.create_context_aware_optimizer_rule in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/tests/mod.rs:116

--- 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.48.0/src/lints/repr_c_plain_struct_fields_reordered.ron

Failed in:
  FFI_PhysicalOptimizerRule.private_data moved from position 7 to 8, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/physical_optimizer.rs:144
  FFI_PhysicalOptimizerRule.library_marker_id moved from position 8 to 9, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/physical_optimizer.rs:148
  ForeignLibraryModule.version moved from position 17 to 18, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/tests/mod.rs:118

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

@github-actions github-actions Bot added the auto detected api change Auto detected API change label Jun 1, 2026
@nathanb9 nathanb9 requested a review from kosiew June 1, 2026 23:38
Copy link
Copy Markdown
Contributor

@kosiew kosiew left a comment

Choose a reason for hiding this comment

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

@nathanb9
Thanks for the follow-up updates. The original review comments look resolved, and the new integration test covers the important cross-library path nicely.

I found a couple of follow-up issues that seem worth fixing before this lands, mostly around unused API surface and avoidable cloning.

Comment thread datafusion/ffi/src/physical_optimizer.rs Outdated
Comment thread datafusion/ffi/src/physical_optimizer.rs
Comment thread datafusion/ffi/src/physical_optimizer.rs Outdated

pub create_physical_optimizer_rule: extern "C" fn() -> FFI_PhysicalOptimizerRule,

pub create_context_aware_optimizer_rule: extern "C" fn() -> FFI_PhysicalOptimizerRule,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ForeignLibraryModule gained a required public field here, so existing struct literal construction would break. cargo-semver-checks flags this as constructible_struct_adds_field.

The API change label covers this, and I realize this lives under the test module for cross-library integration coverage. Just calling it out so the public API impact is explicit.

@kosiew kosiew added this pull request to the merge queue Jun 5, 2026
Merged via the queue into apache:main with commit e1d8d46 Jun 5, 2026
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto detected api change Auto detected API change ffi Changes to the ffi crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FFI: FFI_PhysicalOptimizerRule missing optimize_with_context

2 participants