Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARROW-14106: [Go][C] Implement Exporting to the C Data Interface #11220

Closed
wants to merge 10 commits into from

Conversation

zeroshade
Copy link
Member

@github-actions
Copy link

go/arrow/cdata/cdata_exports.go Outdated Show resolved Hide resolved
go/arrow/cdata/exports.go Show resolved Hide resolved
go/arrow/cdata/test/test_export_to_cgo.py Outdated Show resolved Hide resolved
assert sc == self.make_schema()

def test_get_batch(self):
with self.assert_pyarrow_memory_released():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there also a way to check that all Go memory was released?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have an "allocator" object in the Go that I could expose a reference to for the tests which would then provide a way to check that the Go memory got freed. I'll see what i can do, but that's a really good point. It also ties into #11206 a bit where I set up a way to directly use and reference the MemoryPool in the C++ from Go

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a CurrentAlloc function to the CheckedAllocator and have everything in the test use that in order to expose a function that allows confirming the Go memory has been released.

@zeroshade
Copy link
Member Author

@pitrou any further comments on this one too?

Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 from me (but keep in mind that I'm not a Go developer!)

@zeroshade
Copy link
Member Author

@pitrou Thanks for the review, and I know you're not a Go developer but it seems i haven't been able to get any other Go developers reliably commenting on these and Go isn't particularly difficult to pick up or understand so I'll take it :)

@asfgit asfgit closed this in 3080d25 Oct 4, 2021
@zeroshade zeroshade deleted the arrow-14106 branch October 5, 2021 15:10
ViniciusSouzaRoque pushed a commit to s1mbi0se/arrow that referenced this pull request Oct 20, 2021
CC @pitrou @emkornfield

Closes apache#11220 from zeroshade/arrow-14106

Lead-authored-by: Matthew Topol <mtopol@factset.com>
Co-authored-by: Matt Topol <mtopol@factset.com>
Signed-off-by: Matthew Topol <mtopol@factset.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants