Implement FFI_PhysicalExpr and the structs it needs to support it. #18916

timsaucer · 2025-11-24T20:36:24Z

Which issue does this PR close?

Addresses part of #18671 but does not close it.

Rationale for this change

This PR exposes the PhysicalExpr trait via FFI. This will allow us to remove using protobuf for transferring physical expressions across the FFI boundary. We will still use protobuf for the logical side.

The reason this is important is because it will allow us to eventually remove the core crate as described in #18671 but also it will enable keeping ColumnarValue::Scalar when using UDFs. This is important for UDF performance.

Of all of the PRs I have prepared for #18671 this is the largest of the individual PRs. That is because it requires quite a few supporting structures from datafusion-expr in order to support it.

What changes are included in this PR?

This PR introduces the FFI_PhysicalExpr trait and a variety of enums and structs that are needed to be FFI stable in order to implement it. It does not replace the existing usage in the UDFs and other places with the FFI_PhysicalExpr yet. That comes in a later PR in order to keep the size of the individual requests to manageable.

Are these changes tested?

Unit tests are included.

Are there any user-facing changes?

Since this is pure addition, no user facing changes in this PR.

timsaucer · 2025-11-24T20:56:09Z

datafusion/ffi/Cargo.toml

+datafusion-expr = { workspace = true }
 datafusion-functions-aggregate-common = { workspace = true }
+datafusion-physical-expr = { workspace = true }
+datafusion-physical-expr-common = { workspace = true }


In this and the following PRs I am introducing more of these crates, even when they are re-exported in datafusion core crate so that it will have a smaller PR when we remove the core crate at the end of this work epic.

timsaucer · 2025-11-25T19:42:22Z

@renato2099 @comphead @paleolimbot @kevinjqliu Would any of you be available to review this PR? I know it is large - I think it's the biggest single PR of the entire FFI update effort. I am particularly interested if anyone disagrees that this is a correct approach to take - implementing the PhysicalExpr trait because it does require so many other enums to copy their internal counterparts.

I appreciate any time and thoughts you can spare.

Copilot

Pull request overview

This PR introduces FFI (Foreign Function Interface) support for PhysicalExpr and its supporting structures, enabling physical expressions to be safely passed across FFI boundaries. This is a foundational change that will eventually allow removal of protobuf usage for physical expressions and improve UDF performance by preserving ColumnarValue::Scalar.

Key changes:

Implementation of FFI_PhysicalExpr trait with comprehensive wrapper functions for all PhysicalExpr methods
FFI-stable structures for intervals, distributions, expression properties, columnar values, partitioning, and sort expressions
Bidirectional conversion support between native DataFusion types and FFI-safe representations

Reviewed changes

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

Show a summary per file

File	Description
datafusion/ffi/src/util.rs	Added `FFIResult` type alias for FFI return types
datafusion/ffi/src/record_batch_stream.rs	Made conversion utilities public for use in physical expression FFI
datafusion/ffi/src/physical_expr/sort.rs	Implemented FFI wrapper for physical sort expressions
datafusion/ffi/src/physical_expr/partitioning.rs	Implemented FFI wrapper for partitioning schemes
datafusion/ffi/src/physical_expr/mod.rs	Core implementation of `FFI_PhysicalExpr` with wrapper functions and conversions
datafusion/ffi/src/lib.rs	Exported new `expr` and `physical_expr` modules
datafusion/ffi/src/expr/util.rs	Utility functions for scalar value serialization via protobuf
datafusion/ffi/src/expr/mod.rs	Module organization for expression-related FFI types
datafusion/ffi/src/expr/interval.rs	FFI wrapper for interval arithmetic
datafusion/ffi/src/expr/expr_properties.rs	FFI wrappers for expression properties and sort options
datafusion/ffi/src/expr/distribution.rs	FFI wrappers for all distribution types
datafusion/ffi/src/expr/columnar_value.rs	FFI wrapper for columnar values (arrays and scalars)
datafusion/ffi/Cargo.toml	Added dependencies for expression and physical expression crates

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

Copilot · 2025-11-25T19:43:33Z

datafusion/ffi/src/physical_expr/mod.rs

