zeroshade commented on code in PR #34454: URL: https://github.com/apache/arrow/pull/34454#discussion_r1131368102
########## go/arrow/array/extension_builder.go: ########## @@ -0,0 +1,21 @@ +// 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 array + +type ExtensionBuilderWrapper interface { Review Comment: can you add a Godoc comment here to explain what this is for and how to use it? It also might be worthwhile to move the entirety of the `ExtensionBuilder` implementation to this file and update the comment on it to explain the functionality you've added as far as for customizing the behavior. ########## go/arrow/internal/testing/types/extension_test.go: ########## @@ -0,0 +1,63 @@ +// 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 types_test + +import ( + "bytes" + "encoding/json" + "testing" + + "github.com/apache/arrow/go/v12/arrow" + "github.com/apache/arrow/go/v12/arrow/array" + "github.com/apache/arrow/go/v12/arrow/internal/testing/types" + "github.com/apache/arrow/go/v12/arrow/memory" + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +var testUUID = uuid.New() + +func TestExtensionBuilder(t *testing.T) { + extBuilder := array.NewExtensionBuilder(memory.DefaultAllocator, types.NewUUIDType()) + builder := types.NewUUIDBuilder(extBuilder) + builder.Append(testUUID) + arr := builder.NewArray() + arrStr := arr.String() + assert.Equal(t, "[\""+testUUID.String()+"\"]", arrStr) + jsonStr, err := json.Marshal(arr) + assert.NoError(t, err) + + arr1, _, err := array.FromJSON(memory.DefaultAllocator, types.NewUUIDType(), bytes.NewReader(jsonStr)) + assert.NoError(t, err) + assert.Equal(t, arr, arr1) +} + +func TestExtensionRecordBuilder(t *testing.T) { Review Comment: it would also be probably worthwhile to add a test for the `String()` method of `UUIDArray` , since you did the work to have it print the string representation of the UUID instead of the raw bytes when calling `String()` ########## go/arrow/internal/testing/types/extension_types.go: ########## @@ -18,20 +18,171 @@ 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/google/uuid" "golang.org/x/xerrors" ) +type UUIDBuilder struct { + *array.ExtensionBuilder +} + +func NewUUIDBuilder(bldr *array.ExtensionBuilder) *UUIDBuilder { + b := &UUIDBuilder{ + ExtensionBuilder: bldr, + } + 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[:]) +} Review Comment: is `UnsafeAppend` actually needed here at all? ########## go/arrow/datatype_extension.go: ########## @@ -122,18 +122,16 @@ 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) - Review Comment: please put the empty line back here, I'd prefer to keep a single empty line between the public and private method declarations here. ########## go/arrow/internal/testing/types/extension_test.go: ########## @@ -0,0 +1,63 @@ +// 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 types_test + +import ( + "bytes" + "encoding/json" + "testing" + + "github.com/apache/arrow/go/v12/arrow" + "github.com/apache/arrow/go/v12/arrow/array" + "github.com/apache/arrow/go/v12/arrow/internal/testing/types" + "github.com/apache/arrow/go/v12/arrow/memory" + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +var testUUID = uuid.New() + +func TestExtensionBuilder(t *testing.T) { + extBuilder := array.NewExtensionBuilder(memory.DefaultAllocator, types.NewUUIDType()) + builder := types.NewUUIDBuilder(extBuilder) Review Comment: use ```go mem := memory.NewCheckedAllocator(memory.DefaultAllocator) defer mem.AssertSize(t, 0) ``` to ensure that the memory is handled properly (for example, you're missing the `Release` calls here for the builders and generated arrays ########## go/arrow/internal/testing/types/extension_types.go: ########## @@ -18,20 +18,171 @@ 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/google/uuid" "golang.org/x/xerrors" ) +type UUIDBuilder struct { + *array.ExtensionBuilder +} + +func NewUUIDBuilder(bldr *array.ExtensionBuilder) *UUIDBuilder { + b := &UUIDBuilder{ + ExtensionBuilder: bldr, + } + 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) Review Comment: this could be done more efficiently if desired via resize + `UnsafeAppend` or other methods. Not necessary for this PR but just something to think about. ########## go/arrow/internal/testing/types/extension_types.go: ########## @@ -18,20 +18,171 @@ 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/google/uuid" "golang.org/x/xerrors" ) +type UUIDBuilder struct { + *array.ExtensionBuilder +} + +func NewUUIDBuilder(bldr *array.ExtensionBuilder) *UUIDBuilder { + b := &UUIDBuilder{ + ExtensionBuilder: bldr, + } + 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() + if err != nil { + return err + } + + var val uuid.UUID + switch v := t.(type) { + case string: + data, err := uuid.Parse(v) + if err != nil { + return err + } + val = data + case []byte: + data, err := uuid.ParseBytes(v) + if err != nil { + return err + } + val = data Review Comment: Is this case actually getting tested by the tests you wrote? Check the test coverage please to confirm. I think it would only go through the String case. I'm not positive though. -- 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]
