zeroshade commented on code in PR #34454:
URL: https://github.com/apache/arrow/pull/34454#discussion_r1130035871
##########
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:
Darn, that's really annoying..... I agree it's better to avoid a big
refactor atm.
I still want to change the signature though instead of having consumers have
to create both the underlying storage builder & wrap it.
Instead of adding this method to the `ExtensionType` interface, we could
just go with an interface defined in the `array` package which would let us use
`Builder` in the method definition. something like:
```go
type ExtensionTypeCustomBuilder interface {
NewBuilder(ExtensionBuilder) Builder
}
```
Then in `builder.go` you can do:
```go
case arrow.EXTENSION:
typ := dtype.(arrow.ExtensionType)
bldr := NewExtensionBuilder(mem, typ)
if custom, ok := typ.(ExtensionTypeCustomBuilder); ok {
return custom.NewBuilder(bldr)
}
return bldr
```
What do you think?
--
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]
