zeroshade commented on code in PR #645:
URL: https://github.com/apache/arrow-go/pull/645#discussion_r2710121574
##########
arrow/schema.go:
##########
@@ -194,11 +193,10 @@ func (sc *Schema) WithEndianness(e endian.Endianness)
*Schema {
func (sc *Schema) Endianness() endian.Endianness { return sc.endianness }
func (sc *Schema) IsNativeEndian() bool { return sc.endianness ==
endian.NativeEndian }
func (sc *Schema) Metadata() Metadata { return sc.meta }
-func (sc *Schema) Fields() []Field {
- fields := make([]Field, len(sc.fields))
- copy(fields, sc.fields)
- return fields
-}
+
+// Fields returns the fields of the schema.
+// The result is not a clone of the schema's fields and therefore should not
be modified by the caller.
+func (sc *Schema) Fields() []Field { return sc.fields }
Review Comment:
A while back we explicitly converted this into being a clone operation.
Instead of changing this, can we introduce a function that returns
`iter.Seq[Field]`? that way we can avoid the slice copy while still preventing
a user from being able to modify the slice.
##########
arrow/schema.go:
##########
@@ -207,11 +205,17 @@ func (sc *Schema) FieldsByName(n string) ([]Field, bool) {
if !ok {
return nil, ok
}
- fields := make([]Field, 0, len(indices))
- for _, v := range indices {
- fields = append(fields, sc.fields[v])
+ if len(indices) == 1 {
+ return sc.fields[indices[0] : indices[0]+1], ok
+ } else if len(indices) > 1 {
+ fields := make([]Field, 0, len(indices))
+ for _, v := range indices {
+ fields = append(fields, sc.fields[v])
+ }
+ return fields, ok
+ } else {
+ return nil, false
}
Review Comment:
```suggestion
}
return nil, false
}
```
##########
arrow/schema.go:
##########
@@ -170,18 +170,17 @@ func NewSchema(fields []Field, metadata *Metadata)
*Schema {
func NewSchemaWithEndian(fields []Field, metadata *Metadata, e
endian.Endianness) *Schema {
sc := &Schema{
- fields: make([]Field, 0, len(fields)),
+ fields: fields,
index: make(map[string][]int, len(fields)),
endianness: e,
}
if metadata != nil {
- sc.meta = metadata.clone()
+ sc.meta = *metadata
Review Comment:
same as above, since the members of the `Metadata` object are slices
(pointer types) just copying the metadata struct like this means that the
caller could potentially modify the metadata after constructing the schema.
This is less concerning than possibly modifying the fields slice so I'm more
inclined to not see this as an issue here.
--
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]
