martin-g commented on code in PR #18672:
URL: https://github.com/apache/datafusion/pull/18672#discussion_r2548813339
##########
datafusion/ffi/README.md:
##########
@@ -101,6 +101,36 @@ In this crate we have a variety of structs which closely
mimic the behavior of
their internal counterparts. To see detailed notes about how to use them, see
the example in `FFI_TableProvider`.
+## Library Marker ID
+
+When reviewing the code, many of the structs in this crate contain a call to
+a `library_maker_id`. The purpose of this call is to determine if a library is
Review Comment:
```suggestion
a `library_marker_id`. The purpose of this call is to determine if a library
is
```
##########
datafusion/ffi/src/udf/mod.rs:
##########
@@ -66,13 +67,6 @@ pub struct FFI_ScalarUDF {
/// FFI equivalent to the `volatility` of a [`ScalarUDF`]
pub volatility: FFI_Volatility,
- /// Determines the return type of the underlying [`ScalarUDF`] based on the
- /// argument types.
- pub return_type: unsafe extern "C" fn(
- udf: &Self,
- arg_types: RVec<WrappedSchema>,
- ) -> RResult<WrappedSchema, RString>,
-
/// Determines the return info of the underlying [`ScalarUDF`]. Either this
/// or return_type may be implemented on a UDF.
Review Comment:
`return_type` is removed few lines above
##########
docs/source/library-user-guide/upgrading.md:
##########
@@ -67,6 +67,47 @@ SELECT median(c1) IGNORE NULLS FROM table
Instead of silently succeeding.
+### FFI object conversion
+
+Many of the structs in the `datafusion-ffi` crate have been updated to allow
easier
+conversion to the underlying trait types they represent. This simplifies some
code
+paths, but also provides an additional improvement in cases where library code
goes
+through a round trip via the foreign function interface.
+
+To update your code, suppose you have a `FFI_SchemaProvider` called
`ffi_provider`
+and you wish to use this as a `SchemaProvider`. In the old approach you would
do
+something like:
+
+```rust,ignore
+ let foreign_provider: ForeignSchemaProvider = provider.into();
+ let foreign_provider = Arc::new(foreign_provider) as Arc<dyn
SchemaProvider>;
+```
+
+This code should now be written as:
+
+```rust,ignore
+ let foreign_provider: Arc<dyn SchemaProvider + Send> = provider.into();
Review Comment:
```suggestion
let foreign_provider: Arc<dyn SchemaProvider + Send> =
ffi_provider.into();
```
##########
datafusion/ffi/src/plan_properties.rs:
##########
@@ -314,19 +318,43 @@ mod tests {
let _ = eqp.reorder([PhysicalSortExpr::new_default(
datafusion::physical_plan::expressions::col("a", &schema)?,
)]);
- let original_props = PlanProperties::new(
+ Ok(PlanProperties::new(
eqp,
Partitioning::RoundRobinBatch(3),
EmissionType::Incremental,
Boundedness::Bounded,
- );
+ ))
+ }
- let local_props_ptr = FFI_PlanProperties::from(&original_props);
+ #[test]
+ fn test_round_trip_ffi_plan_properties() -> Result<()> {
+ let original_props = create_test_props()?;
+ let mut local_props_ptr = FFI_PlanProperties::from(&original_props);
+ local_props_ptr.library_marker_id = crate::mock_foreign_marker_id;
let foreign_props: PlanProperties = local_props_ptr.try_into()?;
assert_eq!(format!("{foreign_props:?}"),
format!("{original_props:?}"));
Ok(())
}
+
+ #[test]
+ fn test_ffi_execution_plan_local_bypass() -> Result<()> {
+ let props = create_test_props()?;
+
+ let ffi_plan = FFI_PlanProperties::from(&props);
+
+ // Verify local libraries
+ let foreign_plan: PlanProperties = ffi_plan.try_into()?;
+ assert_eq!(format!("{foreign_plan:?}"), format!("{:?}", foreign_plan));
Review Comment:
comparing with itself
##########
docs/source/library-user-guide/upgrading.md:
##########
@@ -67,6 +67,47 @@ SELECT median(c1) IGNORE NULLS FROM table
Instead of silently succeeding.
+### FFI object conversion
+
+Many of the structs in the `datafusion-ffi` crate have been updated to allow
easier
+conversion to the underlying trait types they represent. This simplifies some
code
+paths, but also provides an additional improvement in cases where library code
goes
+through a round trip via the foreign function interface.
+
+To update your code, suppose you have a `FFI_SchemaProvider` called
`ffi_provider`
+and you wish to use this as a `SchemaProvider`. In the old approach you would
do
+something like:
+
+```rust,ignore
+ let foreign_provider: ForeignSchemaProvider = provider.into();
Review Comment:
```suggestion
let foreign_provider: ForeignSchemaProvider = ffi_provider.into();
```
##########
datafusion-examples/examples/ffi/ffi_module_loader/src/main.rs:
##########
@@ -50,12 +50,12 @@ async fn main() -> Result<()> {
// In order to access the table provider within this executable, we need to
// turn it into a `ForeignTableProvider`.
Review Comment:
```suggestion
// turn it into a `Arc<dyn TableProvider>`.
```
##########
datafusion/ffi/src/lib.rs:
##########
@@ -58,5 +58,31 @@ pub extern "C" fn version() -> u64 {
version.major
}
+static LIBRARY_MARKER: u8 = 0;
+
+/// This utility is used to determine if two FFI structs are within
+/// the same library. It is possible that the interplay between
+/// foreign and local functions calls create one FFI struct that
+/// references another. It is helpful to determine if a foreign
+/// struct is truly foreign or in the same library. If we are in the
+/// same library, then we can access the underlying types directly.
+///
+/// This function works by checking the address of the library
+/// marker. Each library that implements the FFI code will have
+/// a different address for the marker. By checking the marker
+/// address we can determine if a struct is truly Foreign or is
+/// actually within the same originating library.
+pub extern "C" fn get_library_marker_id() -> u64 {
Review Comment:
```suggestion
pub extern "C" fn get_library_marker_id() -> usize {
```
?!
to support 32/128 bit systems
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
