ARROW-4081: [Go] Sum methods panic when the array is empty #3906
Conversation
|
@alexandreyc it looks like you're developing out of your master branch and so some unrelated changes got exposed here |
|
maybe make this PR the fix for ARROW-4081? It's fine to combine those stylistic changes with this I think |
Codecov Report
@@ Coverage Diff @@
## master #3906 +/- ##
===========================================
- Coverage 87.84% 68.86% -18.98%
===========================================
Files 726 60 -666
Lines 89340 4134 -85206
Branches 1252 0 -1252
===========================================
- Hits 78480 2847 -75633
+ Misses 10742 1182 -9560
+ Partials 118 105 -13
Continue to review full report at Codecov.
|
|
Yes ok, we will use this PR for ARROW-4081. I just need a bit more work to be sure for the fix. I will do it pretty soon. |
wesm
changed the title
alexandreyc
changed the title
|
The issue is not specific to macOS since I've been able to reproduce it on Linux. When reading the code it's clear why we panic when an empty array is passed: we take a pointer on the first element of the array to pass to low-level optimized function. See for instance: arrow/go/arrow/math/float64_avx2_amd64.go Line 35 in d90c159 What do you think about this fix? |
go/arrow/math/type_test.go.tmpl
Outdated
| func Test{{.Name}}Funcs_SumEmpty(t *testing.T) { | ||
| b := array.New{{.Name}}Builder(memory.NewGoAllocator()) | ||
| vec := b.New{{.Name}}Array() | ||
| b.Release() |
There was a problem hiding this comment.
I'd put it at line 38 as a defer b.Release() instead.
also: vec is leaked. we should probably have a defer vec.Release() somewhere as well.
perhaps we should use a CheckedAllocator:
mem := memory.NewCheckedAllocator(memory.NewGoAllocator())
defer mem.AssertSize(t, 0)
|
Should be good now. |
sbinet
changed the title
Nothing fundamental but more idiomatic and coherent.