zeroshade commented on code in PR #34454:
URL: https://github.com/apache/arrow/pull/34454#discussion_r1126868015
##########
go/arrow/internal/testing/types/extension_types.go:
##########
@@ -18,26 +18,181 @@
package types
import (
+ "bytes"
"encoding/binary"
"fmt"
"reflect"
+ "strings"
+
+ "github.com/goccy/go-json"
"github.com/apache/arrow/go/v12/arrow"
"github.com/apache/arrow/go/v12/arrow/array"
+ "github.com/apache/arrow/go/v12/arrow/memory"
+ "github.com/google/uuid"
"golang.org/x/xerrors"
)
+type UUIDBuilder struct {
+ *array.ExtensionBuilder
+ dtype *UUIDType
+}
+
+func NewUUIDBuilder(mem memory.Allocator, dtype arrow.ExtensionType)
*UUIDBuilder {
+ b := &UUIDBuilder{
+ ExtensionBuilder: array.NewExtensionBuilder(mem, dtype),
+ dtype: dtype.(*UUIDType),
+ }
+ return b
+}
+
+func (b *UUIDBuilder) Append(v uuid.UUID) {
+ b.ExtensionBuilder.Builder.(*array.FixedSizeBinaryBuilder).Append(v[:])
+}
+
+func (b *UUIDBuilder) UnsafeAppend(v uuid.UUID) {
+
b.ExtensionBuilder.Builder.(*array.FixedSizeBinaryBuilder).UnsafeAppend(v[:])
+}
+
+func (b *UUIDBuilder) AppendValues(v []uuid.UUID, valid []bool) {
+ data := make([][]byte, len(v))
+ for i, v := range v {
+ data[i] = v[:]
+ }
+
b.ExtensionBuilder.Builder.(*array.FixedSizeBinaryBuilder).AppendValues(data,
valid)
+}
+
+func (b *UUIDBuilder) UnmarshalOne(dec *json.Decoder) error {
+ t, err := dec.Token()
Review Comment:
Should we have default implementations of the Unmarshal methods for the base
`ExtensionBuilder` which just forwards to the storage type?
##########
go/arrow/datatype_extension.go:
##########
@@ -122,7 +124,9 @@ type ExtensionType interface {
// If the storage type is incorrect or something else is invalid with
the data this should
// return nil and an appropriate error.
Deserialize(storageType DataType, data string) (ExtensionType, error)
-
+ // this should return array.Builder interface but we cannot import due
to cycle import, so we use
+ // interface{} instead. At least for
+ NewBuilder(mem memory.Allocator, dt ExtensionType) interface{}
Review Comment:
should we follow the same pattern as `ArrayType` and have a `BuilderType`
method which returns a `reflect.Type` that we use to wrap the
`ExtensionBuilder` with? This also avoids the import cycle.
Another thing to consider is that this is going to break any and all
existing Extension types in other consumers' codebases. We should probably make
a second interface type which contains the `BuilderType` method so that we can
just use a type assertion test in `NewBuilder` rather than break existing
consumers?
--
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]
