zeroshade commented on code in PR #570:
URL: https://github.com/apache/arrow-go/pull/570#discussion_r3876193537
##########
arrow/ipc/metadata.go:
##########
@@ -183,7 +185,8 @@ func fieldFromFB(field *flatbuf.Field, pos
dictutils.FieldPos, memo *dictutils.M
o arrow.Field
)
- o.Name = string(field.Name())
+ name := field.Name()
+ o.Name = unsafe.String(&name[0], len(name))
Review Comment:
**Blocking:** Arrow permits empty field names, so `&name[0]` panics when
`len(name) == 0`. More broadly, `unsafe.String` makes the decoded schema borrow
FlatBuffer storage; with `NewMappedFileReader`, that storage can be
caller-owned or memory-mapped, and mutating it changes the already-decoded
field name. Please retain the owned conversion:
```go
o.Name = string(field.Name())
```
`unsafe.SliceData` would only address empty input, not ownership.
##########
arrow/ipc/metadata.go:
##########
@@ -986,8 +989,9 @@ func timeFromFB(data flatbuf.Time) (arrow.DataType, error) {
func timestampFromFB(data flatbuf.Timestamp) (arrow.DataType, error) {
unit := unitFromFB(data.Unit())
- tz := string(data.Timezone())
- return &arrow.TimestampType{Unit: unit, TimeZone: tz}, nil
+ tz := data.Timezone()
+ tzs := unsafe.String(&tz[0], len(tz))
Review Comment:
**Blocking:** Unzoned Arrow timestamps have an empty timezone, so this
panics on `&tz[0]`. The resulting string also borrows FlatBuffer storage.
Please retain an owned conversion:
```go
tz := string(data.Timezone())
return &arrow.TimestampType{Unit: unit, TimeZone: tz}, nil
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
