You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "rtadepalli (via GitHub)" <gi...@apache.org> on 2023/03/04 01:42:21 UTC
[GitHub] [arrow] rtadepalli opened a new pull request, #34450: GH-33999: [Go] Removed cast from byte[] to string and copied entire string when is called
rtadepalli opened a new pull request, #34450:
URL: https://github.com/apache/arrow/pull/34450
### 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.Pointer` and 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] zeroshade commented on a diff in pull request #34450: GH-33999: [Go] Removed cast from byte[] to string and copied entire string when `Value()` is called
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #34450:
URL: https://github.com/apache/arrow/pull/34450#discussion_r1126671392
##########
go/arrow/array/string.go:
##########
@@ -50,8 +50,11 @@ func (a *String) Reset(data arrow.ArrayData) {
// Value returns the slice at index i. This value should not be mutated.
func (a *String) Value(i int) string {
+ 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()
Review Comment:
There's no need to use the string builder here, you can just do `string(a.values[a.offsets[i]:a.offsets[i+1]])`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #34450: GH-33999: [Go] Removed cast from byte[] to string and copied entire string when `Value()` is called
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34450:
URL: https://github.com/apache/arrow/pull/34450#issuecomment-1454328721
* Closes: #33999
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33999: [Go] Removed cast from byte[] to string and copied entire string when `Value()` is called [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34450:
URL: https://github.com/apache/arrow/pull/34450#issuecomment-2319658112
:warning: GitHub issue #33999 **has no components**, please add labels for components.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] zeroshade commented on a diff in pull request #34450: GH-33999: [Go] Removed cast from byte[] to string and copied entire string when `Value()` is called
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #34450:
URL: https://github.com/apache/arrow/pull/34450#discussion_r1126689841
##########
go/arrow/array/string.go:
##########
@@ -50,8 +50,11 @@ func (a *String) Reset(data arrow.ArrayData) {
// Value returns the slice at index i. This value should not be mutated.
func (a *String) Value(i int) string {
+ 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()
Review 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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] zeroshade commented on a diff in pull request #34450: GH-33999: [Go] Removed cast from byte[] to string and copied entire string when `Value()` is called
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #34450:
URL: https://github.com/apache/arrow/pull/34450#discussion_r1126673992
##########
go/arrow/array/string.go:
##########
@@ -246,7 +249,7 @@ func (a *LargeString) setData(data *Data) {
if vdata := data.buffers[2]; vdata != nil {
b := vdata.Bytes()
- a.values = *(*string)(unsafe.Pointer(&b))
+ a.values = string(b[:])
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #34450: GH-33999: [Go] Removed cast from byte[] to string and copied entire string when `Value()` is called
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34450:
URL: https://github.com/apache/arrow/pull/34450#issuecomment-1454328730
:warning: GitHub issue #33999 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] amol- commented on pull request #34450: GH-33999: [Go] Removed cast from byte[] to string and copied entire string when `Value()` is called
Posted by "amol- (via GitHub)" <gi...@apache.org>.
amol- commented on PR #34450:
URL: https://github.com/apache/arrow/pull/34450#issuecomment-1490667298
Closing because it has been untouched for a while, in case it's still relevant feel free to reopen and move it forward 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] zeroshade commented on a diff in pull request #34450: GH-33999: [Go] Removed cast from byte[] to string and copied entire string when `Value()` is called
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #34450:
URL: https://github.com/apache/arrow/pull/34450#discussion_r1126671392
##########
go/arrow/array/string.go:
##########
@@ -50,8 +50,11 @@ func (a *String) Reset(data arrow.ArrayData) {
// Value returns the slice at index i. This value should not be mutated.
func (a *String) Value(i int) string {
+ 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()
Review 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:
```go
val := a.values[a.offsets[i]:a.offsets[i+1]]
out := make([]byte, len(val))
copy(out, val)
return *(*string)(unsafe.Pointer(&out))
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] amol- closed pull request #34450: GH-33999: [Go] Removed cast from byte[] to string and copied entire string when `Value()` is called
Posted by "amol- (via GitHub)" <gi...@apache.org>.
amol- closed pull request #34450: GH-33999: [Go] Removed cast from byte[] to string and copied entire string when `Value()` is called
URL: https://github.com/apache/arrow/pull/34450
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org