+
+unsafe extern "C" fn hash_fn_wrapper(expr: &FFI_PhysicalExpr) -> u64 {
+    let expr = expr.inner();
+    // let mut hasher = DefaultHasher::new();


Duplicate code with commented-out line. Remove the commented line 375 as it serves no purpose.

Suggested change

// let mut hasher = DefaultHasher::new();

Copilot · 2025-11-25T19:43:34Z

datafusion/ffi/src/physical_expr/mod.rs

+    fn from(expr: &FFI_PhysicalExpr) -> Self {
+        if (expr.library_marker_id)() == crate::get_library_marker_id() {
+            Arc::clone(expr.inner())
+        } else {
+            let children = unsafe {
+                (expr.children)(expr)
+                    .into_iter()
+                    .map(|expr| <Arc<dyn PhysicalExpr>>::from(&expr))
+                    .collect()
+            };
+
+            Arc::new(ForeignPhysicalExpr {
+                expr: expr.clone(),


[nitpick] The variable name expr is used both for the parameter and the field, which is confusing. Consider renaming to expr: unsafe { (expr.clone)(expr) } or use ffi_expr to clarify that we're cloning the FFI structure.

Suggested change

fn from(expr: &FFI_PhysicalExpr) -> Self {

if (expr.library_marker_id)() == crate::get_library_marker_id() {

Arc::clone(expr.inner())

} else {

let children = unsafe {

(expr.children)(expr)

.into_iter()

.map(|expr| <Arc<dyn PhysicalExpr>>::from(&expr))

.collect()

};

Arc::new(ForeignPhysicalExpr {

expr: expr.clone(),

fn from(ffi_expr: &FFI_PhysicalExpr) -> Self {

if (ffi_expr.library_marker_id)() == crate::get_library_marker_id() {

Arc::clone(ffi_expr.inner())

} else {

let children = unsafe {

(ffi_expr.children)(ffi_expr)

.into_iter()

.map(|expr| <Arc<dyn PhysicalExpr>>::from(&expr))

.collect()

};

Arc::new(ForeignPhysicalExpr {

expr: ffi_expr.clone(),

Copilot · 2025-11-25T19:43:34Z

datafusion/ffi/src/physical_expr/mod.rs

+            let batch = df_result!(record_batch_to_wrapped_array(batch.clone()))?;
+            // This is not ideal - we are cloning the selection array
+            // This is not terrible since it will be a small array.
+            // The other alternative is to modify the trait signature.


This comment acknowledges a performance concern but doesn't explain why modifying the trait signature isn't viable. Consider adding context about the constraints that prevent trait signature modification.

Suggested change

// The other alternative is to modify the trait signature.

// The other alternative is to modify the trait signature.

// However, modifying the trait signature is not viable here because

// the PhysicalExpr trait is part of a stable public API used across

// multiple crates and FFI boundaries. Changing its signature would

// break backward compatibility and require coordinated updates in

// downstream consumers, which is undesirable for maintaining API stability.

Copilot · 2025-11-25T19:43:35Z

datafusion/ffi/src/physical_expr/mod.rs

+impl Eq for ForeignPhysicalExpr {}
+impl PartialEq for ForeignPhysicalExpr {
+    fn eq(&self, other: &Self) -> bool {
+        // FFI_PhysicalExpr cannot be compared, so identity equality is the best we can do.


The comment states "FFI_PhysicalExpr cannot be compared" but doesn't explain why. Consider clarifying that this is due to opaque private_data across FFI boundaries making semantic comparison impossible.

Suggested change

// FFI_PhysicalExpr cannot be compared, so identity equality is the best we can do.

// FFI_PhysicalExpr cannot be compared semantically because its private_data is opaque

// across FFI boundaries, making it impossible to determine equivalence of underlying state.

// Therefore, identity equality is the best we can do.

comphead · 2025-11-25T20:27:26Z

I appreciate any time and thoughts you can spare.

Thanks @timsaucer for the PR I'm planning to look it at it today or tomorrow!

paleolimbot

Awesome!

All my comments here likely lack some context, so any comments here are more like questions 🙂 .

The only high-level question I have is whether passing filters across an FFI boundary in this way are going to be useful everywhere. For example, our filtering makes extensive use of downcasting to prune row groups out of GeoParquet files and push bounding boxes into spatial indexes: https://github.com/apache/sedona-db/blob/main/rust/sedona-expr/src/spatial_filter.rs#L176-L211 . (That shouldn't block anything here...we may be totally unique and are fully capable of inventing our own FFI if we need to!)

I also wonder if mismatched struct definitions should be considered at some level here (or likely a battle for a different day given there's no precedent for that yet!)

paleolimbot · 2025-11-25T22:05:19Z

datafusion/ffi/src/expr/util.rs

+pub fn scalar_value_to_rvec_u8(value: &ScalarValue) -> Result<RVec<u8>> {
+    let value: datafusion_proto_common::ScalarValue = value.try_into()?;
+    Ok(value.encode_to_vec().into())
+}


Can these be passed as FFI_ArrowArray of length one? Serializing probably has non-trivial performance implications for things like ScalarValue::List that might otherwise be able to avoid a copy of its values. I had been planning to use this trick to pass around references to imagery.

paleolimbot · 2025-11-25T22:07:25Z

datafusion/ffi/src/expr/columnar_value.rs

+            ColumnarValue::Scalar(v) => {
+                FFI_ColumnarValue::Scalar(scalar_value_to_rvec_u8(&v)?)
+            }


I left a note in a later section as well, but I think an FFI_ArrowArray of length one is a more flexible choice for passing a scalar across the FFI boundary.

paleolimbot · 2025-11-25T22:14:38Z

datafusion/ffi/src/expr/distribution.rs

+    fn try_from(value: &FFI_Distribution) -> Result<Self, Self::Error> {
+        match value {
+            FFI_Distribution::Uniform(d) => d.try_into(),


It's unclear to me what would happen on version mismatch at this point. Does derive(StableAbi) have a mechanism for loading an FFI enum with an unrecognized variant? I feel as though _ => { / * error communicating that there is a version mismatch */ } would be a nice failure route (can/should the FFI enums be non-exhaustive?)

paleolimbot · 2025-11-25T22:22:04Z

datafusion/ffi/src/physical_expr/partitioning.rs

+    /// Allocate batches using a round-robin algorithm and the specified number of partitions
+    RoundRobinBatch(usize),
+
+    /// Allocate rows based on a hash of one of more expressions and the specified number of
+    /// partitions
+    Hash(RVec<FFI_PhysicalExpr>, usize),
+
+    /// Unknown partitioning scheme with a known number of partitions
+    UnknownPartitioning(usize),


Is there a reason these are documented and not the other FFI enum items?

paleolimbot · 2025-11-25T22:33:43Z

datafusion/ffi/src/physical_expr/mod.rs

+unsafe extern "C" fn release_fn_wrapper(expr: &mut FFI_PhysicalExpr) {
+    let private_data = Box::from_raw(expr.private_data as *mut PhysicalExprPrivateData);
+    drop(private_data);
+}


It may be a good idea to set expr.private_data to null_mut() after dropping (and perhaps debug_assert!() that the private data is non-null before creating the box).

paleolimbot · 2025-11-25T22:35:00Z

datafusion/ffi/src/physical_expr/mod.rs

+/// This wrapper struct exists on the receiver side of the FFI interface, so it has
+/// no guarantees about being able to access the data in `private_data`. Any functions
+/// defined on this struct must only use the stable functions provided in
+/// FFI_PhysicalExpr to interact with the foreign table provider.


Suggested change

/// FFI_PhysicalExpr to interact with the foreign table provider.

/// FFI_PhysicalExpr to interact with the expression.

paleolimbot · 2025-11-25T22:43:08Z

datafusion/ffi/src/physical_expr/mod.rs

+#[cfg(test)]
+mod tests {
+    use std::hash::{DefaultHasher, Hash, Hasher};
+    use std::sync::Arc;
+
+    use arrow::array::{record_batch, BooleanArray, RecordBatch};
+    use datafusion_common::{tree_node::DynTreeNode, DataFusionError, ScalarValue};
+    use datafusion_expr::{interval_arithmetic::Interval, statistics::Distribution};
+    use datafusion_physical_expr::expressions::{Column, NegativeExpr, NotExpr};
+    use datafusion_physical_expr_common::physical_expr::{fmt_sql, PhysicalExpr};
+
+    use crate::physical_expr::FFI_PhysicalExpr;
+
+    fn create_test_expr() -> (Arc<dyn PhysicalExpr>, Arc<dyn PhysicalExpr>) {


Perhaps there is another spot for integration tests that is more appropriate, but I wonder if you could use a physical optimizer rule to replace all (or some) expressions in a planned DataFrame/SQL query and check if it executes.

Implement FFI_PhysicalExpr and the structs it needs to support it.

4208bbd

github-actions bot added the ffi Changes to the ffi crate label Nov 24, 2025

timsaucer mentioned this pull request Nov 24, 2025

Support user defined functions as expressions in FFI calls #18671

Open

timsaucer commented Nov 24, 2025

View reviewed changes

Add additional unit tests

d835958

timsaucer marked this pull request as ready for review November 25, 2025 19:42

timsaucer requested a review from Copilot November 25, 2025 19:42

Copilot AI reviewed Nov 25, 2025

View reviewed changes

paleolimbot approved these changes Nov 25, 2025

View reviewed changes

-            // The other alternative is to modify the trait signature.
+            // The other alternative is to modify the trait signature.
+            // However, modifying the trait signature is not viable here because
+            // the PhysicalExpr trait is part of a stable public API used across
+            // multiple crates and FFI boundaries. Changing its signature would
+            // break backward compatibility and require coordinated updates in
+            // downstream consumers, which is undesirable for maintaining API stability.

-        // FFI_PhysicalExpr cannot be compared, so identity equality is the best we can do.
+        // FFI_PhysicalExpr cannot be compared semantically because its private_data is opaque
+        // across FFI boundaries, making it impossible to determine equivalence of underlying state.
+        // Therefore, identity equality is the best we can do.

	/// FFI_PhysicalExpr to interact with the foreign table provider.
	/// FFI_PhysicalExpr to interact with the expression.

Implement FFI_PhysicalExpr and the structs it needs to support it. #18916

Are you sure you want to change the base?

Implement FFI_PhysicalExpr and the structs it needs to support it. #18916

Uh oh!

Conversation

timsaucer commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

timsaucer commented Nov 25, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

comphead commented Nov 25, 2025

Uh oh!

paleolimbot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

timsaucer commented Nov 24, 2025 •

edited

Loading