fix: Arrow Retain and Release fixes #795
Conversation
|
Ah wait a minute, I was using an outdated base branch 😮💨 Let me double-check this |
| pks[key] = struct{}{} | ||
| res = append(res, r) | ||
| continue | ||
| case reported: |
There was a problem hiding this comment.
the reported variable was not being used
⏱️ Benchmark resultsComparing with 591502f
|
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## main #795 +/- ##
=======================================
Coverage 46.71% 46.71%
=======================================
Files 76 76
Lines 7832 7832
=======================================
Hits 3659 3659
Misses 3682 3682
Partials 491 491 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
hermanschaaf
changed the title
| var reported bool | ||
| for _, r := range resources { | ||
| if r.NumRows() > 1 { | ||
| panic(fmt.Sprintf("record with more than 1 row: %d", r.NumRows())) |
There was a problem hiding this comment.
I also added this panic because this function's logic will break down if a record ever contains more than one row.
I wonder if we may want to add a special type that guarantees we're dealing with a arrow.Record with only a single row.
yevgenypats
left a comment
yevgenypats
left a comment
There was a problem hiding this comment.
Had another counter comment :)
| MaxRows: 1, | ||
| } | ||
| record1 := testdata.GenTestData(mem, table.ToArrowSchema(), opts)[0] | ||
| record1.Retain() |
There was a problem hiding this comment.
Why this is removed? I specifically called Retain here because unlike in the server we do use this after call to writeOne or writeAll for compare/testing purposes. This is also why I called defer record1.Release afterwards.
There was a problem hiding this comment.
This is basically just a refactor: no change in functionality. Rather than have many retains, I think it's better to have it only in writeAll (and by extension writeOne), where it's clear that the resource is being sent over a channel. Here, just by looking at the call to writeOne, you can't really know whether it's going to be sent over a channel or not.
The record is still available for comparison until the end of this function because the release is deferred.
There was a problem hiding this comment.
I think it is crucial for it to be here. I can explain :) maybe let's sync tmrw morning.
|
@yevgenypats Thanks for catching up with me earlier; I've now updated it as we discussed. The TL;DR is that we'll keep the extra |
🤖 I have created a release *beep* *boop* --- ## [2.3.2](v2.3.1...v2.3.2) (2023-04-19) ### Bug Fixes * Arrow Retain and Release fixes ([#795](#795)) ([a893db6](a893db6)) * Disallow null character in strings per utf8 spec ([#797](#797)) ([591502f](591502f)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
This is mainly to fix the memory leak fixed in the plugin-sdk update cloudquery/plugin-sdk#795
This fixes a memory leak detected by the
fileands3destination tests, and refactors the retain/release logic a little bit.Quoting the Go Arrow docs:
I think we should call
Retainas close as possible to where a record is sent over a channel.