ARROW-4130: [Go] offset not used when accessing binary array#3283
ARROW-4130: [Go] offset not used when accessing binary array#3283jpacik wants to merge 4 commits intoapache:masterfrom jpacik:fix/go-binary-slice
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3283 +/- ##
===========================================
- Coverage 88.5% 67.88% -20.63%
===========================================
Files 539 58 -481
Lines 72968 4029 -68939
===========================================
- Hits 64583 2735 -61848
+ Misses 8278 1185 -7093
- Partials 107 109 +2Continue to review full report at Codecov.
|
|
@stuartcarnie can you review this? |
sbinet
left a comment
sbinet
left a comment
There was a problem hiding this comment.
just a first pass.
seems good.
go/arrow/array/binary_test.go
Outdated
There was a problem hiding this comment.
I think reusing the same vs slice is unnecessarily clever.
what about:
vs := make([]string, slice.Len())
for i := range vs {
vs[i] = slice.ValueString(i)
}instead ?
There was a problem hiding this comment.
@sbinet I'd like it if the tests made as few allocations as possible, but if you think this change would increase readability then I have no problem with it. The extra allocations are negligible anyway.
go/arrow/array/binary_test.go
Outdated
There was a problem hiding this comment.
what about:
vs := make([]string, arr.Len())
for i := range vs {
vs[i] = arr.ValueString(i)
}instead of creating this easy-to-mistake-with-1 l value?
go/arrow/array/binary_test.go
Outdated
go/arrow/array/binary_test.go
Outdated
sbinet
left a comment
sbinet
left a comment
There was a problem hiding this comment.
LGTM.
but let's wait for @stuartcarnie and/or @alexandreyc
go/arrow/array/binary_test.go
Outdated
There was a problem hiding this comment.
perhaps store the returned value and display it if tc.panic is true to prevent any unwanted compiler ellision?
|
@stuartcarnie @alexandreyc can you review? |
| func (a *Binary) Value(i int) []byte { | ||
| if i < 0 || i >= a.array.data.length { | ||
| panic("arrow/array: index out of range") | ||
| } |
There was a problem hiding this comment.
Go compilers do it for slices and arrays. It's in the specs of the Go language.
|
LGTM (Sorry for being rather inactive these last weeks, I hope I will have more time to contribute in a few weeks) |
5 participants
Closes #3270 .