ARROW-4126: [Go] offset not used when accessing boolean array#3275
ARROW-4126: [Go] offset not used when accessing boolean array#3275jpacik wants to merge 3 commits intoapache:masterfrom jpacik:fix/go-boolean-slice
Conversation
|
@stuartcarnie can you review this? |
|
just a quick first pass, but seems good. |
Codecov Report
@@ Coverage Diff @@
## master #3275 +/- ##
===========================================
- Coverage 88.5% 68.14% -20.37%
===========================================
Files 539 58 -481
Lines 72968 4015 -68953
===========================================
- Hits 64583 2736 -61847
+ Misses 8278 1176 -7102
+ Partials 107 103 -4
Continue to review full report at Codecov.
|
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/boolean_test.go
Outdated
There was a problem hiding this comment.
perhaps store the value retuned by Value(int) and display it if tc.panic is true?
this way we make sure the compiler won't elide this piece of code?
stuartcarnie
left a comment
stuartcarnie
left a comment
There was a problem hiding this comment.
Just a minor nit, but otherwise looks good
|
|
||
| func (a *Boolean) Value(i int) bool { return bitutil.BitIsSet(a.values, i) } | ||
| func (a *Boolean) Value(i int) bool { | ||
| if i < 0 || i >= a.array.data.length { |
There was a problem hiding this comment.
BitIsSet is going to panic if the index is out of bounds, so there is no need to perform this check here
There was a problem hiding this comment.
@stuartcarnie I don't think this is always true. For instance an array of 10 boolean values is stored as a byte array of length 2. The first 8 boolean values are represented in the first byte and the last 2 boolean values are represented in the second byte. Here BitIsSet will only panic if the given index falls outside of second byte, which means an index of 16 or higher.
There was a problem hiding this comment.
We've been focused on a performance-first approach with similar semantics to the C++ implementation and not performing bounds checks similar APIs, however, I think it is reasonable in this case, to avoid undefined behavior. We may wish to formalize where we make these tradeoffs.
4 participants
Closes #3273 .