timsaucer commented on code in PR #18672:
URL: https://github.com/apache/datafusion/pull/18672#discussion_r2556686199
##########
datafusion/ffi/src/udaf/accumulator.rs:
##########
@@ -217,9 +225,20 @@ pub struct ForeignAccumulator {
unsafe impl Send for ForeignAccumulator {}
unsafe impl Sync for ForeignAccumulator {}
-impl From<FFI_Accumulator> for ForeignAccumulator {
- fn from(accumulator: FFI_Accumulator) -> Self {
- Self { accumulator }
+impl From<FFI_Accumulator> for Box<dyn Accumulator> {
+ fn from(mut accumulator: FFI_Accumulator) -> Self {
+ if (accumulator.library_marker_id)() == crate::get_library_marker_id()
{
+ unsafe {
+ let private_data = Box::from_raw(
+ accumulator.private_data as *mut AccumulatorPrivateData,
+ );
+ // We must set this to null to avoid a double free
+ accumulator.private_data = null_mut();
Review Comment:
You made me realize this, groups accumulator, and partition evaluator were
all marked as Send + Sync when they do not need to be. I removed those traits.
##########
datafusion/ffi/src/lib.rs:
##########
@@ -58,5 +58,33 @@ 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
Review Comment:
Added some text
--
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]
