Skip to content

FFI: plumb placement for FFI_ScalarUDF#22608

Open
Amogh-2404 wants to merge 1 commit into
apache:mainfrom
Amogh-2404:fix/issue-22330-ffi-scalarudf-placement
Open

FFI: plumb placement for FFI_ScalarUDF#22608
Amogh-2404 wants to merge 1 commit into
apache:mainfrom
Amogh-2404:fix/issue-22330-ffi-scalarudf-placement

Conversation

@Amogh-2404
Copy link
Copy Markdown

@Amogh-2404 Amogh-2404 commented May 29, 2026

Which issue does this PR close?

This is the first of the per-method PRs that issue describes. It plumbs placement only; the remaining defaulted methods follow separately, so the umbrella issue stays open.

Rationale for this change

FFI_ScalarUDF (datafusion/ffi/src/udf/mod.rs) carried no function pointer for placement, and ForeignScalarUDF did not override it, so a producer's override of ScalarUDFImpl::placement (default body at datafusion/expr/src/udf.rs:1028) was dropped on the consumer side and every foreign UDF fell back to KeepInPlace. A UDF loaded over FFI never delivered its leaf-pushdown hint to the optimizer.

What changes are included in this PR?

  • New FFI_ExpressionPlacement enum bridge in datafusion/ffi/src/placement.rs, in the shape of FFI_Volatility: #[repr(u8)] with From impls both ways and a round-trip test over every variant.
  • A placement function pointer on FFI_ScalarUDF, populated in the From<Arc<ScalarUDF>> constructor, with placement_fn_wrapper on the producer side and a forwarding ForeignScalarUDF::placement on the consumer side. placement is infallible, so the pointer returns the enum directly rather than FFI_Result.

Adding a field to the #[repr(C)] struct changes its layout, so this is an API change and should carry the api change label (I can't add it myself). It targets main and should not be back-ported to a release branch.

display_name is also on the issue's list, but it has been deprecated since 50.0.0, so it should be dropped from the gap list rather than plumbed. I have left it and the remaining methods to follow-up PRs.

Are these changes tested?

Yes.

  • Unit: a round-trip test over all four ExpressionPlacement variants, plus a forced-foreign test (mock_foreign_marker_id) using a UDF whose placement override depends on its arguments. The assertions cover ordered, reordered, and empty argument slices, so argument marshalling is checked, not just the return value.
  • Integration: tests/ffi_udf.rs loads the UDF from the real cdylib and asserts the override survives the boundary, which is the surface a layout change needs.

Run with cargo test -p datafusion-ffi and cargo test -p datafusion-ffi --features integration-tests.

Are there any user-facing changes?

A placement override on a ScalarUDFImpl is now preserved across the FFI boundary instead of being silently replaced by the default. This is an ABI change to FFI_ScalarUDF; consumers must be recompiled against the new layout.

`FFI_ScalarUDF` carried no function pointer for `placement`, so a producer's
override of `ScalarUDFImpl::placement` was lost crossing the boundary and
every foreign UDF fell back to the default `KeepInPlace`. The optimizer never
saw the leaf-pushdown hint for a UDF loaded over FFI.

Add an `FFI_ExpressionPlacement` enum bridge in the shape of `FFI_Volatility`,
a `placement` pointer on `FFI_ScalarUDF`, the producer-side wrapper, and the
`ForeignScalarUDF` forwarding impl. The wrapper calls `placement` on the inner
`Arc`, so dispatch resolves the override (or the default) on the producer side
and only the resulting enum crosses.

Adding a field to the `#[repr(C)]` struct changes its layout, so this is an
API change.

Part of apache#22330

Signed-off-by: Amogh Ramesh <ramogh2404@gmail.com>
@github-actions github-actions Bot added the ffi Changes to the ffi crate label May 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ffi Changes to the ffi crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant