pitrou commented on code in PR #38314:
URL: https://github.com/apache/arrow/pull/38314#discussion_r1363535403
##########
go/arrow/memory/buffer.go:
##########
@@ -33,6 +33,10 @@ type Buffer struct {
parent *Buffer
}
+func NewBufferWithAllocator(data []byte, mem Allocator) *Buffer {
Review Comment:
Should this be exposed here or only in `importAllocator.go`? It doesn't seem
like a good idea to use this API with allocators other than `importAllocator`
##########
go/arrow/cdata/exports.go:
##########
@@ -59,12 +58,7 @@ func releaseExportedSchema(schema *CArrowSchema) {
C.free(unsafe.Pointer(schema.dictionary))
}
- var children []*CArrowSchema
- s := (*reflect.SliceHeader)(unsafe.Pointer(&children))
- s.Data = uintptr(unsafe.Pointer(schema.children))
- s.Len = int(schema.n_children)
- s.Cap = int(schema.n_children)
-
+ children := unsafe.Slice(schema.children, schema.n_children)
Review Comment:
Nice :-)
##########
go/arrow/cdata/cdata.go:
##########
@@ -418,26 +409,41 @@ func (imp *cimporter) doImportChildren() error {
func (imp *cimporter) initarr() {
imp.arr = C.get_arr()
+ if imp.alloc == nil {
+ imp.alloc = &importAllocator{arr: imp.arr}
+ }
+}
+
+func (imp *cimporter) doImportArr(src *CArrowArray) error {
+ imp.arr = C.get_arr()
+ C.ArrowArrayMove(src, imp.arr)
+ if imp.alloc == nil {
+ imp.alloc = &importAllocator{arr: imp.arr}
+ }
+
+ // we tie the releasing of the array to when the buffers are
+ // cleaned up, so if there are no buffers that we've imported
+ // such as for a null array or a nested array with no bitmap
+ // and only null columns, then we can release the CArrowArray
+ // struct immediately after import, since we have no imported
+ // memory that we have to track the lifetime of.
+ defer func() {
+ if imp.alloc.bufCount == 0 {
+ C.ArrowArrayRelease(imp.arr)
+ }
+ }()
+
+ return imp.doImport()
}
// import is called recursively as needed for importing an array and its
children
// in order to generate array.Data objects
-func (imp *cimporter) doImport(src *CArrowArray) error {
- imp.initarr()
+func (imp *cimporter) doImport() error {
// move the array from the src object passed in to the one referenced by
// this importer. That way we can set up a finalizer on the created
// arrow.ArrayData object so we clean up our Array's memory when
garbage collected.
- C.ArrowArrayMove(src, imp.arr)
defer func(arr *CArrowArray) {
- if imp.data != nil {
- runtime.SetFinalizer(imp.data, func(arrow.ArrayData) {
- defer C.free(unsafe.Pointer(arr))
- C.ArrowArrayRelease(arr)
- if C.ArrowArrayIsReleased(arr) != 1 {
- panic("did not release C mem")
- }
- })
- } else {
+ if imp.data == nil {
Review Comment:
This only happens on error, right? Might add a comment about that.
##########
go/arrow/cdata/cdata_fulltest.c:
##########
@@ -27,8 +27,27 @@
#include "arrow/c/helpers.h"
#include "utils.h"
+int is_little_endian()
+{
+ unsigned int x = 1;
+ char *c = (char*) &x;
+ return (int)*c;
+}
+
static const int64_t kDefaultFlags = ARROW_FLAG_NULLABLE;
+extern void releaseTestArr(struct ArrowArray* array);
+void goReleaseTestArray(struct ArrowArray* array) {
+ releaseTestArr(array);
+}
+
+void release_test_arr(struct ArrowArray* arr) {
Review Comment:
Hmm, can we avoid having both `release_test_arr` and `releaseTestArr`? It's
confusing which is what, especially as this one isn't `static` (is it called
somewhere else)?
--
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]
