Copilot commented on code in PR #771: URL: https://github.com/apache/arrow-go/pull/771#discussion_r3114218064
########## arrow/array/arreflect/reflect_arrow_to_go.go: ########## @@ -0,0 +1,430 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package arreflect + +import ( + "fmt" + "reflect" + "time" + + "github.com/apache/arrow-go/v18/arrow" + "github.com/apache/arrow-go/v18/arrow/array" +) + +func assertArray[T any](arr arrow.Array) (*T, error) { + a, ok := any(arr).(*T) + if !ok { + var zero T + return nil, fmt.Errorf("expected *%T, got %T: %w", zero, arr, ErrTypeMismatch) + } + return a, nil +} + +func isIntKind(k reflect.Kind) bool { + return k == reflect.Int || k == reflect.Int8 || k == reflect.Int16 || + k == reflect.Int32 || k == reflect.Int64 +} + +func isUintKind(k reflect.Kind) bool { + return k == reflect.Uint || k == reflect.Uint8 || k == reflect.Uint16 || + k == reflect.Uint32 || k == reflect.Uint64 || k == reflect.Uintptr +} + +func isFloatKind(k reflect.Kind) bool { return k == reflect.Float32 || k == reflect.Float64 } + +func setValue(v reflect.Value, arr arrow.Array, i int) error { + if arr.IsNull(i) { + v.Set(reflect.Zero(v.Type())) + return nil + } + if v.Kind() == reflect.Ptr { + v.Set(reflect.New(v.Type().Elem())) + v = v.Elem() + } + + switch arr.DataType().ID() { + case arrow.BOOL: + a, err := assertArray[array.Boolean](arr) + if err != nil { + return err + } + if v.Kind() != reflect.Bool { + return fmt.Errorf("cannot set bool into %s: %w", v.Type(), ErrTypeMismatch) + } + v.SetBool(a.Value(i)) + + case arrow.INT8, arrow.INT16, arrow.INT32, arrow.INT64, + arrow.UINT8, arrow.UINT16, arrow.UINT32, arrow.UINT64, + arrow.FLOAT32, arrow.FLOAT64: + return setPrimitiveValue(v, arr, i) + + case arrow.STRING, arrow.LARGE_STRING: + type stringer interface{ Value(int) string } + a, ok := arr.(stringer) + if !ok { + return fmt.Errorf("expected string array, got %T: %w", arr, ErrTypeMismatch) + } + if v.Kind() != reflect.String { + return fmt.Errorf("cannot set string into %s: %w", v.Type(), ErrTypeMismatch) + } + v.SetString(a.Value(i)) + Review Comment: String values are assigned directly from the Arrow array buffer. Arrow string arrays typically return views into the array’s backing buffer, so the resulting Go string can become invalid/corrupted after the Arrow array is released. Consider copying/cloning the string value before setting it into the destination. ########## arrow/array/arreflect/reflect.go: ########## @@ -0,0 +1,542 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package arreflect + +import ( + "errors" + "fmt" + "reflect" + "sort" + "strconv" + "strings" + "sync" + + "github.com/apache/arrow-go/v18/arrow" + "github.com/apache/arrow-go/v18/arrow/array" + "github.com/apache/arrow-go/v18/arrow/memory" +) + +var ( + ErrUnsupportedType = errors.New("arreflect: unsupported type") + ErrTypeMismatch = errors.New("arreflect: type mismatch") +) + +type tagOpts struct { + Name string + Skip bool + Dict bool + ListView bool + REE bool + DecimalPrecision int32 + DecimalScale int32 + HasDecimalOpts bool + Temporal string // "timestamp" (default), "date32", "date64", "time32", "time64" +} + +type fieldMeta struct { + Name string + Index []int + Type reflect.Type + Nullable bool + Opts tagOpts +} + +func parseTag(tag string) tagOpts { + if tag == "-" { + return tagOpts{Skip: true} + } + + var name, rest string + if idx := strings.Index(tag, ","); idx >= 0 { + name = tag[:idx] + rest = tag[idx+1:] + } else { + name = tag + rest = "" + } + + opts := tagOpts{Name: name} + + if rest == "" { + return opts + } + + parseOptions(&opts, rest) + return opts +} + +func splitTagTokens(rest string) []string { + var tokens []string + depth := 0 + start := 0 + for i := 0; i < len(rest); i++ { + switch rest[i] { + case '(': + depth++ + case ')': + depth-- + case ',': + if depth == 0 { + tokens = append(tokens, strings.TrimSpace(rest[start:i])) + start = i + 1 + } + } + } + if start < len(rest) { + tokens = append(tokens, strings.TrimSpace(rest[start:])) + } + return tokens +} + +func parseOptions(opts *tagOpts, rest string) { + for _, token := range splitTagTokens(rest) { + if strings.HasPrefix(token, "decimal(") && strings.HasSuffix(token, ")") { + parseDecimalOpt(opts, token) + continue + } + switch token { + case "dict": + opts.Dict = true + case "listview": + opts.ListView = true + case "ree": + opts.REE = true + case "date32", "date64", "time32", "time64", "timestamp": + opts.Temporal = token + } + } +} + +func parseDecimalOpt(opts *tagOpts, token string) { + inner := strings.TrimPrefix(token, "decimal(") + inner = strings.TrimSuffix(inner, ")") + parts := strings.SplitN(inner, ",", 2) + if len(parts) == 2 { + p, errP := strconv.ParseInt(strings.TrimSpace(parts[0]), 10, 32) + s, errS := strconv.ParseInt(strings.TrimSpace(parts[1]), 10, 32) + if errP == nil && errS == nil { + opts.HasDecimalOpts = true + opts.DecimalPrecision = int32(p) + opts.DecimalScale = int32(s) + } + } +} + +type bfsEntry struct { + t reflect.Type + index []int + depth int +} + +type candidate struct { + meta fieldMeta + depth int + tagged bool + order int +} + +type resolvedField struct { + meta fieldMeta + order int +} + +func collectFieldCandidates(t reflect.Type) map[string][]candidate { + nameMap := make(map[string][]candidate) + orderCounter := 0 + + queue := []bfsEntry{{t: t, index: nil, depth: 0}} + visited := make(map[reflect.Type]bool) + + for len(queue) > 0 { + entry := queue[0] + queue = queue[1:] + + st := entry.t + for st.Kind() == reflect.Ptr { + st = st.Elem() + } + if st.Kind() != reflect.Struct { + continue + } + + if visited[st] { + continue + } + if entry.depth > 0 { + visited[st] = true + } + + for i := 0; i < st.NumField(); i++ { + sf := st.Field(i) + + fullIndex := make([]int, len(entry.index)+1) + copy(fullIndex, entry.index) + fullIndex[len(entry.index)] = i + + if !sf.IsExported() && !sf.Anonymous { + continue + } + + tagVal, hasTag := sf.Tag.Lookup("arrow") + var opts tagOpts + if hasTag { + opts = parseTag(tagVal) + } + + if opts.Skip { + continue + } + + arrowName := opts.Name + if arrowName == "" { + arrowName = sf.Name + } + + if sf.Anonymous && !hasTag { + ft := sf.Type + for ft.Kind() == reflect.Ptr { + ft = ft.Elem() + } + if ft.Kind() == reflect.Struct { + queue = append(queue, bfsEntry{ + t: ft, + index: fullIndex, + depth: entry.depth + 1, + }) + continue + } + } + + nullable := sf.Type.Kind() == reflect.Ptr + tagged := hasTag && opts.Name != "" + + meta := fieldMeta{ + Name: arrowName, + Index: fullIndex, + Type: sf.Type, + Nullable: nullable, + Opts: opts, + } + + existingCands := nameMap[arrowName] + order := orderCounter + if len(existingCands) > 0 { + order = existingCands[0].order + } else { + orderCounter++ + } + + nameMap[arrowName] = append(existingCands, candidate{ + meta: meta, + depth: entry.depth, + tagged: tagged, + order: order, + }) + } + } + + return nameMap +} + +func resolveFieldCandidates(nameMap map[string][]candidate) []fieldMeta { + resolved := make([]resolvedField, 0, len(nameMap)) + for _, candidates := range nameMap { + minDepth := candidates[0].depth + for _, c := range candidates[1:] { + if c.depth < minDepth { + minDepth = c.depth + } + } + + var atMin []candidate + for _, c := range candidates { + if c.depth == minDepth { + atMin = append(atMin, c) + } + } + + var winner *candidate + if len(atMin) == 1 { + winner = &atMin[0] + } else { + var tagged []candidate + for _, c := range atMin { + if c.tagged { + tagged = append(tagged, c) + } + } + if len(tagged) == 1 { + winner = &tagged[0] + } + } + + if winner != nil { + resolved = append(resolved, resolvedField{meta: winner.meta, order: winner.order}) + } + } + + sort.Slice(resolved, func(i, j int) bool { + return resolved[i].order < resolved[j].order + }) + + result := make([]fieldMeta, len(resolved)) + for i, r := range resolved { + result[i] = r.meta + } + return result +} + +func getStructFields(t reflect.Type) []fieldMeta { + for t.Kind() == reflect.Ptr { + t = t.Elem() + } + + if t.Kind() != reflect.Struct { + return nil + } + + return resolveFieldCandidates(collectFieldCandidates(t)) +} + +var structFieldCache sync.Map + +func cachedStructFields(t reflect.Type) []fieldMeta { + for t.Kind() == reflect.Ptr { + t = t.Elem() + } + + if v, ok := structFieldCache.Load(t); ok { + return v.([]fieldMeta) + } + + fields := getStructFields(t) + v, _ := structFieldCache.LoadOrStore(t, fields) + return v.([]fieldMeta) +} + +func At[T any](arr arrow.Array, i int) (T, error) { + var result T + v := reflect.ValueOf(&result).Elem() + if err := setValue(v, arr, i); err != nil { + var zero T + return zero, err + } + return result, nil +} + +func ToSlice[T any](arr arrow.Array) ([]T, error) { + n := arr.Len() + result := make([]T, n) + for i := 0; i < n; i++ { + v := reflect.ValueOf(&result[i]).Elem() + if err := setValue(v, arr, i); err != nil { + return nil, fmt.Errorf("index %d: %w", i, err) + } + } + return result, nil +} + +// Option configures encoding behavior for [FromSlice] and [RecordFromSlice]. +type Option func(*tagOpts) + +// WithDict requests dictionary encoding for the top-level array. +func WithDict() Option { return func(o *tagOpts) { o.Dict = true } } + +// WithListView requests ListView encoding instead of List for slice types. +func WithListView() Option { return func(o *tagOpts) { o.ListView = true } } + +// WithREE requests run-end encoding for the top-level array. +func WithREE() Option { return func(o *tagOpts) { o.REE = true } } + +// WithDecimal sets the precision and scale for decimal types. +func WithDecimal(precision, scale int32) Option { + return func(o *tagOpts) { + o.DecimalPrecision = precision + o.DecimalScale = scale + o.HasDecimalOpts = true + } +} + +// WithTemporal overrides the Arrow temporal encoding for time.Time slices. +// Valid values: "date32", "date64", "time32", "time64", "timestamp" (default). +// Equivalent to tagging a struct field with arrow:",date32" etc. +// Invalid values cause FromSlice to return an error. +func WithTemporal(temporal string) Option { + return func(o *tagOpts) { o.Temporal = temporal } +} + +func validateTemporalOpt(temporal string) error { + switch temporal { + case "", "timestamp", "date32", "date64", "time32", "time64": + return nil + default: + return fmt.Errorf("arreflect: invalid WithTemporal value %q; valid values are date32, date64, time32, time64, timestamp: %w", temporal, ErrUnsupportedType) + } +} + +func buildEmptyTyped(goType reflect.Type, opts tagOpts, mem memory.Allocator) (arrow.Array, error) { + dt, err := inferArrowType(goType) + if err != nil { + return nil, err + } + derefType := goType + for derefType.Kind() == reflect.Ptr { + derefType = derefType.Elem() + } + dt = applyDecimalOpts(dt, derefType, opts) + dt = applyTemporalOpts(dt, derefType, opts) + if opts.ListView { + if derefType.Kind() != reflect.Slice || derefType == typeOfByteSlice { + return nil, fmt.Errorf("arreflect: WithListView requires a slice-of-slices element type, got %s: %w", goType, ErrUnsupportedType) + } + innerElem := derefType.Elem() + for innerElem.Kind() == reflect.Ptr { + innerElem = innerElem.Elem() + } + innerDT, err := inferArrowType(innerElem) + if err != nil { + return nil, err + } + dt = arrow.ListViewOf(innerDT) + } + if opts.Dict { + if err := validateDictValueType(dt); err != nil { + return nil, err + } + dt = &arrow.DictionaryType{IndexType: arrow.PrimitiveTypes.Int32, ValueType: dt} + } else if opts.REE { + dt = arrow.RunEndEncodedOf(arrow.PrimitiveTypes.Int32, dt) + } + b := array.NewBuilder(mem, dt) + defer b.Release() + return b.NewArray(), nil +} + +func FromSlice[T any](vals []T, mem memory.Allocator, opts ...Option) (arrow.Array, error) { + if mem == nil { + mem = memory.DefaultAllocator + } + var tOpts tagOpts + for _, o := range opts { + o(&tOpts) + } Review Comment: FromSlice applies options by setting booleans in tagOpts, but it doesn’t validate incompatible combinations (e.g. WithDict+WithREE, WithListView+WithREE, WithDict+WithListView). The current behavior silently gives one option precedence (see buildEmptyTyped and buildArray), which is hard for callers to reason about. Consider rejecting conflicting combinations early (e.g. returning ErrUnsupportedType) or clearly documenting precedence. ########## arrow/array/arreflect/reflect_go_to_arrow.go: ########## @@ -0,0 +1,781 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package arreflect + +import ( + "fmt" + "reflect" + "time" + + "github.com/apache/arrow-go/v18/arrow" + "github.com/apache/arrow-go/v18/arrow/array" + "github.com/apache/arrow-go/v18/arrow/decimal" + "github.com/apache/arrow-go/v18/arrow/decimal128" + "github.com/apache/arrow-go/v18/arrow/decimal256" + "github.com/apache/arrow-go/v18/arrow/memory" +) + +func buildArray(vals reflect.Value, opts tagOpts, mem memory.Allocator) (arrow.Array, error) { + if vals.Kind() != reflect.Slice { + return nil, fmt.Errorf("arreflect: expected slice, got %v", vals.Kind()) + } + + elemType := vals.Type().Elem() + for elemType.Kind() == reflect.Ptr { + elemType = elemType.Elem() + } + + if opts.Dict { + return buildDictionaryArray(vals, mem) + } + if opts.REE { + return buildRunEndEncodedArray(vals, opts, mem) + } + if opts.ListView { + if elemType.Kind() != reflect.Slice || elemType == typeOfByteSlice { + return nil, fmt.Errorf("arreflect: WithListView requires a slice-of-slices element type, got %s: %w", elemType, ErrUnsupportedType) + } + return buildListViewArray(vals, mem) + } + + switch elemType { + case typeOfDec32, typeOfDec64, typeOfDec128, typeOfDec256: + return buildDecimalArray(vals, opts, mem) + } + + switch elemType.Kind() { + case reflect.Slice: + if elemType == typeOfByteSlice { + return buildPrimitiveArray(vals, mem) + } + return buildListArray(vals, mem) + + case reflect.Array: + return buildFixedSizeListArray(vals, mem) + + case reflect.Map: + return buildMapArray(vals, mem) + + case reflect.Struct: + switch elemType { + case typeOfTime: + return buildTemporalArray(vals, opts, mem) + default: + return buildStructArray(vals, mem) + } + + default: + return buildPrimitiveArray(vals, mem) + } +} + +func buildPrimitiveArray(vals reflect.Value, mem memory.Allocator) (arrow.Array, error) { + elemType, isPtr := derefSliceElem(vals) + + dt, err := inferArrowType(elemType) + if err != nil { + return nil, err + } + + b := array.NewBuilder(mem, dt) + defer b.Release() + b.Reserve(vals.Len()) + + if err := iterSlice(vals, isPtr, b.AppendNull, func(v reflect.Value) error { + return appendValue(b, v) + }); err != nil { + return nil, err + } + return b.NewArray(), nil +} + +func timeOfDayNanos(t time.Time) int64 { + t = t.UTC() + midnight := time.Date(t.Year(), t.Month(), t.Day(), 0, 0, 0, 0, time.UTC) + return t.Sub(midnight).Nanoseconds() +} + +func asTime(v reflect.Value) (time.Time, error) { + t, ok := reflect.TypeAssert[time.Time](v) + if !ok { + return time.Time{}, fmt.Errorf("expected time.Time, got %s: %w", v.Type(), ErrTypeMismatch) + } + return t, nil +} + +func asDuration(v reflect.Value) (time.Duration, error) { + d, ok := reflect.TypeAssert[time.Duration](v) + if !ok { + return 0, fmt.Errorf("expected time.Duration, got %s: %w", v.Type(), ErrTypeMismatch) + } + return d, nil +} + +func derefSliceElem(vals reflect.Value) (elemType reflect.Type, isPtr bool) { + elemType = vals.Type().Elem() + isPtr = elemType.Kind() == reflect.Ptr + for elemType.Kind() == reflect.Ptr { + elemType = elemType.Elem() + } + return +} + +func iterSlice(vals reflect.Value, isPtr bool, appendNull func(), appendVal func(reflect.Value) error) error { + for i := 0; i < vals.Len(); i++ { + v := vals.Index(i) + if isPtr { + wasNull := false + for v.Kind() == reflect.Ptr { + if v.IsNil() { + appendNull() + wasNull = true + break + } + v = v.Elem() + } + if wasNull { + continue + } + } + if err := appendVal(v); err != nil { + return err + } + } + return nil +} + +func inferListElemDT(vals reflect.Value) (elemDT arrow.DataType, err error) { + outerSliceType, _ := derefSliceElem(vals) + innerElemType := outerSliceType.Elem() + for innerElemType.Kind() == reflect.Ptr { + innerElemType = innerElemType.Elem() + } + elemDT, err = inferArrowType(innerElemType) + return +} + +func temporalBuilder(opts tagOpts, mem memory.Allocator) array.Builder { + switch opts.Temporal { + case "date32": + return array.NewDate32Builder(mem) + case "date64": + return array.NewDate64Builder(mem) + case "time32": + return array.NewTime32Builder(mem, &arrow.Time32Type{Unit: arrow.Millisecond}) + case "time64": + return array.NewTime64Builder(mem, &arrow.Time64Type{Unit: arrow.Nanosecond}) + default: + return array.NewTimestampBuilder(mem, &arrow.TimestampType{Unit: arrow.Nanosecond, TimeZone: "UTC"}) + } +} + +func buildTemporalArray(vals reflect.Value, opts tagOpts, mem memory.Allocator) (arrow.Array, error) { + elemType, isPtr := derefSliceElem(vals) + if elemType != typeOfTime { + return nil, fmt.Errorf("unsupported temporal type %v: %w", elemType, ErrUnsupportedType) + } + b := temporalBuilder(opts, mem) + defer b.Release() + b.Reserve(vals.Len()) + if err := iterSlice(vals, isPtr, b.AppendNull, func(v reflect.Value) error { + return appendTemporalValue(b, v) + }); err != nil { + return nil, err + } + return b.NewArray(), nil +} + +func decimalPrecisionScale(opts tagOpts, defaultPrec int32) (precision, scale int32) { + if opts.HasDecimalOpts { + return opts.DecimalPrecision, opts.DecimalScale + } + return defaultPrec, 0 +} + +func buildDecimalArray(vals reflect.Value, opts tagOpts, mem memory.Allocator) (arrow.Array, error) { + elemType, isPtr := derefSliceElem(vals) + + var b array.Builder + switch elemType { + case typeOfDec128: + p, s := decimalPrecisionScale(opts, dec128DefaultPrecision) + b = array.NewDecimal128Builder(mem, &arrow.Decimal128Type{Precision: p, Scale: s}) + case typeOfDec256: + p, s := decimalPrecisionScale(opts, dec256DefaultPrecision) + b = array.NewDecimal256Builder(mem, &arrow.Decimal256Type{Precision: p, Scale: s}) + case typeOfDec32: + p, s := decimalPrecisionScale(opts, dec32DefaultPrecision) + b = array.NewDecimal32Builder(mem, &arrow.Decimal32Type{Precision: p, Scale: s}) + case typeOfDec64: + p, s := decimalPrecisionScale(opts, dec64DefaultPrecision) + b = array.NewDecimal64Builder(mem, &arrow.Decimal64Type{Precision: p, Scale: s}) + default: + return nil, fmt.Errorf("unsupported decimal type %v: %w", elemType, ErrUnsupportedType) + } + defer b.Release() + b.Reserve(vals.Len()) + if err := iterSlice(vals, isPtr, b.AppendNull, func(v reflect.Value) error { + return appendDecimalValue(b, v) + }); err != nil { + return nil, err + } + return b.NewArray(), nil +} + +func appendStructFields(sb *array.StructBuilder, v reflect.Value, fields []fieldMeta) error { + sb.Append(true) + for fi, fm := range fields { + if err := appendValue(sb.FieldBuilder(fi), v.FieldByIndex(fm.Index)); err != nil { Review Comment: appendStructFields uses v.FieldByIndex(fm.Index). If fm.Index traverses an embedded pointer-to-struct that is nil (from anonymous field promotion), reflect will panic. Consider a safe index walker that checks for nil pointers along the path and appends null for the affected promoted fields instead of panicking. ```suggestion func fieldByIndexSafe(v reflect.Value, index []int) (reflect.Value, bool) { for _, i := range index { if v.Kind() == reflect.Pointer { if v.IsNil() { return reflect.Value{}, false } v = v.Elem() } v = v.Field(i) } return v, true } func appendStructFields(sb *array.StructBuilder, v reflect.Value, fields []fieldMeta) error { sb.Append(true) for fi, fm := range fields { fv, ok := fieldByIndexSafe(v, fm.Index) if !ok { sb.FieldBuilder(fi).AppendNull() continue } if err := appendValue(sb.FieldBuilder(fi), fv); err != 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]
