GH-35711: [Go] Add Value and GetValueIndex methods to some builders
#35744
Conversation
|
|
zeroshade
left a comment
There was a problem hiding this comment.
@metalmatze the test failure isn't related to this PR, but could you please rebase to resolve the conflict?
Otherwise this LGTM and I'll merge it once the conflict is resolved.
go/arrow/array/dictionary.go
Outdated
There was a problem hiding this comment.
Just now, I saw that this actually allocates quite a lot of bytes...
For now, I have moved our code to use the Value(i int) []byte method above and then using bytes.Equal to compared our to bytes converted string with what we read from this builder.
Is there a better way you use to convert these bytes to strings in the arrow project? I'm thinking of the yoloString Prometheus has.
There was a problem hiding this comment.
return *(*string)(unsafe.Pointer(&b)) will return the string without copying the bytes. 😄
59e44b8 to
571a42c
Compare
|
Rebased. 🙂 |
|
Benchmark runs are scheduled for baseline = 422174e and contender = 762bd2a. 762bd2a is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
### Rationale for this change Similar to the changes we've made in #35744 we need this for the BooleanBuilder too. ### What changes are included in this PR? Adding the method itself and adjusting a test to test both the Value method on the builder and the final array. ### Are these changes tested? Yes ### Are there any user-facing changes? Just an enhancement to get on-par with other builders. * Closes: #37465 Authored-by: Matthias Loibl <mail@matthiasloibl.com> Signed-off-by: Matt Topol <zotthewizard@gmail.com>
### Rationale for this change Similar to the changes we've made in apache#35744 we need this for the BooleanBuilder too. ### What changes are included in this PR? Adding the method itself and adjusting a test to test both the Value method on the builder and the final array. ### Are these changes tested? Yes ### Are there any user-facing changes? Just an enhancement to get on-par with other builders. * Closes: apache#37465 Authored-by: Matthias Loibl <mail@matthiasloibl.com> Signed-off-by: Matt Topol <zotthewizard@gmail.com>
### Rationale for this change Similar to the changes we've made in apache#35744 we need this for the BooleanBuilder too. ### What changes are included in this PR? Adding the method itself and adjusting a test to test both the Value method on the builder and the final array. ### Are these changes tested? Yes ### Are there any user-facing changes? Just an enhancement to get on-par with other builders. * Closes: apache#37465 Authored-by: Matthias Loibl <mail@matthiasloibl.com> Signed-off-by: Matt Topol <zotthewizard@gmail.com>
Rationale for this change
See #35711
What changes are included in this PR?
Adding
*BinaryDictionaryBuilder.GetValueIndex(i int) intto get the value's index in the dictionary. Then two methods to get a[]byteandstringresults.Value(i int) []byteandValueStr(i int) string.Are these changes tested?
So far I have added very little testing. Mostly some sort of integration test to read back values from the BinaryDictionaryBuilder. We should definitely add some test for the
*BinaryMemoTable) Value(i int) []byte.Are there any user-facing changes?
Just additions, no breaking API changes.
cc @zeroshade