refactor(arreflect): final cleanup — extract buildEmptyTyped, delete dead types, adopt derefSliceElem

zeroshade · zeroshade · commit 2e35ae5e38a8 · 2026-04-15T18:24:39.000-04:00
reflect.go:
- Extract buildEmptyTyped from FromSlice empty-slice branch, eliminating
  37-line duplication of type inference + option application
- Replace validateTemporalOpt map with idiomatic switch (zero-alloc)

reflect_go_to_arrow.go:
- Replace 3 inline deref patterns with derefSliceElem calls

reflect_arrow_to_go.go:
- Extract fillFixedSizeList eliminating duplicated loop between
  reflect.Array and reflect.Slice cases

reflect_test.go:
- Delete 5 unused test struct types and TestHelpers (-58 lines dead code)

reflect_public_test.go:
- Add fieldValueByTag helper, replace 4 inline tag-search loops

Net: -88 lines
diff --git a/arrow/array/arreflect/reflect.go b/arrow/array/arreflect/reflect.go
@@ -379,15 +379,51 @@ func WithTemporal(temporal string) Option {
 	return func(o *tagOpts) { o.Temporal = temporal }
 }
 
-var validTemporalOpts = map[string]bool{
-	"": true, "timestamp": true, "date32": true, "date64": true, "time32": true, "time64": true,
-}
-
 func validateTemporalOpt(temporal string) error {
-	if !validTemporalOpts[temporal] {
+	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)
 	}
-	return nil
+}
+
+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) {
@@ -413,42 +449,7 @@ func FromSlice[T any](vals []T, mem memory.Allocator, opts ...Option) (arrow.Arr
 		}
 	}
 	if len(vals) == 0 {
-		goType := reflect.TypeFor[T]()
-		dt, err := inferArrowType(goType)
-		if err != nil {
-			return nil, err
-		}
-		derefType := goType
-		for derefType.Kind() == reflect.Ptr {
-			derefType = derefType.Elem()
-		}
-		dt = applyDecimalOpts(dt, derefType, tOpts)
-		dt = applyTemporalOpts(dt, derefType, tOpts)
-		if tOpts.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 tOpts.Dict {
-			if err := validateDictValueType(dt); err != nil {
-				return nil, err
-			}
-			dt = &arrow.DictionaryType{IndexType: arrow.PrimitiveTypes.Int32, ValueType: dt}
-		} else if tOpts.REE {
-			dt = arrow.RunEndEncodedOf(arrow.PrimitiveTypes.Int32, dt)
-		}
-		b := array.NewBuilder(mem, dt)
-		defer b.Release()
-		return b.NewArray(), nil
+		return buildEmptyTyped(reflect.TypeFor[T](), tOpts, mem)
 	}
 	sv := reflect.ValueOf(vals)
 	return buildArray(sv, tOpts, mem)
diff --git a/arrow/array/arreflect/reflect_arrow_to_go.go b/arrow/array/arreflect/reflect_arrow_to_go.go
@@ -389,6 +389,15 @@ func setMapValue(v reflect.Value, arr *array.Map, i int) error {
 	return nil
 }
 
