You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/03/12 18:50:06 UTC

[GitHub] [arrow-rs] viirya opened a new pull request #1432: Replace Arc with Box in ArrowArray for FFI structs

viirya opened a new pull request #1432:
URL: https://github.com/apache/arrow-rs/pull/1432


   # Which issue does this PR close?
   
   <!---
   We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
   -->
   
   Closes #1425.
   
   # Rationale for this change
    
    <!---
    Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
    Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
   -->
   
   # What changes are included in this PR?
   
   <!---
   There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
   -->
   
   # Are there any user-facing changes?
   
   
   <!---
   If there are user-facing changes then we may require documentation to be updated before approving the PR.
   -->
   
   <!---
   If there are any breaking changes to public APIs, please add the `breaking change` label.
   -->
   


-- 
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-unsubscribe@arrow.apache.org

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

[GitHub] [arrow-rs] viirya commented on pull request #1432: Replace Arc with Box in ArrowArray for FFI structs

Posted by GitBox <gi...@apache.org>.
viirya commented on pull request #1432:
URL: https://github.com/apache/arrow-rs/pull/1432#issuecomment-1065972188


   cc @sunchao @alamb @wangfenjin @jorgecarleitao 


-- 
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-unsubscribe@arrow.apache.org

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

[GitHub] [arrow-rs] alamb commented on pull request #1432: Replace Arc with Box in ArrowArray for FFI structs

Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1432:
URL: https://github.com/apache/arrow-rs/pull/1432#issuecomment-1066438336


   > Note that because the struct pointers are now Box instead of Arc, it requires the users of these structs to be more careful on managing them. For example, ArrayData::try_from. Users need to keep these Box/raw pointers (in order not to release them too early) and do release after using the array data.
   
   > That's why I personally prefer alternative Arc + clone + removing source structs' release approach (see https://github.com/apache/arrow-rs/pull/1436). But I'm fine with Box approach too if this is preferred by the community.
   
   I think the "be explicit about memory management" is the typical Rust mantra in this case, so I would prefer the `Box` approach. 
   
   Perhaps in your use of the FFI structs, you could use `std::mem::forget` if you don't mind not releasing the memory over time and don't want to track the allocations individually.


-- 
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-unsubscribe@arrow.apache.org

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

[GitHub] [arrow-rs] viirya commented on a change in pull request #1432: Replace Arc with Box in ArrowArray for FFI structs

Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #1432:
URL: https://github.com/apache/arrow-rs/pull/1432#discussion_r826550474



##########
File path: arrow/src/array/array.rs
##########
@@ -629,7 +629,9 @@ pub unsafe fn make_array_from_raw(
     schema: *const ffi::FFI_ArrowSchema,
 ) -> Result<ArrayRef> {
     let array = ffi::ArrowArray::try_from_raw(array, schema)?;
-    let data = ArrayData::try_from(array)?;
+    let data = ArrayData::try_from(&array)?;
+    // Avoid dropping the `Box` pointers and trigger the `release` mechanism.
+    let _ = ffi::ArrowArray::into_raw(array);

Review comment:
       When we try to get an array data from ffi array and schema, we will keep `Arc` pointer of the array in created `Buffer`. That is why we don't need to specially take care about when `array` and `schema` are out of scope.
   
   But as we move to `Box`, it is not possible now. As we cannot clone ffi struct too, we cannot let the `release` is called when the created `Buffer` to be deallocated. That's the pain point.
   
   




-- 
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-unsubscribe@arrow.apache.org

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

[GitHub] [arrow-rs] viirya commented on a change in pull request #1432: Replace Arc with Box in ArrowArray for FFI structs

Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #1432:
URL: https://github.com/apache/arrow-rs/pull/1432#discussion_r825483017



##########
File path: arrow/src/array/array.rs
##########
@@ -629,7 +629,9 @@ pub unsafe fn make_array_from_raw(
     schema: *const ffi::FFI_ArrowSchema,
 ) -> Result<ArrayRef> {
     let array = ffi::ArrowArray::try_from_raw(array, schema)?;
-    let data = ArrayData::try_from(array)?;
+    let data = ArrayData::try_from(&array)?;
+    // Avoid dropping the `Box` pointers and trigger the `release` mechanism.
+    let _ = ffi::ArrowArray::into_raw(array);

Review comment:
       Since `ArrayData::try_from()` moves `ffi::ArrowArray`, it will drop the `ArrowArray` and trigger `release` for the structs (as they are just `Box` pointers now). Users cannot prevent it happened. For example, without this change, our internal usecase got a SIGSEGV.
   
   So I change it to a borrowed reference to avoid dropping/releasing there.
   
   




-- 
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-unsubscribe@arrow.apache.org

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

[GitHub] [arrow-rs] wangfenjin commented on a change in pull request #1432: Replace Arc with Box in ArrowArray for FFI structs

Posted by GitBox <gi...@apache.org>.
wangfenjin commented on a change in pull request #1432:
URL: https://github.com/apache/arrow-rs/pull/1432#discussion_r825588387



##########
File path: arrow/src/array/array.rs
##########
@@ -629,7 +629,9 @@ pub unsafe fn make_array_from_raw(
     schema: *const ffi::FFI_ArrowSchema,
 ) -> Result<ArrayRef> {
     let array = ffi::ArrowArray::try_from_raw(array, schema)?;
-    let data = ArrayData::try_from(array)?;
+    let data = ArrayData::try_from(&array)?;
+    // Avoid dropping the `Box` pointers and trigger the `release` mechanism.
+    let _ = ffi::ArrowArray::into_raw(array);

Review comment:
       If we follow this style seems also a lot of change




-- 
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-unsubscribe@arrow.apache.org

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

[GitHub] [arrow-rs] wangfenjin commented on a change in pull request #1432: Replace Arc with Box in ArrowArray for FFI structs

Posted by GitBox <gi...@apache.org>.
wangfenjin commented on a change in pull request #1432:
URL: https://github.com/apache/arrow-rs/pull/1432#discussion_r825560814



##########
File path: arrow/src/array/array.rs
##########
@@ -629,7 +629,9 @@ pub unsafe fn make_array_from_raw(
     schema: *const ffi::FFI_ArrowSchema,
 ) -> Result<ArrayRef> {
     let array = ffi::ArrowArray::try_from_raw(array, schema)?;
-    let data = ArrayData::try_from(array)?;
+    let data = ArrayData::try_from(&array)?;
+    // Avoid dropping the `Box` pointers and trigger the `release` mechanism.
+    let _ = ffi::ArrowArray::into_raw(array);

Review comment:
       Hi @viirya , understand the issue here.
   
   I'm not sure if https://github.com/apache/arrow-rs/pull/1441 this can be the solution? Or even we can add a ArrowArray::try_from_box() to do the similar thing




-- 
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-unsubscribe@arrow.apache.org

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

[GitHub] [arrow-rs] viirya commented on a change in pull request #1432: Replace Arc with Box in ArrowArray for FFI structs

Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #1432:
URL: https://github.com/apache/arrow-rs/pull/1432#discussion_r826611252



##########
File path: arrow/src/array/array.rs
##########
@@ -629,7 +629,9 @@ pub unsafe fn make_array_from_raw(
     schema: *const ffi::FFI_ArrowSchema,
 ) -> Result<ArrayRef> {
     let array = ffi::ArrowArray::try_from_raw(array, schema)?;
-    let data = ArrayData::try_from(array)?;
+    let data = ArrayData::try_from(&array)?;
+    // Avoid dropping the `Box` pointers and trigger the `release` mechanism.
+    let _ = ffi::ArrowArray::into_raw(array);

Review comment:
       Previously you don't need it. As `Arc` is kept in the created Buffer of Array data, you can rely on deallocation of the Buffer to call `release` of such ffi structs.
   
   But `Box` cannot give us such benefit. So it makes the management more explicit and relying on users. We need to keep these structs so `release` won't be called, before we don't need the Array data (Buffer).
   
   The code is at `ArrowArrayRef.to_data`. It is to create an `ArrayData` from an `ArrowArray(Ref)`. And you can follow `buffers` -> `create_buffer` -> `Buffer::from_unowned`.




-- 
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-unsubscribe@arrow.apache.org

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

[GitHub] [arrow-rs] wangfenjin commented on a change in pull request #1432: Replace Arc with Box in ArrowArray for FFI structs

Posted by GitBox <gi...@apache.org>.
wangfenjin commented on a change in pull request #1432:
URL: https://github.com/apache/arrow-rs/pull/1432#discussion_r825432249



##########
File path: arrow/src/array/array.rs
##########
@@ -629,7 +629,9 @@ pub unsafe fn make_array_from_raw(
     schema: *const ffi::FFI_ArrowSchema,
 ) -> Result<ArrayRef> {
     let array = ffi::ArrowArray::try_from_raw(array, schema)?;
-    let data = ArrayData::try_from(array)?;
+    let data = ArrayData::try_from(&array)?;
+    // Avoid dropping the `Box` pointers and trigger the `release` mechanism.
+    let _ = ffi::ArrowArray::into_raw(array);

Review comment:
       Why we need to change ArrayData::try_from() to have &? This line seems very tricky.
   
   And as we change the ArrayData::try_from() API, all other users may also need to change the code and also add this tricky line.




-- 
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-unsubscribe@arrow.apache.org

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

[GitHub] [arrow-rs] viirya commented on a change in pull request #1432: Replace Arc with Box in ArrowArray for FFI structs

Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #1432:
URL: https://github.com/apache/arrow-rs/pull/1432#discussion_r825578179



##########
File path: arrow/src/array/array.rs
##########
@@ -629,7 +629,9 @@ pub unsafe fn make_array_from_raw(
     schema: *const ffi::FFI_ArrowSchema,
 ) -> Result<ArrayRef> {
     let array = ffi::ArrowArray::try_from_raw(array, schema)?;
-    let data = ArrayData::try_from(array)?;
+    let data = ArrayData::try_from(&array)?;
+    // Avoid dropping the `Box` pointers and trigger the `release` mechanism.
+    let _ = ffi::ArrowArray::into_raw(array);

Review comment:
       Oh, I'm wrong. I'm not aware that there is `from(v: Box<T>)` API in Arc.




-- 
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-unsubscribe@arrow.apache.org

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

[GitHub] [arrow-rs] wangfenjin commented on a change in pull request #1432: Replace Arc with Box in ArrowArray for FFI structs

Posted by GitBox <gi...@apache.org>.
wangfenjin commented on a change in pull request #1432:
URL: https://github.com/apache/arrow-rs/pull/1432#discussion_r825585866



##########
File path: arrow/src/array/array.rs
##########
@@ -629,7 +629,9 @@ pub unsafe fn make_array_from_raw(
     schema: *const ffi::FFI_ArrowSchema,
 ) -> Result<ArrayRef> {
     let array = ffi::ArrowArray::try_from_raw(array, schema)?;
-    let data = ArrayData::try_from(array)?;
+    let data = ArrayData::try_from(&array)?;
+    // Avoid dropping the `Box` pointers and trigger the `release` mechanism.
+    let _ = ffi::ArrowArray::into_raw(array);

Review comment:
       Does it also copy the arrow data? Then the performance might be bad




-- 
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-unsubscribe@arrow.apache.org

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

[GitHub] [arrow-rs] codecov-commenter commented on pull request #1432: Replace Arc with Box in ArrowArray for FFI structs

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #1432:
URL: https://github.com/apache/arrow-rs/pull/1432#issuecomment-1065950456


   # [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1432?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#1432](https://codecov.io/gh/apache/arrow-rs/pull/1432?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (cdabda3) into [master](https://codecov.io/gh/apache/arrow-rs/commit/729934c5311b42fc7fd3522a22ddc049bdde9569?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (729934c) will **increase** coverage by `0.00%`.
   > The diff coverage is `73.68%`.
   
   > :exclamation: Current head cdabda3 differs from pull request most recent head 9f4126a. Consider uploading reports for the commit 9f4126a to get more accurate results
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/arrow-rs/pull/1432/graphs/tree.svg?width=650&height=150&src=pr&token=pq9V9qWZ1N&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/arrow-rs/pull/1432?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@           Coverage Diff           @@
   ##           master    #1432   +/-   ##
   =======================================
     Coverage   82.67%   82.68%           
   =======================================
     Files         185      185           
     Lines       53822    53820    -2     
   =======================================
     Hits        44500    44500           
   + Misses       9322     9320    -2     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1432?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [arrow/src/array/array.rs](https://codecov.io/gh/apache/arrow-rs/pull/1432/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2FycmF5LnJz) | `85.18% <0.00%> (-0.23%)` | :arrow_down: |
   | [arrow/src/bytes.rs](https://codecov.io/gh/apache/arrow-rs/pull/1432/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2J5dGVzLnJz) | `56.25% <0.00%> (ø)` | |
   | [arrow/src/ffi.rs](https://codecov.io/gh/apache/arrow-rs/pull/1432/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2ZmaS5ycw==) | `85.91% <82.75%> (+0.13%)` | :arrow_up: |
   | [arrow/src/array/ffi.rs](https://codecov.io/gh/apache/arrow-rs/pull/1432/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2ZmaS5ycw==) | `100.00% <100.00%> (ø)` | |
   | [arrow/src/buffer/immutable.rs](https://codecov.io/gh/apache/arrow-rs/pull/1432/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2J1ZmZlci9pbW11dGFibGUucnM=) | `99.45% <100.00%> (ø)` | |
   | [arrow/src/array/transform/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/1432/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L3RyYW5zZm9ybS9tb2QucnM=) | `86.20% <0.00%> (-0.12%)` | :arrow_down: |
   | [parquet\_derive/src/parquet\_field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1432/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldF9kZXJpdmUvc3JjL3BhcnF1ZXRfZmllbGQucnM=) | `66.21% <0.00%> (+0.22%)` | :arrow_up: |
   | [arrow/src/datatypes/field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1432/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2RhdGF0eXBlcy9maWVsZC5ycw==) | `54.10% <0.00%> (+0.30%)` | :arrow_up: |
   | [arrow/src/datatypes/datatype.rs](https://codecov.io/gh/apache/arrow-rs/pull/1432/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2RhdGF0eXBlcy9kYXRhdHlwZS5ycw==) | `66.80% <0.00%> (+0.39%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1432?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1432?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [729934c...9f4126a](https://codecov.io/gh/apache/arrow-rs/pull/1432?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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-unsubscribe@arrow.apache.org

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

[GitHub] [arrow-rs] viirya commented on a change in pull request #1432: Replace Arc with Box in ArrowArray for FFI structs

Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #1432:
URL: https://github.com/apache/arrow-rs/pull/1432#discussion_r826550474



##########
File path: arrow/src/array/array.rs
##########
@@ -629,7 +629,9 @@ pub unsafe fn make_array_from_raw(
     schema: *const ffi::FFI_ArrowSchema,
 ) -> Result<ArrayRef> {
     let array = ffi::ArrowArray::try_from_raw(array, schema)?;
-    let data = ArrayData::try_from(array)?;
+    let data = ArrayData::try_from(&array)?;
+    // Avoid dropping the `Box` pointers and trigger the `release` mechanism.
+    let _ = ffi::ArrowArray::into_raw(array);

Review comment:
       When previously we try to get an array data from ffi array and schema, we will keep `Arc` pointer of the array in created `Buffer`. That is why we don't need to specially take care about when `array` and `schema` are out of scope.
   
   But as we move to `Box`, it is not possible now. As we cannot clone ffi struct too, we cannot let the `release` is called when the created `Buffer` to be deallocated.
   
   




-- 
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-unsubscribe@arrow.apache.org

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

[GitHub] [arrow-rs] viirya commented on pull request #1432: Replace Arc with Box in ArrowArray for FFI structs

Posted by GitBox <gi...@apache.org>.
viirya commented on pull request #1432:
URL: https://github.com/apache/arrow-rs/pull/1432#issuecomment-1075875187


   This can be closed now.


-- 
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-unsubscribe@arrow.apache.org

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

[GitHub] [arrow-rs] sunchao commented on a change in pull request #1432: Replace Arc with Box in ArrowArray for FFI structs

Posted by GitBox <gi...@apache.org>.
sunchao commented on a change in pull request #1432:
URL: https://github.com/apache/arrow-rs/pull/1432#discussion_r827449744



##########
File path: arrow/src/array/array.rs
##########
@@ -629,7 +629,9 @@ pub unsafe fn make_array_from_raw(
     schema: *const ffi::FFI_ArrowSchema,
 ) -> Result<ArrayRef> {
     let array = ffi::ArrowArray::try_from_raw(array, schema)?;
-    let data = ArrayData::try_from(array)?;
+    let data = ArrayData::try_from(&array)?;
+    // Avoid dropping the `Box` pointers and trigger the `release` mechanism.
+    let _ = ffi::ArrowArray::into_raw(array);

Review comment:
       I see, thanks for the explanation.
   
   I'm wondering if we can still use `Arc` but drop the "envelop memory" allocated for the struct holding the actual pointers, for the input array and schema. For example:
   ```rust
       pub unsafe fn try_from_raw(
           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(),
               ));
           };
   
           let array_mut = array as *mut FFI_ArrowArray;
           let schema_mut = schema as *mut FFI_ArrowSchema;
   
           let array_data = std::ptr::replace(array_mut, FFI_ArrowArray::empty());
           let schema_data = std::ptr::replace(schema_mut, FFI_ArrowSchema::empty());
   
           std::ptr::drop_in_place(array_mut);
           std::ptr::drop_in_place(schema_mut);
   
           Ok(Self {
               array: Arc::new(array_data),
               schema: Arc::new(schema_data),
           })
       }
   ```




-- 
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-unsubscribe@arrow.apache.org

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

[GitHub] [arrow-rs] viirya commented on a change in pull request #1432: Replace Arc with Box in ArrowArray for FFI structs

Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #1432:
URL: https://github.com/apache/arrow-rs/pull/1432#discussion_r827577920



##########
File path: arrow/src/array/array.rs
##########
@@ -629,7 +629,9 @@ pub unsafe fn make_array_from_raw(
     schema: *const ffi::FFI_ArrowSchema,
 ) -> Result<ArrayRef> {
     let array = ffi::ArrowArray::try_from_raw(array, schema)?;
-    let data = ArrayData::try_from(array)?;
+    let data = ArrayData::try_from(&array)?;
+    // Avoid dropping the `Box` pointers and trigger the `release` mechanism.
+    let _ = ffi::ArrowArray::into_raw(array);

Review comment:
       I've tried it. It seems okay. `Arc::from` previously not work, I think, is because it calls allocator to deallocate the memory allocation. As it is allocated by Java in our case, we cannot let Rust to deallocate it.
   
   `std::ptr::drop_in_place` seems only trigger dropping. As we make it as empty structs, it won't trigger `release`. I think this is close to #1436 which cleans up `release` field of source structs after cloning it. Here we in fact still clone it, but just internally and don't expose `clone`.
   
   Looks good to me. Thanks @sunchao . 
   
   cc @alamb @wangfenjin WDYT? Are you agreed with this approach?




-- 
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-unsubscribe@arrow.apache.org

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

[GitHub] [arrow-rs] viirya commented on a change in pull request #1432: Replace Arc with Box in ArrowArray for FFI structs

Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #1432:
URL: https://github.com/apache/arrow-rs/pull/1432#discussion_r827471258



##########
File path: arrow/src/array/array.rs
##########
@@ -629,7 +629,9 @@ pub unsafe fn make_array_from_raw(
     schema: *const ffi::FFI_ArrowSchema,
 ) -> Result<ArrayRef> {
     let array = ffi::ArrowArray::try_from_raw(array, schema)?;
-    let data = ArrayData::try_from(array)?;
+    let data = ArrayData::try_from(&array)?;
+    // Avoid dropping the `Box` pointers and trigger the `release` mechanism.
+    let _ = ffi::ArrowArray::into_raw(array);

Review comment:
       It looks close to previous suggestion `Arc::from` as it also copies bytes from source structs. But it causes SIGSEGV. I will try to test this.




-- 
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-unsubscribe@arrow.apache.org

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

[GitHub] [arrow-rs] wangfenjin commented on a change in pull request #1432: Replace Arc with Box in ArrowArray for FFI structs

Posted by GitBox <gi...@apache.org>.
wangfenjin commented on a change in pull request #1432:
URL: https://github.com/apache/arrow-rs/pull/1432#discussion_r825587396



##########
File path: arrow/src/array/array.rs
##########
@@ -629,7 +629,9 @@ pub unsafe fn make_array_from_raw(
     schema: *const ffi::FFI_ArrowSchema,
 ) -> Result<ArrayRef> {
     let array = ffi::ArrowArray::try_from_raw(array, schema)?;
-    let data = ArrayData::try_from(array)?;
+    let data = ArrayData::try_from(&array)?;
+    // Avoid dropping the `Box` pointers and trigger the `release` mechanism.
+    let _ = ffi::ArrowArray::into_raw(array);

Review comment:
       My understanding is, it should be zero-copy if we share arrow data by C data interface




-- 
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-unsubscribe@arrow.apache.org

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

[GitHub] [arrow-rs] viirya commented on pull request #1432: Replace Arc with Box in ArrowArray for FFI structs

Posted by GitBox <gi...@apache.org>.
viirya commented on pull request #1432:
URL: https://github.com/apache/arrow-rs/pull/1432#issuecomment-1066187145


   Note that because the struct pointers are now `Box` instead of `Arc`, it requires the users of these structs to be more careful on managing them. For example, `ArrayData::try_from`. Users need to keep these `Box`/raw pointers (in order not to release them too early) and do `release` after using the array data.
   
   That's why I personally prefer alternative `Arc` + `clone` + removing source structs' `release` approach. But I'm fine with `Box` approach too if this is preferred by the community.


-- 
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-unsubscribe@arrow.apache.org

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

[GitHub] [arrow-rs] viirya commented on pull request #1432: Replace Arc with Box in ArrowArray for FFI structs

Posted by GitBox <gi...@apache.org>.
viirya commented on pull request #1432:
URL: https://github.com/apache/arrow-rs/pull/1432#issuecomment-1066448184


   @alamb Thanks. It makes sense. Then I think the `Box` approach is good.
   


-- 
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-unsubscribe@arrow.apache.org

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

[GitHub] [arrow-rs] viirya commented on a change in pull request #1432: Replace Arc with Box in ArrowArray for FFI structs

Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #1432:
URL: https://github.com/apache/arrow-rs/pull/1432#discussion_r827588611



##########
File path: arrow/src/array/array.rs
##########
@@ -629,7 +629,9 @@ pub unsafe fn make_array_from_raw(
     schema: *const ffi::FFI_ArrowSchema,
 ) -> Result<ArrayRef> {
     let array = ffi::ArrowArray::try_from_raw(array, schema)?;
-    let data = ArrayData::try_from(array)?;
+    let data = ArrayData::try_from(&array)?;
+    // Avoid dropping the `Box` pointers and trigger the `release` mechanism.
+    let _ = ffi::ArrowArray::into_raw(array);

Review comment:
       The suggested approach is at #1449.




-- 
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-unsubscribe@arrow.apache.org

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

[GitHub] [arrow-rs] viirya commented on a change in pull request #1432: Replace Arc with Box in ArrowArray for FFI structs

Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #1432:
URL: https://github.com/apache/arrow-rs/pull/1432#discussion_r825591940



##########
File path: arrow/src/array/array.rs
##########
@@ -629,7 +629,9 @@ pub unsafe fn make_array_from_raw(
     schema: *const ffi::FFI_ArrowSchema,
 ) -> Result<ArrayRef> {
     let array = ffi::ArrowArray::try_from_raw(array, schema)?;
-    let data = ArrayData::try_from(array)?;
+    let data = ArrayData::try_from(&array)?;
+    // Avoid dropping the `Box` pointers and trigger the `release` mechanism.
+    let _ = ffi::ArrowArray::into_raw(array);

Review comment:
       I think `from(v: Box<T>)` only copies the content of the structs, i.e., the pointer. The array data is out of the structs. I think this is basically similar to #1436, i.e. cloning the source structs and making sure we don't trigger `release` on source structs (either cleaning up `release`, or like`from(v: Box<T>)` to copy it by bytes without dropping it).
   
   I think `from(v: Box<T>)` should work for our usecase, but I need to test it to make sure we don't miss anything. I will update this accordingly.
   




-- 
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-unsubscribe@arrow.apache.org

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

[GitHub] [arrow-rs] alamb edited a comment on pull request #1432: Replace Arc with Box in ArrowArray for FFI structs

Posted by GitBox <gi...@apache.org>.
alamb edited a comment on pull request #1432:
URL: https://github.com/apache/arrow-rs/pull/1432#issuecomment-1066443787


   https://github.com/apache/arrow-rs/issues/1442 tracks the patch releases


-- 
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-unsubscribe@arrow.apache.org

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

[GitHub] [arrow-rs] alamb commented on pull request #1432: Replace Arc with Box in ArrowArray for FFI structs

Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1432:
URL: https://github.com/apache/arrow-rs/pull/1432#issuecomment-1066443787


   https://github.com/apache/arrow-rs/issues/1442


-- 
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-unsubscribe@arrow.apache.org

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

[GitHub] [arrow-rs] viirya edited a comment on pull request #1432: Replace Arc with Box in ArrowArray for FFI structs

Posted by GitBox <gi...@apache.org>.
viirya edited a comment on pull request #1432:
URL: https://github.com/apache/arrow-rs/pull/1432#issuecomment-1066187145


   Note that because the struct pointers are now `Box` instead of `Arc`, it requires the users of these structs to be more careful on managing them. For example, `ArrayData::try_from`. Users need to keep these `Box`/raw pointers (in order not to release them too early) and do `release` after using the array data.
   
   That's why I personally prefer alternative `Arc` + `clone` + removing source structs' `release` approach (see https://github.com/apache/arrow-rs/pull/1436). But I'm fine with `Box` approach too if this is preferred by the community.


-- 
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-unsubscribe@arrow.apache.org

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

[GitHub] [arrow-rs] viirya commented on a change in pull request #1432: Replace Arc with Box in ArrowArray for FFI structs

Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #1432:
URL: https://github.com/apache/arrow-rs/pull/1432#discussion_r825572234



##########
File path: arrow/src/array/array.rs
##########
@@ -629,7 +629,9 @@ pub unsafe fn make_array_from_raw(
     schema: *const ffi::FFI_ArrowSchema,
 ) -> Result<ArrayRef> {
     let array = ffi::ArrowArray::try_from_raw(array, schema)?;
-    let data = ArrayData::try_from(array)?;
+    let data = ArrayData::try_from(&array)?;
+    // Avoid dropping the `Box` pointers and trigger the `release` mechanism.
+    let _ = ffi::ArrowArray::into_raw(array);

Review comment:
       I don't think so.
   
   `Arc::from(Box::from_raw(array as *mut FFI_ArrowArray))` is `Arc<Box<FFI_ArrowArray>>`. Then the raw pointer is `*Box<FFI_ArrowArray>`, but you treat it as `*FFI_ArrowArray`.
   
   




-- 
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-unsubscribe@arrow.apache.org

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

[GitHub] [arrow-rs] codecov-commenter edited a comment on pull request #1432: Replace Arc with Box in ArrowArray for FFI structs

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1432:
URL: https://github.com/apache/arrow-rs/pull/1432#issuecomment-1065950456


   # [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1432?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#1432](https://codecov.io/gh/apache/arrow-rs/pull/1432?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e10b1d6) into [master](https://codecov.io/gh/apache/arrow-rs/commit/729934c5311b42fc7fd3522a22ddc049bdde9569?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (729934c) will **decrease** coverage by `0.00%`.
   > The diff coverage is `71.79%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/arrow-rs/pull/1432/graphs/tree.svg?width=650&height=150&src=pr&token=pq9V9qWZ1N&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/arrow-rs/pull/1432?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #1432      +/-   ##
   ==========================================
   - Coverage   82.67%   82.67%   -0.01%     
   ==========================================
     Files         185      185              
     Lines       53822    53820       -2     
   ==========================================
   - Hits        44500    44498       -2     
     Misses       9322     9322              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1432?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [arrow/src/array/array.rs](https://codecov.io/gh/apache/arrow-rs/pull/1432/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2FycmF5LnJz) | `85.18% <0.00%> (-0.23%)` | :arrow_down: |
   | [arrow/src/bytes.rs](https://codecov.io/gh/apache/arrow-rs/pull/1432/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2J5dGVzLnJz) | `56.25% <0.00%> (ø)` | |
   | [arrow/src/pyarrow.rs](https://codecov.io/gh/apache/arrow-rs/pull/1432/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL3B5YXJyb3cucnM=) | `0.00% <0.00%> (ø)` | |
   | [arrow/src/ffi.rs](https://codecov.io/gh/apache/arrow-rs/pull/1432/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2ZmaS5ycw==) | `85.91% <82.75%> (+0.13%)` | :arrow_up: |
   | [arrow/src/array/ffi.rs](https://codecov.io/gh/apache/arrow-rs/pull/1432/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2ZmaS5ycw==) | `100.00% <100.00%> (ø)` | |
   | [arrow/src/buffer/immutable.rs](https://codecov.io/gh/apache/arrow-rs/pull/1432/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2J1ZmZlci9pbW11dGFibGUucnM=) | `99.45% <100.00%> (ø)` | |
   | [parquet/src/encodings/encoding.rs](https://codecov.io/gh/apache/arrow-rs/pull/1432/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvZW5jb2RpbmdzL2VuY29kaW5nLnJz) | `93.52% <0.00%> (-0.20%)` | :arrow_down: |
   | [arrow/src/array/transform/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/1432/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L3RyYW5zZm9ybS9tb2QucnM=) | `86.20% <0.00%> (-0.12%)` | :arrow_down: |
   | [parquet\_derive/src/parquet\_field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1432/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldF9kZXJpdmUvc3JjL3BhcnF1ZXRfZmllbGQucnM=) | `65.98% <0.00%> (ø)` | |
   | [arrow/src/datatypes/field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1432/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2RhdGF0eXBlcy9maWVsZC5ycw==) | `54.10% <0.00%> (+0.30%)` | :arrow_up: |
   | ... and [1 more](https://codecov.io/gh/apache/arrow-rs/pull/1432/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1432?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1432?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [729934c...e10b1d6](https://codecov.io/gh/apache/arrow-rs/pull/1432?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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-unsubscribe@arrow.apache.org

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

[GitHub] [arrow-rs] viirya closed pull request #1432: Replace Arc with Box in ArrowArray for FFI structs

Posted by GitBox <gi...@apache.org>.
viirya closed pull request #1432:
URL: https://github.com/apache/arrow-rs/pull/1432


   


-- 
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-unsubscribe@arrow.apache.org

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

[GitHub] [arrow-rs] sunchao commented on a change in pull request #1432: Replace Arc with Box in ArrowArray for FFI structs

Posted by GitBox <gi...@apache.org>.
sunchao commented on a change in pull request #1432:
URL: https://github.com/apache/arrow-rs/pull/1432#discussion_r826608711



##########
File path: arrow/src/array/array.rs
##########
@@ -629,7 +629,9 @@ pub unsafe fn make_array_from_raw(
     schema: *const ffi::FFI_ArrowSchema,
 ) -> Result<ArrayRef> {
     let array = ffi::ArrowArray::try_from_raw(array, schema)?;
-    let data = ArrayData::try_from(array)?;
+    let data = ArrayData::try_from(&array)?;
+    // Avoid dropping the `Box` pointers and trigger the `release` mechanism.
+    let _ = ffi::ArrowArray::into_raw(array);

Review comment:
       You mean after calling `make_array_from_raw` to obtain `ArrayRef`, the caller is responsible to call `release` on the aliases (i.e., `Arc`) of the input `array` and `schema`, after it has done using the `ArrayRef`? 
   
   BTW what is the `Buffer` you mentioned? I don't see it in the code.




-- 
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-unsubscribe@arrow.apache.org

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

[GitHub] [arrow-rs] viirya commented on a change in pull request #1432: Replace Arc with Box in ArrowArray for FFI structs

Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #1432:
URL: https://github.com/apache/arrow-rs/pull/1432#discussion_r826550474



##########
File path: arrow/src/array/array.rs
##########
@@ -629,7 +629,9 @@ pub unsafe fn make_array_from_raw(
     schema: *const ffi::FFI_ArrowSchema,
 ) -> Result<ArrayRef> {
     let array = ffi::ArrowArray::try_from_raw(array, schema)?;
-    let data = ArrayData::try_from(array)?;
+    let data = ArrayData::try_from(&array)?;
+    // Avoid dropping the `Box` pointers and trigger the `release` mechanism.
+    let _ = ffi::ArrowArray::into_raw(array);

Review comment:
       When previously we try to get an array data from ffi array and schema, we will keep `Arc` pointer of the array in created `Buffer`. That is why we don't need to specially take care about when `array` and `schema` are out of scope. So usually `release` is called when created Buffer is deallocated.
   
   But as we move to `Box`, it is not possible now. As we cannot clone ffi struct too, we cannot let the `release` is called when the created `Buffer` to be deallocated.
   
   




-- 
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-unsubscribe@arrow.apache.org

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

[GitHub] [arrow-rs] wangfenjin commented on a change in pull request #1432: Replace Arc with Box in ArrowArray for FFI structs

Posted by GitBox <gi...@apache.org>.
wangfenjin commented on a change in pull request #1432:
URL: https://github.com/apache/arrow-rs/pull/1432#discussion_r825588216



##########
File path: arrow/src/array/array.rs
##########
@@ -629,7 +629,9 @@ pub unsafe fn make_array_from_raw(
     schema: *const ffi::FFI_ArrowSchema,
 ) -> Result<ArrayRef> {
     let array = ffi::ArrowArray::try_from_raw(array, schema)?;
-    let data = ArrayData::try_from(array)?;
+    let data = ArrayData::try_from(&array)?;
+    // Avoid dropping the `Box` pointers and trigger the `release` mechanism.
+    let _ = ffi::ArrowArray::into_raw(array);

Review comment:
       [arrow2](https://github.com/jorgecarleitao/arrow2/blob/63ca34136da40ecfeef37c40e9a9c7acbd0ef873/src/ffi/array.rs#L416) use a struct to hold the box data, and then use Arc to hold this struct




-- 
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-unsubscribe@arrow.apache.org

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

[GitHub] [arrow-rs] viirya commented on a change in pull request #1432: Replace Arc with Box in ArrowArray for FFI structs

Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #1432:
URL: https://github.com/apache/arrow-rs/pull/1432#discussion_r825483017



##########
File path: arrow/src/array/array.rs
##########
@@ -629,7 +629,9 @@ pub unsafe fn make_array_from_raw(
     schema: *const ffi::FFI_ArrowSchema,
 ) -> Result<ArrayRef> {
     let array = ffi::ArrowArray::try_from_raw(array, schema)?;
-    let data = ArrayData::try_from(array)?;
+    let data = ArrayData::try_from(&array)?;
+    // Avoid dropping the `Box` pointers and trigger the `release` mechanism.
+    let _ = ffi::ArrowArray::into_raw(array);

Review comment:
       Since `ArrayData::try_from()` moves `ffi::ArrowArray`, it will drop the `ArrowArray` and trigger `release`. Users cannot prevent it happened. For example, without this change, our internal usecase got a SIGSEGV.
   
   So I change it to a borrowed reference to avoid dropping/releasing there.
   
   




-- 
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-unsubscribe@arrow.apache.org

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

[GitHub] [arrow-rs] viirya commented on a change in pull request #1432: Replace Arc with Box in ArrowArray for FFI structs

Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #1432:
URL: https://github.com/apache/arrow-rs/pull/1432#discussion_r825582215



##########
File path: arrow/src/array/array.rs
##########
@@ -629,7 +629,9 @@ pub unsafe fn make_array_from_raw(
     schema: *const ffi::FFI_ArrowSchema,
 ) -> Result<ArrayRef> {
     let array = ffi::ArrowArray::try_from_raw(array, schema)?;
-    let data = ArrayData::try_from(array)?;
+    let data = ArrayData::try_from(&array)?;
+    // Avoid dropping the `Box` pointers and trigger the `release` mechanism.
+    let _ = ffi::ArrowArray::into_raw(array);

Review comment:
       Hmm, I'm taking a look how `from(v: Box<T>)` is implemented.
   
   It basically allocates another memory space and copies all bytes from boxed content to new memory. And then free the memory allocation without dropping. If it won't trigger `Drop`, then it might work. I will do some test with our internal project to see if it works there.




-- 
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-unsubscribe@arrow.apache.org

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

[GitHub] [arrow-rs] sunchao commented on a change in pull request #1432: Replace Arc with Box in ArrowArray for FFI structs

Posted by GitBox <gi...@apache.org>.
sunchao commented on a change in pull request #1432:
URL: https://github.com/apache/arrow-rs/pull/1432#discussion_r826506993



##########
File path: arrow/src/array/array.rs
##########
@@ -629,7 +629,9 @@ pub unsafe fn make_array_from_raw(
     schema: *const ffi::FFI_ArrowSchema,
 ) -> Result<ArrayRef> {
     let array = ffi::ArrowArray::try_from_raw(array, schema)?;
-    let data = ArrayData::try_from(array)?;
+    let data = ArrayData::try_from(&array)?;
+    // Avoid dropping the `Box` pointers and trigger the `release` mechanism.
+    let _ = ffi::ArrowArray::into_raw(array);

Review comment:
       I agree it'd be nice if we can retain the previous API. When the `release` on the input `array` and `schema` are supposed to be called previously?




-- 
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-unsubscribe@arrow.apache.org

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

[GitHub] [arrow-rs] viirya commented on a change in pull request #1432: Replace Arc with Box in ArrowArray for FFI structs

Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #1432:
URL: https://github.com/apache/arrow-rs/pull/1432#discussion_r825604428



##########
File path: arrow/src/array/array.rs
##########
@@ -629,7 +629,9 @@ pub unsafe fn make_array_from_raw(
     schema: *const ffi::FFI_ArrowSchema,
 ) -> Result<ArrayRef> {
     let array = ffi::ArrowArray::try_from_raw(array, schema)?;
-    let data = ArrayData::try_from(array)?;
+    let data = ArrayData::try_from(&array)?;
+    // Avoid dropping the `Box` pointers and trigger the `release` mechanism.
+    let _ = ffi::ArrowArray::into_raw(array);

Review comment:
       Hmm, I'm afraid that might also not work.
   
   I got a SIGSEGV in `box_free` when the allocator tries to deallocate the struct allocation.
   
   




-- 
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-unsubscribe@arrow.apache.org

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