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 2021/07/22 12:26:39 UTC
[GitHub] [arrow-rs] roee88 opened a new issue #595: FFI implementation deviates from specification for array release
roee88 opened a new issue #595:
URL: https://github.com/apache/arrow-rs/issues/595
The FFI implementation deviates from the C Data Interface specification in [Release callback semantics – for producers](https://arrow.apache.org/docs/format/CDataInterface.html#release-callback-semantics-for-producers).
> The release callback MUST walk all children structures (including the optional dictionary) and call their own release callbacks.
The current implementation doesn't walk through the direct children but rather only through the children in private data. This blocks support for moving child arrays.
It also assumes that child arrays are not already released. This assumption is not true for the case of child array move. The release member must be checked to be not null before dropping/releasing an array.
The current implementation doesn't seem to release the dictionary array.
Please see the reference implementation in the spec:
```c
static void ReleaseExportedArray(struct ArrowArray* array) {
// This should not be called on already released array
assert(array->format != NULL);
// Release children
for (int64_t i = 0; i < array->n_children; ++i) {
struct ArrowArray* child = array->children[i];
if (child->release != NULL) {
child->release(child);
assert(child->release == NULL);
}
}
// Release dictionary
struct ArrowArray* dict = array->dictionary;
if (dict != NULL && dict->release != NULL) {
dict->release(dict);
assert(dict->release == NULL);
}
// TODO here: release and/or deallocate all data directly owned by
// the ArrowArray struct, such as the private_data.
// Mark array released
array->release = NULL;
}
```
--
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] roee88 commented on issue #595: FFI implementation deviates from specification for array release
Posted by GitBox <gi...@apache.org>.
roee88 commented on issue #595:
URL: https://github.com/apache/arrow-rs/issues/595#issuecomment-886084706
On a second thought I'm not sure about this one. The rust implementation has an assumption that apart from nullifying the release callback everything is immutable. This might be a reasonable assumption even though the cpp implementation doesn't do the same and (at least to me) the spec aren't clear enough here. In contrast to the original description, the current release implementation doesn't block the possibility of (child) array move. I will verify this use case via a test case separately (e.g., to ensure that buffer lifetime is correct).
--
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] pitrou commented on issue #595: FFI implementation deviates from specification for array release
Posted by GitBox <gi...@apache.org>.
pitrou commented on issue #595:
URL: https://github.com/apache/arrow-rs/issues/595#issuecomment-908330515
@jorgecarleitao Sorry for the large delay, is there something you'd like me to take a look at?
--
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] pitrou commented on issue #595: FFI implementation deviates from specification for array release
Posted by GitBox <gi...@apache.org>.
pitrou commented on issue #595:
URL: https://github.com/apache/arrow-rs/issues/595#issuecomment-908330515
@jorgecarleitao Sorry for the large delay, is there something you'd like me to take a look at?
--
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] roee88 closed issue #595: FFI implementation deviates from specification for array release
Posted by GitBox <gi...@apache.org>.
roee88 closed issue #595:
URL: https://github.com/apache/arrow-rs/issues/595
--
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] kszucs commented on issue #595: FFI implementation deviates from specification for array release
Posted by GitBox <gi...@apache.org>.
kszucs commented on issue #595:
URL: https://github.com/apache/arrow-rs/issues/595#issuecomment-885277902
If I recall correctly metadata and dictionary has not been implemented so far, though we should definitely support those.
The `with_name` and `with_flags` methods are used as [setters here](https://github.com/apache/arrow-rs/blob/9f40f899e439d072fc859e0b4abf46776387e0d1/arrow/src/datatypes/ffi.rs#L240-L241) (perhaps we should use a set_ prefix instead). These are separate fields in the c-struct from the private_data field.
Regarding the ArrowArray abstraction I'd need to check the implementation again (which we should clean up eventually), but I'm currently busy with the release procedure. Hopefully I can have a more thorough look at the issue after that.
--
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] jorgecarleitao commented on issue #595: FFI implementation deviates from specification for array release
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on issue #595:
URL: https://github.com/apache/arrow-rs/issues/595#issuecomment-886085244
cc @pitrou , that may help us further here.
--
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] roee88 commented on issue #595: FFI implementation deviates from specification for array release
Posted by GitBox <gi...@apache.org>.
roee88 commented on issue #595:
URL: https://github.com/apache/arrow-rs/issues/595#issuecomment-885237131
The same is true for schema release. As part of a fix, the allocated format, name, (and in the future: metadata, dictionary) should also go into private data. However, currently the code has a [with_name](https://github.com/apache/arrow-rs/blob/9f40f899e439d072fc859e0b4abf46776387e0d1/arrow/src/ffi.rs#L177) function that would probably need to go away because `private_data` is already a raw pointer at that point. @kszucs what's the rational behind these `with_*` methods and could you suggest a clean alternative given the above?
--
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] roee88 commented on issue #595: FFI implementation deviates from specification for array release
Posted by GitBox <gi...@apache.org>.
roee88 commented on issue #595:
URL: https://github.com/apache/arrow-rs/issues/595#issuecomment-912038074
@pitrou the question is whether it's safe to assume that other than nullifying the release callback the consumer doesn't change anything in the structures.
Two examples to demonstrate the difference in implementation:
1. In C++ the allocated format field is released based on a pointer stored in private data. In Rust it's released based on the format field itself (in the ArrowSchema structure).
2. In C++ the release callback of every child in the children field is called. In Rust the release callback of every child in private data is called.
--
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] roee88 commented on issue #595: FFI implementation deviates from specification for array release
Posted by GitBox <gi...@apache.org>.
roee88 commented on issue #595:
URL: https://github.com/apache/arrow-rs/issues/595#issuecomment-885381533
> The with_name and with_flags methods are used as setters here (perhaps we should use a set_ prefix instead). These are separate fields in the c-struct from the private_data field.
My point was that own allocations (like name) must go to private data, not sure if that was expressed clearly in my message. I'll create a draft PR to make it clearer. I can help contribute here but will probably need your guidelines for the with_name setters.
> Regarding the ArrowArray abstraction I'd need to check the implementation again (which we should clean up eventually), but I'm currently busy with the release procedure. Hopefully I can have a more thorough look at the issue after that.
In the code and text above whenever I wrote ArrowArray I meant FFI_ArrowArray, I just used the spec name sorry for the confusion. I do agree that the ArrowArray abstraction is confusing and probably need to go away. The API should probably be similar to the bridge implementation in arrow cpp. This is more related to https://github.com/apache/arrow-rs/issues/596.
--
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] pitrou commented on issue #595: FFI implementation deviates from specification for array release
Posted by GitBox <gi...@apache.org>.
pitrou commented on issue #595:
URL: https://github.com/apache/arrow-rs/issues/595#issuecomment-912464498
> @pitrou the question is whether it's safe to assume that other than nullifying the release callback the consumer doesn't change anything in the structures.
It is, indeed.
> In C++ the allocated format field is released based on a pointer stored in private data. In Rust it's released based on the format field itself (in the ArrowSchema structure).
I'm not sure what difference it makes. In both cases it is released (directly or indirectly) by calling the release callback, no?
--
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