alamb commented on code in PR #18672:
URL: https://github.com/apache/datafusion/pull/18672#discussion_r2552911550
##########
datafusion/ffi/src/udaf/accumulator.rs:
##########
@@ -173,9 +177,11 @@ unsafe extern "C" fn retract_batch_fn_wrapper(
}
unsafe extern "C" fn release_fn_wrapper(accumulator: &mut FFI_Accumulator) {
- let private_data =
- Box::from_raw(accumulator.private_data as *mut AccumulatorPrivateData);
- drop(private_data);
+ if !accumulator.private_data.is_null() {
Review Comment:
is this a drive by cleanup, or do we now need to ensure that the release
wrappers are re-entrant (can be called more than once)?
##########
datafusion/ffi/src/udaf/accumulator.rs:
##########
@@ -70,6 +70,10 @@ pub struct FFI_Accumulator {
/// Internal data. This is only to be accessed by the provider of the
accumulator.
/// A [`ForeignAccumulator`] should never attempt to access this data.
pub private_data: *mut c_void,
+
+ /// Utility to identify when FFI objects are accessed locally through
Review Comment:
Perhaps link to `pub extern "C" fn get_library_marker_id() -> usize {`
##########
datafusion-examples/examples/ffi/ffi_module_loader/src/main.rs:
##########
@@ -49,13 +49,13 @@ async fn main() -> Result<()> {
))?();
// In order to access the table provider within this executable, we need to
- // turn it into a `ForeignTableProvider`.
- let foreign_table_provider: ForeignTableProvider =
(&ffi_table_provider).into();
+ // turn it into a `TableProvider`.
Review Comment:
will this be a FFI / breaking change for DataFusion 52? Specifically, will
it be possible to use the same FFI interface with 52 and load a table provider
written against 51?
I am just asking for clarity
##########
datafusion/ffi/src/udaf/accumulator.rs:
##########
@@ -70,6 +70,10 @@ pub struct FFI_Accumulator {
/// Internal data. This is only to be accessed by the provider of the
accumulator.
/// A [`ForeignAccumulator`] should never attempt to access this data.
pub private_data: *mut c_void,
+
+ /// Utility to identify when FFI objects are accessed locally through
Review Comment:
I feel like this is a super important new api -- maybe can you add a doc
link to somewhere that descsribes the high level `library_marker_id` concept
and
1. how it should be used?
2. how users should choose a library marker (how do we ensure no conflicts?)
##########
docs/source/library-user-guide/upgrading.md:
##########
@@ -152,6 +152,47 @@ Instead of silently succeeding.
The remove API no longer requires a mutable instance
+### FFI object conversion
Review Comment:
Nice - I also think it would be good to mention removal of the return type
functions here
##########
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
Review Comment:
Can you also please document when users can/should use this function? It
seems like may mostly be used internally and most users will not have to worry
about it.
##########
datafusion/ffi/src/udf/mod.rs:
##########
@@ -66,15 +67,7 @@ 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
Review Comment:
I think this is a reduction in API surface now that we are mostly using
Fields -- seems like a good change to me, but we might also want to call it out
in the title/description of the PR
--
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]