+func fillFixedSizeList(dst reflect.Value, child arrow.Array, start, n int) error {
+	for k := 0; k < n; k++ {
+		if err := setValue(dst.Index(k), child, start+k); err != nil {
+			return fmt.Errorf("arreflect: fixed-size list element %d: %w", k, err)
+		}
+	}
+	return nil
+}
+
 func setFixedSizeListValue(v reflect.Value, arr *array.FixedSizeList, i int) error {
 	n := int(arr.DataType().(*arrow.FixedSizeListType).Len())
 	child := arr.ListValues()
@@ -399,17 +408,11 @@ func setFixedSizeListValue(v reflect.Value, arr *array.FixedSizeList, i int) err
 		if v.Len() != n {
 			return fmt.Errorf("fixed-size list length %d does not match Go array length %d: %w", n, v.Len(), ErrTypeMismatch)
 		}
-		for k := 0; k < n; k++ {
-			if err := setValue(v.Index(k), child, int(start)+k); err != nil {
-				return fmt.Errorf("arreflect: fixed-size list element %d: %w", k, err)
-			}
-		}
+		return fillFixedSizeList(v, child, int(start), n)
 	case reflect.Slice:
 		result := reflect.MakeSlice(v.Type(), n, n)
-		for k := 0; k < n; k++ {
-			if err := setValue(result.Index(k), child, int(start)+k); err != nil {
-				return fmt.Errorf("arreflect: fixed-size list element %d: %w", k, err)
-			}
+		if err := fillFixedSizeList(result, child, int(start), n); err != nil {
+			return err
 		}
 		v.Set(result)
 	default:
diff --git a/arrow/array/arreflect/reflect_go_to_arrow.go b/arrow/array/arreflect/reflect_go_to_arrow.go
@@ -247,11 +247,7 @@ func appendStructFields(sb *array.StructBuilder, v reflect.Value, fields []field
 }
 
 func buildStructArray(vals reflect.Value, mem memory.Allocator) (arrow.Array, error) {
-	elemType := vals.Type().Elem()
-	isPtr := elemType.Kind() == reflect.Ptr
-	for elemType.Kind() == reflect.Ptr {
-		elemType = elemType.Elem()
-	}
+	elemType, isPtr := derefSliceElem(vals)
 
 	st, err := inferStructType(elemType)
 	if err != nil {
@@ -574,11 +570,7 @@ func buildListViewArray(vals reflect.Value, mem memory.Allocator) (arrow.Array,
 }
 
 func buildMapArray(vals reflect.Value, mem memory.Allocator) (arrow.Array, error) {
-	mapType := vals.Type().Elem()
-	isPtr := mapType.Kind() == reflect.Ptr
-	for mapType.Kind() == reflect.Ptr {
-		mapType = mapType.Elem()
-	}
+	mapType, isPtr := derefSliceElem(vals)
 
 	keyType := mapType.Key()
 	valType := mapType.Elem()
@@ -628,11 +620,7 @@ func buildMapArray(vals reflect.Value, mem memory.Allocator) (arrow.Array, error
 }
 
 func buildFixedSizeListArray(vals reflect.Value, mem memory.Allocator) (arrow.Array, error) {
-	elemType := vals.Type().Elem()
-	isPtr := elemType.Kind() == reflect.Ptr
-	for elemType.Kind() == reflect.Ptr {
-		elemType = elemType.Elem()
-	}
+	elemType, isPtr := derefSliceElem(vals)
 
 	if elemType.Kind() != reflect.Array {
 		return nil, fmt.Errorf("arreflect: expected array element, got %v", elemType.Kind())
diff --git a/arrow/array/arreflect/reflect_public_test.go b/arrow/array/arreflect/reflect_public_test.go
@@ -30,6 +30,15 @@ import (
 
 func testMem() memory.Allocator { return memory.NewGoAllocator() }
 
+func fieldValueByTag(v reflect.Value, tag string) reflect.Value {
+	for i := 0; i < v.NumField(); i++ {
+		if v.Type().Field(i).Tag.Get("arrow") == tag {
+			return v.Field(i)
+		}
+	}
+	return reflect.Value{}
+}
+
 func TestToGo(t *testing.T) {
 	mem := testMem()
 
@@ -469,16 +478,8 @@ func TestRecordAtAny(t *testing.T) {
 	require.NoError(t, err, "RecordAtAny(0)")
 	v := reflect.ValueOf(got)
 	require.Equal(t, reflect.Struct, v.Kind())
-	var nameField, scoreField reflect.Value
-	for i := 0; i < v.NumField(); i++ {
-		tag := v.Type().Field(i).Tag.Get("arrow")
-		switch tag {
-		case "name":
-			nameField = v.Field(i)
-		case "score":
-			scoreField = v.Field(i)
-		}
-	}
+	nameField := fieldValueByTag(v, "name")
+	scoreField := fieldValueByTag(v, "score")
 	require.True(t, nameField.IsValid(), "name field not found")
 	require.True(t, scoreField.IsValid(), "score field not found")
 	assert.Equal(t, "alice", nameField.String())
@@ -502,12 +503,7 @@ func TestRecordToAnySlice(t *testing.T) {
 	for i, row := range got {
 		v := reflect.ValueOf(row)
 		require.Equal(t, reflect.Struct, v.Kind(), "row %d", i)
-		var nameField reflect.Value
-		for fi := 0; fi < v.NumField(); fi++ {
-			if v.Type().Field(fi).Tag.Get("arrow") == "name" {
-				nameField = v.Field(fi)
-			}
-		}
+		nameField := fieldValueByTag(v, "name")
 		assert.Equal(t, rows[i].Name, nameField.String(), "row %d name", i)
 	}
 }
@@ -534,17 +530,8 @@ func TestAtAnyComposite(t *testing.T) {
 		v := reflect.ValueOf(got)
 		require.Equal(t, reflect.Struct, v.Kind())
 
-		vt := v.Type()
-		var idField, nameField reflect.Value
-		for i := 0; i < v.NumField(); i++ {
-			tag := vt.Field(i).Tag.Get("arrow")
-			switch tag {
-			case "id":
-				idField = v.Field(i)
-			case "name":
-				nameField = v.Field(i)
-			}
-		}
+		idField := fieldValueByTag(v, "id")
+		nameField := fieldValueByTag(v, "name")
 		require.True(t, idField.IsValid(), "id field not found")
 		require.True(t, nameField.IsValid(), "name field not found")
 		assert.Equal(t, int64(99), idField.Int())
@@ -640,17 +627,9 @@ func TestToAnySliceStructArray(t *testing.T) {
 		require.Equal(t, reflect.Struct, v.Kind(), "row %d", i)
 		require.Equal(t, 3, v.NumField(), "row %d", i)
 
-		var id, label, score reflect.Value
-		for fi := 0; fi < v.NumField(); fi++ {
-			switch v.Type().Field(fi).Tag.Get("arrow") {
-			case "id":
-				id = v.Field(fi)
-			case "label":
-				label = v.Field(fi)
-			case "score":
-				score = v.Field(fi)
-			}
-		}
+		id := fieldValueByTag(v, "id")
+		label := fieldValueByTag(v, "label")
+		score := fieldValueByTag(v, "score")
 		require.True(t, id.IsValid(), "row %d: id field not found", i)
 		require.True(t, label.IsValid(), "row %d: label field not found", i)
 		require.True(t, score.IsValid(), "row %d: score field not found", i)
diff --git a/arrow/array/arreflect/reflect_test.go b/arrow/array/arreflect/reflect_test.go
@@ -197,62 +197,3 @@ func TestCachedStructFields(t *testing.T) {
 	assert.Equal(t, "X", fields1[0].Name)
 	assert.Equal(t, "Y", fields1[1].Name)
 }
-
-// ── shared test types used across reflect test files ──────────────────────────
-
-type testPrimitive struct {
-	I8   int8
-	I16  int16
-	I32  int32
-	I64  int64
-	U8   uint8
-	U16  uint16
-	U32  uint32
-	U64  uint64
-	F32  float32
-	F64  float64
-	B    bool
-	S    string
-	Blob []byte
-}
-
-type testNested struct {
-	Name    string
-	Scores  []float64
-	Tags    map[string]string
-	Address struct {
-		City string
-		Zip  int32
-	}
-}
-
-type testNullable struct {
-	Required string
-	Optional *string
-	MaybeInt *int32
-}
-
-type testEmbedded struct {
-	ID string
-	testEmbeddedInner
-}
-
-type testEmbeddedInner struct { //nolint:unused
-	City string
-	Code int32
-}
-
-type testTagged struct {
-	UserName string  `arrow:"user_name"`
-	Score    float64 `arrow:"score"`
-	Hidden   string  `arrow:"-"`
-}
-
-func TestHelpers(t *testing.T) {
-	// Verify shared test types are usable
-	_ = testPrimitive{I8: 1, I32: 2, S: "hi"}
-	_ = testNested{Name: "n", Scores: []float64{1.0}}
-	_ = testNullable{Required: "r"}
-	_ = testTagged{UserName: "u", Score: 3.14}
-	_ = testEmbedded{ID: "id"}
-}
