wangfenjin edited a comment on issue #1425:
URL: https://github.com/apache/arrow-rs/issues/1425#issuecomment-1065799969


   I get your point. You are calling rust from Java, so it's right you alloc 
the memory in Java. The problem is rust assume this pointer is from 
Arc::into_raw, but acctually it's not it's just a raw pointer from JNI. In this 
case, our solution should be add a new API like:
   
   ```rust
       /// creates a new [ArrowArray] from two pointers. Used to import from 
the C Data Interface.
       /// # Safety
       /// See safety of [ArrowArray]
       /// # Error
       /// Errors if any of the pointers is null
       pub unsafe fn try_from_c(
           array: *const FFI_ArrowArray,
           schema: *const FFI_ArrowSchema,
       ) -> Result<Self> {
           if array.is_null() || schema.is_null() {
               return Err(ArrowError::MemoryError(
                   "At least one of the pointers passed to `try_from_raw` is 
null"
                       .to_string(),
               ));
           };
           Ok(Self {
               array: Arc::new(*array), // notice we don't need to clone here, 
if will also solve your problem
               schema: Arc::new(*schema),
           })
       }
   ```
   
   The idea is we don't need to Clone, why bother? As in your Java program you 
need to hold the pointer as long as you need it, and can only free it when the 
job is done.
   
   There are several drawbacks in the current solution:
   1. It breaks current API convention. Anyone use the API like me will be 
detected memory leak because of this change.
   2. You can say your Java program don't leak memory because you always 
holding the pointer, but in some aspect we can say it's, because we alloc two 
struct point to the same data, which can be easily and should managed in one.
   3. Allow clone will make us have the same issue mentioned by @jorgecarleitao 
, I add a demo here https://github.com/apache/arrow-rs/pull/1431
   
   About the link you mentioned [Moving an 
array](https://arrow.apache.org/docs/format/CDataInterface.html#moving-an-array),
 please noted that it talks about *consumer* which is rust in this case, need 
to call release_call if it tries to move the data, seems have nothing to do 
with our discuess here. 
   > If you really want to free it, make sure to clear up the release field in 
the struct. This is easy to do with Java FFI API. 
   
   In here you are talking about the producer side, but actually you can't just 
clear up the release field and forget about it. What if rust have any issue to 
release the data, Java as the producer, as the one alloc the memory, should 
still be responsible to release it. Again, "the one who allocate it should free 
it".


-- 
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: github-unsubscr...@arrow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


Reply via email to