GH-33999: [Go] Removed cast from byte[] to string and copied entire string when Value() is called
#34450
Conversation
rtadepalli
changed the title
Value() is called
|
|
| sb := strings.Builder{} | ||
|
|
||
| i = i + a.array.data.offset | ||
| return a.values[a.offsets[i]:a.offsets[i+1]] | ||
| sb.WriteString(a.values[a.offsets[i]:a.offsets[i+1]]) | ||
| return sb.String() |
There was a problem hiding this comment.
There's no need to use the string builder here, you can just do string([]byte(a.values[a.offsets[i]:a.offsets[i+1]])[:]) or something like:
val := a.values[a.offsets[i]:a.offsets[i+1]]
out := make([]byte, len(val))
copy(out, val)
return *(*string)(unsafe.Pointer(&out))There was a problem hiding this comment.
Ideally we'd still maintain a method that would let someone retrieve the slice referencing the data to allow consumers to avoid the copy if desired (such as the GetView in the C++ lib)
github-actions
bot
added
awaiting committer review
| if vdata := data.buffers[2]; vdata != nil { | ||
| b := vdata.Bytes() | ||
| a.values = *(*string)(unsafe.Pointer(&b)) | ||
| a.values = string(b[:]) |
There was a problem hiding this comment.
we shouldn't copy the entire buffer here. Keep the unsafe.Pointer here so we only perform copies in the Value method and only on the values that are requested.
|
Closing because it has been untouched for a while, in case it's still relevant feel free to reopen and move it forward 👍 |
|
|
Rationale for this change
Remove some unsafe casts and copy a string instead of returning a reference to an underlying array.
What changes are included in this PR?
Remove
unsafe.Pointerand add a string deep copy.Are these changes tested?
I think existing tests should suffice.
Are there any user-facing changes?
There are no user facing/breaking changes.