Repository: segmentio/parquet-go
Pull request #387 (2022-10-25 20:44:32)

Refactor assigning Parquet values to Go values
As part of exploring #266, it was identified that the code around deserializing Parquet values into Go values should be refactored to make supporting other types like `time.Time` possible. This is the first change which refactors the monolithic `assignValue()` function into an `AssignValue()` function implemented on each Parquet type.

achille-roussel: :shipit: 

Patch:
diff --git a/row.go b/row.go
index b716bb5..602fdb0 100644
--- a/row.go
+++ b/row.go
@@ -739,10 +739,13 @@ func reconstructFuncOfGroup(columnIndex int16, node Node) (int16, reconstructFun
 
 //go:noinline
 func reconstructFuncOfLeaf(columnIndex int16, node Node) (int16, reconstructFunc) {
+	typ := node.Type()
 	return columnIndex + 1, func(value reflect.Value, _ levels, row Row) (Row, error) {
 		if !row.startsWith(columnIndex) {
 			return row, fmt.Errorf("no values found in parquet row for column %d", columnIndex)
 		}
-		return row[1:], assignValue(value, row[0])
+
+		err := typ.AssignValue(value, row[0])
+		return row[1:], err
 	}
 }
diff --git a/type.go b/type.go
index 312989f..556dde5 100644
--- a/type.go
+++ b/type.go
@@ -4,11 +4,14 @@ import (
 	"bytes"
 	"fmt"
 	"math/bits"
+	"reflect"
 	"time"
+	"unsafe"
 
 	"github.com/segmentio/parquet-go/deprecated"
 	"github.com/segmentio/parquet-go/encoding"
 	"github.com/segmentio/parquet-go/format"
+	"github.com/segmentio/parquet-go/internal/unsafecast"
 )
 
 // Kind is an enumeration type representing the physical types supported by the
@@ -184,6 +187,10 @@ type Type interface {
 	// buffer passed as first argument by dispatching the call to one of the
 	// encoding methods.
 	Decode(dst encoding.Values, src []byte, enc encoding.Encoding) (encoding.Values, error)
+
+	// Assigns a Parquet value to a Go value. Returns an error if conversion is
+	// not possible.
+	AssignValue(dst reflect.Value, src Value) error
 }
 
 var (
@@ -278,6 +285,18 @@ func (t booleanType) Decode(dst encoding.Values, src []byte, enc encoding.Encodi
 	return encoding.DecodeBoolean(dst, src, enc)
 }
 
+func (t booleanType) AssignValue(dst reflect.Value, src Value) error {
+	v := src.Boolean()
+	switch dst.Kind() {
+	case reflect.Bool:
+		dst.SetBool(v)
+	default:
+		dst.Set(reflect.ValueOf(v))
+	}
+
+	return nil
+}
+
 type int32Type struct{}
 
 func (t int32Type) String() string                           { return "INT32" }
@@ -318,6 +337,20 @@ func (t int32Type) Decode(dst encoding.Values, src []byte, enc encoding.Encoding
 	return encoding.DecodeInt32(dst, src, enc)
 }
 
+func (t int32Type) AssignValue(dst reflect.Value, src Value) error {
+	v := src.Int32()
+	switch dst.Kind() {
+	case reflect.Int8, reflect.Int16, reflect.Int32:
+		dst.SetInt(int64(v))
+	case reflect.Uint8, reflect.Uint16, reflect.Uint32:
+		dst.SetUint(uint64(v))
+	default:
+		dst.Set(reflect.ValueOf(v))
+	}
+
+	return nil
+}
+
 type int64Type struct{}
 
 func (t int64Type) String() string                           { return "INT64" }
@@ -358,6 +391,20 @@ func (t int64Type) Decode(dst encoding.Values, src []byte, enc encoding.Encoding
 	return encoding.DecodeInt64(dst, src, enc)
 }
 
+func (t int64Type) AssignValue(dst reflect.Value, src Value) error {
+	v := src.Int64()
+	switch dst.Kind() {
+	case reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64, reflect.Int:
+		dst.SetInt(v)
+	case reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uint, reflect.Uintptr:
+		dst.SetUint(uint64(v))
+	default:
+		dst.Set(reflect.ValueOf(v))
+	}
+
+	return nil
+}
+
 type int96Type struct{}
 
 func (t int96Type) String() string { return "INT96" }
@@ -399,6 +446,12 @@ func (t int96Type) Decode(dst encoding.Values, src []byte, enc encoding.Encoding
 	return encoding.DecodeInt96(dst, src, enc)
 }
 
+func (t int96Type) AssignValue(dst reflect.Value, src Value) error {
+	v := src.Int96()
+	dst.Set(reflect.ValueOf(v))
+	return nil
+}
+
 type floatType struct{}
 
 func (t floatType) String() string                           { return "FLOAT" }
@@ -439,6 +492,18 @@ func (t floatType) Decode(dst encoding.Values, src []byte, enc encoding.Encoding
 	return encoding.DecodeFloat(dst, src, enc)
 }
 
+func (t floatType) AssignValue(dst reflect.Value, src Value) error {
+	v := src.Float()
+	switch dst.Kind() {
+	case reflect.Float32, reflect.Float64:
+		dst.SetFloat(float64(v))
+	default:
+		dst.Set(reflect.ValueOf(v))
+	}
+
+	return nil
+}
+
 type doubleType struct{}
 
 func (t doubleType) String() string                           { return "DOUBLE" }
@@ -479,6 +544,18 @@ func (t doubleType) Decode(dst encoding.Values, src []byte, enc encoding.Encodin
 	return encoding.DecodeDouble(dst, src, enc)
 }
 
+func (t doubleType) AssignValue(dst reflect.Value, src Value) error {
+	v := src.Double()
+	switch dst.Kind() {
+	case reflect.Float32, reflect.Float64:
+		dst.SetFloat(v)
+	default:
+		dst.Set(reflect.ValueOf(v))
+	}
+
+	return nil
+}
+
 type byteArrayType struct{}
 
 func (t byteArrayType) String() string                           { return "BYTE_ARRAY" }
@@ -519,6 +596,21 @@ func (t byteArrayType) Decode(dst encoding.Values, src []byte, enc encoding.Enco
 	return encoding.DecodeByteArray(dst, src, enc)
 }
 
+func (t byteArrayType) AssignValue(dst reflect.Value, src Value) error {
+	v := src.ByteArray()
+	switch dst.Kind() {
+	case reflect.String:
+		dst.SetString(string(v))
+	case reflect.Slice:
+		dst.SetBytes(copyBytes(v))
+	default:
+		val := reflect.ValueOf(string(v))
+		dst.Set(val)
+	}
+
+	return nil
+}
+
 type fixedLenByteArrayType struct{ length int }
 
 func (t fixedLenByteArrayType) String() string {
@@ -571,6 +663,31 @@ func (t fixedLenByteArrayType) Decode(dst encoding.Values, src []byte, enc encod
 	return encoding.DecodeFixedLenByteArray(dst, src, enc)
 }
 
+func (t fixedLenByteArrayType) AssignValue(dst reflect.Value, src Value) error {
+	v := src.ByteArray()
+	switch dst.Kind() {
+	case reflect.Array:
+		if dst.Type().Elem().Kind() == reflect.Uint8 && dst.Len() == len(v) {
+			// This code could be implemented as a call to reflect.Copy but
+			// it would require creating a reflect.Value from v which causes
+			// the heap allocation to pack the []byte value. To avoid this
+			// overhead we instead convert the reflect.Value holding the
+			// destination array into a byte slice which allows us to use
+			// a more efficient call to copy.
+			d := unsafe.Slice((*byte)(unsafecast.PointerOfValue(dst)), len(v))
+			copy(d, v)
+			return nil
+		}
+	case reflect.Slice:
+		dst.SetBytes(copyBytes(v))
+		return nil
+	}
+
+	val := reflect.ValueOf(copyBytes(v))
+	dst.Set(val)
+	return nil
+}
+
 // BE128 stands for "big-endian 128 bits". This type is used as a special case
 // for fixed-length byte arrays of 16 bytes, which are commonly used to
 // represent columns of random unique identifiers such as UUIDs.
@@ -632,6 +749,10 @@ func (t be128Type) Decode(dst encoding.Values, src []byte, enc encoding.Encoding
 	return encoding.DecodeFixedLenByteArray(dst, src, enc)
 }
 
+func (t be128Type) AssignValue(dst reflect.Value, src Value) error {
+	return fixedLenByteArrayType{length: 16}.AssignValue(dst, src)
+}
+
 // FixedLenByteArrayType constructs a type for fixed-length values of the given
 // size (in bytes).
 func FixedLenByteArrayType(length int) Type {
@@ -838,6 +959,14 @@ func (t *intType) Decode(dst encoding.Values, src []byte, enc encoding.Encoding)
 	}
 }
 
+func (t *intType) AssignValue(dst reflect.Value, src Value) error {
+	if t.BitWidth == 64 {
+		return Int64Type.AssignValue(dst, src)
+	} else {
+		return Int32Type.AssignValue(dst, src)
+	}
+}
+
 // Decimal constructs a leaf node of decimal logical type with the given
 // scale, precision, and underlying type.
 //
@@ -935,6 +1064,10 @@ func (t *stringType) Decode(dst encoding.Values, src []byte, enc encoding.Encodi
 	return encoding.DecodeByteArray(dst, src, enc)
 }
 
+func (t *stringType) AssignValue(dst reflect.Value, src Value) error {
+	return ByteArrayType.AssignValue(dst, src)
+}
+
 // UUID constructs a leaf node of UUID logical type.
 //
 // https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#uuid
@@ -992,6 +1125,10 @@ func (t *uuidType) Decode(dst encoding.Values, src []byte, enc encoding.Encoding
 	return encoding.DecodeFixedLenByteArray(dst, src, enc)
 }
 
+func (t *uuidType) AssignValue(dst reflect.Value, src Value) error {
+	return fixedLenByteArrayType{length: 16}.AssignValue(dst, src)
+}
+
 // Enum constructs a leaf node with a logical type representing enumerations.
 //
 // https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#enum
@@ -1055,6 +1192,10 @@ func (t *enumType) Decode(dst encoding.Values, src []byte, enc encoding.Encoding
 	return encoding.DecodeByteArray(dst, src, enc)
 }
 
+func (t *enumType) AssignValue(dst reflect.Value, src Value) error {
+	return ByteArrayType.AssignValue(dst, src)
+}
+
 // JSON constructs a leaf node of JSON logical type.
 //
 // https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#json
@@ -1118,6 +1259,10 @@ func (t *jsonType) Decode(dst encoding.Values, src []byte, enc encoding.Encoding
 	return encoding.DecodeByteArray(dst, src, enc)
 }
 
+func (t *jsonType) AssignValue(dst reflect.Value, src Value) error {
+	return ByteArrayType.AssignValue(dst, src)
+}
+
 // BSON constructs a leaf node of BSON logical type.
 //
 // https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#bson
@@ -1181,6 +1326,10 @@ func (t *bsonType) Decode(dst encoding.Values, src []byte, enc encoding.Encoding
 	return encoding.DecodeByteArray(dst, src, enc)
 }
 
+func (t *bsonType) AssignValue(dst reflect.Value, src Value) error {
+	return ByteArrayType.AssignValue(dst, src)
+}
+
 // Date constructs a leaf node of DATE logical type.
 //
 // https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#date
@@ -1240,6 +1389,10 @@ func (t *dateType) Decode(dst encoding.Values, src []byte, enc encoding.Encoding
 	return encoding.DecodeInt32(dst, src, enc)
 }
 
+func (t *dateType) AssignValue(dst reflect.Value, src Value) error {
+	return Int32Type.AssignValue(dst, src)
+}
+
 // TimeUnit represents units of time in the parquet type system.
 type TimeUnit interface {
 	// Returns the precision of the time unit as a time.Duration value.
@@ -1412,6 +1565,14 @@ func (t *timeType) Decode(dst encoding.Values, src []byte, enc encoding.Encoding
 	}
 }
 
+func (t *timeType) AssignValue(dst reflect.Value, src Value) error {
+	if t.useInt32() {
+		return Int32Type.AssignValue(dst, src)
+	} else {
+		return Int64Type.AssignValue(dst, src)
+	}
+}
+
 // Timestamp constructs of leaf node of TIMESTAMP logical type.
 //
 // https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#timestamp
@@ -1478,6 +1639,10 @@ func (t *timestampType) Decode(dst encoding.Values, src []byte, enc encoding.Enc
 	return encoding.DecodeInt64(dst, src, enc)
 }
 
+func (t *timestampType) AssignValue(dst reflect.Value, src Value) error {
+	return Int64Type.AssignValue(dst, src)
+}
+
 // List constructs a node of LIST logical type.
 //
 // https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#lists
@@ -1541,6 +1706,10 @@ func (t *listType) Decode(_ encoding.Values, _ []byte, _ encoding.Encoding) (enc
 	panic("cannot decode parquet LIST type")
 }
 
+func (t *listType) AssignValue(reflect.Value, Value) error {
+	panic("cannot assign value to a parquet LIST type")
+}
+
 // Map constructs a node of MAP logical type.
 //
 // https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#maps
@@ -1609,6 +1778,10 @@ func (t *mapType) Decode(_ encoding.Values, _ []byte, _ encoding.Encoding) (enco
 	panic("cannot decode parquet MAP type")
 }
 
+func (t *mapType) AssignValue(reflect.Value, Value) error {
+	panic("cannot assign value to a parquet MAP type")
+}
+
 type nullType format.NullType
 
 func (t *nullType) String() string { return (*format.NullType)(t).String() }
@@ -1659,6 +1832,10 @@ func (t *nullType) Decode(dst encoding.Values, _ []byte, _ encoding.Encoding) (e
 	return dst, nil
 }
 
+func (t *nullType) AssignValue(reflect.Value, Value) error {
+	return nil
+}
+
 type groupType struct{}
 
 func (groupType) String() string { return "group" }
@@ -1699,6 +1876,10 @@ func (groupType) Decode(_ encoding.Values, _ []byte, _ encoding.Encoding) (encod
 	panic("cannot decode parquet group")
 }
 
+func (groupType) AssignValue(reflect.Value, Value) error {
+	panic("cannot assign value to a parquet group")
+}
+
 func (groupType) Length() int { return 0 }
 
 func (groupType) EstimateSize(int) int64 { return 0 }
diff --git a/value.go b/value.go
index 2d4b299..d3ece01 100644
--- a/value.go
+++ b/value.go
@@ -383,7 +383,14 @@ func (v Value) Int32() int32 { return int32(v.u64) }
 func (v Value) Int64() int64 { return int64(v.u64) }
 
 // Int96 returns v as a int96, assuming the underlying type is INT96.
-func (v Value) Int96() deprecated.Int96 { return makeInt96(v.ByteArray()) }
+func (v Value) Int96() deprecated.Int96 {
+	var val deprecated.Int96
+	if !v.IsNull() {
+		val = makeInt96(v.ByteArray())
+	}
+
+	return val
+}
 
 // Float returns v as a float32, assuming the underlying type is FLOAT.
 func (v Value) Float() float32 { return math.Float32frombits(uint32(v.u64)) }
@@ -589,124 +596,6 @@ func makeInt96(bits []byte) (i96 deprecated.Int96) {
 	}
 }
 
-func assignValue(dst reflect.Value, src Value) error {
-	if src.IsNull() {
-		dst.Set(reflect.Zero(dst.Type()))
-		return nil
-	}
-
-	dstKind := dst.Kind()
-	srcKind := src.Kind()
-
-	var val reflect.Value
-	switch srcKind {
-	case Boolean:
-		v := src.Boolean()
-		switch dstKind {
-		case reflect.Bool:
-			dst.SetBool(v)
-			return nil
-		default:
-			val = reflect.ValueOf(v)
-		}
-
-	case Int32:
-		v := int64(src.Int32())
-		switch dstKind {
-		case reflect.Int8, reflect.Int16, reflect.Int32:
-			dst.SetInt(int64(v))
-			return nil
-		case reflect.Uint8, reflect.Uint16, reflect.Uint32:
-			dst.SetUint(uint64(v))
-			return nil
-		default:
-			val = reflect.ValueOf(v)
-		}
-
-	case Int64:
-		v := src.Int64()
-		switch dstKind {
-		case reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64, reflect.Int:
-			dst.SetInt(v)
-			return nil
-		case reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uint, reflect.Uintptr:
-			dst.SetUint(uint64(v))
-			return nil
-		default:
-			val = reflect.ValueOf(v)
-		}
-
-	case Int96:
-		val = reflect.ValueOf(src.Int96())
-
-	case Float:
-		v := src.Float()
-		switch dstKind {
-		case reflect.Float32, reflect.Float64:
-			dst.SetFloat(float64(v))
-			return nil
-		default:
-			val = reflect.ValueOf(v)
-		}
-
-	case Double:
-		v := src.Double()
-		switch dstKind {
-		case reflect.Float32, reflect.Float64:
-			dst.SetFloat(v)
-			return nil
-		default:
-			val = reflect.ValueOf(v)
-		}
-
-	case ByteArray:
-		v := src.ByteArray()
-		switch dstKind {
-		case reflect.String:
-			dst.SetString(string(v))
-			return nil
-		case reflect.Slice:
-			if dst.Type().Elem().Kind() == reflect.Uint8 {
-				dst.SetBytes(copyBytes(v))
-				return nil
-			}
-		default:
-			val = reflect.ValueOf(string(v))
-		}
-
-	case FixedLenByteArray:
-		v := src.ByteArray()
-		switch dstKind {
-		case reflect.Array:
-			if dst.Type().Elem().Kind() == reflect.Uint8 && dst.Len() == len(v) {
-				// This code could be implemented as a call to reflect.Copy but
-				// it would require creating a reflect.Value from v which causes
-				// the heap allocation to pack the []byte value. To avoid this
-				// overhead we instead convert the reflect.Value holding the
-				// destination array into a byte slice which allows us to use
-				// a more efficient call to copy.
-				d := unsafe.Slice((*byte)(unsafecast.PointerOfValue(dst)), len(v))
-				copy(d, v)
-				return nil
-			}
-		case reflect.Slice:
-			if dst.Type().Elem().Kind() == reflect.Uint8 {
-				dst.SetBytes(copyBytes(v))
-				return nil
-			}
-		default:
-			val = reflect.ValueOf(copyBytes(v))
-		}
-	}
-
-	if val.IsValid() && val.Type().AssignableTo(dst.Type()) {
-		dst.Set(val)
-		return nil
-	}
-
-	return fmt.Errorf("cannot assign parquet value of type %s to go value of type %s", srcKind.String(), dst.Type())
-}
-
 func parseValue(kind Kind, data []byte) (val Value, err error) {
 	switch kind {
 	case Boolean:


Test patch:
diff --git a/value_test.go b/value_test.go
index cf2fa05..db9d7dd 100644
--- a/value_test.go
+++ b/value_test.go
@@ -1,11 +1,13 @@
 package parquet_test
 
 import (
+	"bytes"
 	"math"
 	"testing"
 	"unsafe"
 
 	"github.com/segmentio/parquet-go"
+	"github.com/segmentio/parquet-go/deprecated"
 )
 
 func TestSizeOfValue(t *testing.T) {
@@ -90,3 +92,52 @@ func TestValueClone(t *testing.T) {
 		})
 	}
 }
+
+func TestZeroValue(t *testing.T) {
+	var v parquet.Value
+	if !v.IsNull() {
+		t.Error("expected zero value parquet.Value to be null")
+	}
+
+	if v.Byte() != byte(0) {
+		t.Errorf("byte not zero value: got=%#v", v.Byte())
+	}
+
+	if v.Boolean() != false {
+		t.Errorf("boolean not zero value: got=%#v", v.Boolean())
+	}
+
+	if v.Int32() != 0 {
+		t.Errorf("int32 not zero value: got=%#v", v.Int32())
+	}
+
+	if v.Int64() != 0 {
+		t.Errorf("int64 not zero value: got=%#v", v.Int64())
+	}
+
+	var zeroInt96 deprecated.Int96
+	if v.Int96() != zeroInt96 {
+		t.Errorf("int96 not zero value: got=%#v", zeroInt96)
+	}
+
+	if v.Float() != 0 {
+		t.Errorf("float not zero value: got=%#v", v.Float())
+	}
+
+	if v.Double() != 0 {
+		t.Errorf("double not zero value: got=%#v", v.Double())
+	}
+
+	if v.Uint32() != 0 {
+		t.Errorf("uint32 not zero value: got=%#v", v.Uint32())
+	}
+
+	if v.Uint64() != 0 {
+		t.Errorf("uint64 not zero value: got=%#v", v.Uint64())
+	}
+
+	var zeroByte []byte
+	if !bytes.Equal(v.ByteArray(), zeroByte) {
+		t.Errorf("byte array not zero value: got=%#v", v.ByteArray())
+	}
+}