You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "candiduslynx (via GitHub)" <gi...@apache.org> on 2023/05/06 06:26:36 UTC
[GitHub] [arrow] candiduslynx opened a new pull request, #35457: GH-35421: [Go] Ensure interface contract between `array.X.ValueStr` & `array.XBuilder.AppendValueFromString`
candiduslynx opened a new pull request, #35457:
URL: https://github.com/apache/arrow/pull/35457
### Rationale for this change
I noticed that some values produced by `array.X.ValueStr` can't be parsed back by `array.XBuilder.AppendValueFromString` while debugging https://github.com/cloudquery/cloudquery/pull/10284.
Additionally, some arrays didn't implement the corresponding functions at all.
### What changes are included in this PR?
* Ensure interface contract
* Use `array.NullValueStr` constant
* Fix bugs found along the way:
* Synced `internal/types/UUID` with CQ implementation to account for `valid` param
### Are these changes tested?
This code was developed in TDD mode. The changes workflow was:
1. Introduce tests:
1. `_ValueStr` to test that the resulting strings are the same
2. `_AppendValueFromString` to test that the stored values are the same
2. Fix any discrepancies
### Are there any user-facing changes?
Some fixes to the accepted values & parse logic to correspond to the interface contract.
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please uncomment the line below and explain which changes are breaking.
-->
<!-- **This PR includes breaking changes to public APIs.** -->
<!--
Please uncomment the line below (and provide explanation) if the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld). We use this to highlight fixes to issues that may affect users without their knowledge. For this reason, fixing bugs that cause errors don't count, since those are usually obvious.
-->
<!-- **This PR contains a "Critical Fix".** -->
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] zeroshade merged pull request #35457: GH-35421: [Go] Ensure interface contract between `array.X.ValueStr` & `array.XBuilder.AppendValueFromString`
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #35457:
URL: https://github.com/apache/arrow/pull/35457
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] candiduslynx commented on a diff in pull request #35457: GH-35421: [Go] Ensure interface contract between `array.X.ValueStr` & `array.XBuilder.AppendValueFromString`
Posted by "candiduslynx (via GitHub)" <gi...@apache.org>.
candiduslynx commented on code in PR #35457:
URL: https://github.com/apache/arrow/pull/35457#discussion_r1187747529
##########
go/arrow/datatype_fixedwidth.go:
##########
@@ -325,6 +325,22 @@ func (u TimeUnit) Multiplier() time.Duration {
func (u TimeUnit) String() string { return [...]string{"s", "ms", "us", "ns"}[uint(u)&3] }
+var _strToTimeUnit = map[string]TimeUnit{
+ "s": Second,
+ "ms": Millisecond,
+ "us": Microsecond,
+ "ns": Nanosecond,
+}
+
+// TimeUnitFromString is the reverse of TimeUnit.String
+func TimeUnitFromString(s string) (TimeUnit, error) {
+ if v, ok := _strToTimeUnit[s]; ok {
+ return v, nil
+ }
+
+ return 0, fmt.Errorf("unsupported time unit: %q", s)
+}
Review Comment:
Not used anymore.
I'll remove this in a minute
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] candiduslynx commented on a diff in pull request #35457: GH-35421: [Go] Ensure interface contract between `array.X.ValueStr` & `array.XBuilder.AppendValueFromString`
Posted by "candiduslynx (via GitHub)" <gi...@apache.org>.
candiduslynx commented on code in PR #35457:
URL: https://github.com/apache/arrow/pull/35457#discussion_r1187602732
##########
go/arrow/array/union.go:
##########
@@ -1110,10 +1133,15 @@ func NewEmptyDenseUnionBuilder(mem memory.Allocator) *DenseUnionBuilder {
// children and type codes. Builders will be constructed for each child
// using the fields in typ
func NewDenseUnionBuilder(mem memory.Allocator, typ *arrow.DenseUnionType) *DenseUnionBuilder {
- children := make([]Builder, len(typ.Fields()))
- for i, f := range typ.Fields() {
- children[i] = NewBuilder(mem, f.Type)
- defer children[i].Release()
+ children := make([]Builder, 0, len(typ.Fields()))
+ defer func() {
+ for _, child := range children {
+ child.Release()
+ }
+ }()
Review Comment:
yep + performing defer inside a loop is overall a bad practice
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] candiduslynx commented on a diff in pull request #35457: GH-35421: [Go] Ensure interface contract between `array.X.ValueStr` & `array.XBuilder.AppendValueFromString`
Posted by "candiduslynx (via GitHub)" <gi...@apache.org>.
candiduslynx commented on code in PR #35457:
URL: https://github.com/apache/arrow/pull/35457#discussion_r1187630688
##########
go/internal/types/extension_types.go:
##########
@@ -208,37 +199,49 @@ type UUIDType struct {
// NewUUIDType is a convenience function to create an instance of UuidType
// with the correct storage type
func NewUUIDType() *UUIDType {
- return &UUIDType{
- ExtensionBase: arrow.ExtensionBase{
- Storage: &arrow.FixedSizeBinaryType{ByteWidth: 16}}}
+ return &UUIDType{ExtensionBase: arrow.ExtensionBase{Storage: &arrow.FixedSizeBinaryType{ByteWidth: 16}}}
}
-// ArrayType returns TypeOf(UuidArray) for constructing uuid arrays
-func (UUIDType) ArrayType() reflect.Type { return reflect.TypeOf(UUIDArray{}) }
+// ArrayType returns TypeOf(UUIDArray{}) for constructing UUID arrays
+func (*UUIDType) ArrayType() reflect.Type {
+ return reflect.TypeOf(UUIDArray{})
+}
+
+func (*UUIDType) ExtensionName() string {
+ return "uuid"
+}
+
+func (e *UUIDType) String() string {
+ return fmt.Sprintf("extension_type<storage=%s>", e.Storage)
+}
-func (UUIDType) ExtensionName() string { return "uuid" }
+func (e *UUIDType) MarshalJSON() ([]byte, error) {
+ return []byte(fmt.Sprintf(`{"name":"%s","metadata":%s}`, e.ExtensionName(), e.Serialize())), nil
+}
// Serialize returns "uuid-serialized" for testing proper metadata passing
-func (UUIDType) Serialize() string { return "uuid-serialized" }
+func (*UUIDType) Serialize() string {
+ return "uuid-serialized"
+}
// Deserialize expects storageType to be FixedSizeBinaryType{ByteWidth: 16} and the data to be
-// "uuid-serialized" in order to correctly create a UUIDType for testing deserialize.
-func (UUIDType) Deserialize(storageType arrow.DataType, data string) (arrow.ExtensionType, error) {
- if string(data) != "uuid-serialized" {
- return nil, fmt.Errorf("type identifier did not match: '%s'", string(data))
+// "uuid-serialized" in order to correctly create a UuidType for testing deserialize.
Review Comment:
Done in 3e314138add5174886bb3c7c507d7ac0ae4f104b
##########
go/internal/types/extension_types.go:
##########
@@ -208,37 +199,49 @@ type UUIDType struct {
// NewUUIDType is a convenience function to create an instance of UuidType
// with the correct storage type
func NewUUIDType() *UUIDType {
- return &UUIDType{
- ExtensionBase: arrow.ExtensionBase{
- Storage: &arrow.FixedSizeBinaryType{ByteWidth: 16}}}
+ return &UUIDType{ExtensionBase: arrow.ExtensionBase{Storage: &arrow.FixedSizeBinaryType{ByteWidth: 16}}}
}
-// ArrayType returns TypeOf(UuidArray) for constructing uuid arrays
-func (UUIDType) ArrayType() reflect.Type { return reflect.TypeOf(UUIDArray{}) }
+// ArrayType returns TypeOf(UUIDArray{}) for constructing UUID arrays
+func (*UUIDType) ArrayType() reflect.Type {
+ return reflect.TypeOf(UUIDArray{})
+}
+
+func (*UUIDType) ExtensionName() string {
+ return "uuid"
+}
+
+func (e *UUIDType) String() string {
+ return fmt.Sprintf("extension_type<storage=%s>", e.Storage)
+}
-func (UUIDType) ExtensionName() string { return "uuid" }
+func (e *UUIDType) MarshalJSON() ([]byte, error) {
+ return []byte(fmt.Sprintf(`{"name":"%s","metadata":%s}`, e.ExtensionName(), e.Serialize())), nil
+}
// Serialize returns "uuid-serialized" for testing proper metadata passing
-func (UUIDType) Serialize() string { return "uuid-serialized" }
+func (*UUIDType) Serialize() string {
+ return "uuid-serialized"
+}
// Deserialize expects storageType to be FixedSizeBinaryType{ByteWidth: 16} and the data to be
-// "uuid-serialized" in order to correctly create a UUIDType for testing deserialize.
-func (UUIDType) Deserialize(storageType arrow.DataType, data string) (arrow.ExtensionType, error) {
- if string(data) != "uuid-serialized" {
- return nil, fmt.Errorf("type identifier did not match: '%s'", string(data))
+// "uuid-serialized" in order to correctly create a UuidType for testing deserialize.
+func (*UUIDType) Deserialize(storageType arrow.DataType, data string) (arrow.ExtensionType, error) {
+ if data != "uuid-serialized" {
+ return nil, fmt.Errorf("type identifier did not match: '%s'", data)
}
if !arrow.TypeEqual(storageType, &arrow.FixedSizeBinaryType{ByteWidth: 16}) {
- return nil, fmt.Errorf("invalid storage type for UUIDType: %s", storageType.Name())
+ return nil, fmt.Errorf("invalid storage type for UuidType: %s", storageType.Name())
Review Comment:
Done in 3e314138add5174886bb3c7c507d7ac0ae4f104b
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] candiduslynx commented on a diff in pull request #35457: GH-35421: [Go] Ensure interface contract between `array.X.ValueStr` & `array.XBuilder.AppendValueFromString`
Posted by "candiduslynx (via GitHub)" <gi...@apache.org>.
candiduslynx commented on code in PR #35457:
URL: https://github.com/apache/arrow/pull/35457#discussion_r1187622953
##########
go/arrow/array/numericbuilder.gen.go.tmpl:
##########
@@ -203,6 +203,21 @@ func (b *{{.Name}}Builder) AppendValueFromString(s string) error {
return err
}
b.Append(arrow.Date64FromTime(tm))
+ {{else if or (eq .Name "Dictionary") -}}
Review Comment:
removed in 720ce704e0b6fa4f6bd2f063bdae0a6b94d34b3f
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] candiduslynx commented on a diff in pull request #35457: GH-35421: [Go] Ensure interface contract between `array.X.ValueStr` & `array.XBuilder.AppendValueFromString`
Posted by "candiduslynx (via GitHub)" <gi...@apache.org>.
candiduslynx commented on code in PR #35457:
URL: https://github.com/apache/arrow/pull/35457#discussion_r1187624903
##########
go/arrow/array/dictionary.go:
##########
@@ -742,11 +745,29 @@ func (b *dictionaryBuilder) Unmarshal(dec *json.Decoder) error {
}
func (b *dictionaryBuilder) AppendValueFromString(s string) error {
Review Comment:
done in 605ac9e51b760f5a42f90ce4eaa512121478294a
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] candiduslynx commented on pull request #35457: GH-35421: [Go] Ensure interface contract between `array.X.ValueStr` & `array.XBuilder.AppendValueFromString`
Posted by "candiduslynx (via GitHub)" <gi...@apache.org>.
candiduslynx commented on PR #35457:
URL: https://github.com/apache/arrow/pull/35457#issuecomment-1538050725
@zeroshade is there a chance to get review sooner? I'd like to sync this to https://github.com/cloudquery/cloudquery/pull/10284 and use the `ValueStr` instead of JSON marshaling by default
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] candiduslynx commented on a diff in pull request #35457: GH-35421: [Go] Ensure interface contract between `array.X.ValueStr` & `array.XBuilder.AppendValueFromString`
Posted by "candiduslynx (via GitHub)" <gi...@apache.org>.
candiduslynx commented on code in PR #35457:
URL: https://github.com/apache/arrow/pull/35457#discussion_r1187622009
##########
.gitignore:
##########
@@ -91,4 +91,6 @@ java-native-cpp/
# archery files
dev/archery/build
-swift/Arrow/.build
\ No newline at end of file
+swift/Arrow/.build
+
+go/vendor
Review Comment:
I like to be able to jump to the definition inside the IDE (and it usually requires to actually have the deps downloaded & analyzed).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] candiduslynx commented on a diff in pull request #35457: GH-35421: [Go] Ensure interface contract between `array.X.ValueStr` & `array.XBuilder.AppendValueFromString`
Posted by "candiduslynx (via GitHub)" <gi...@apache.org>.
candiduslynx commented on code in PR #35457:
URL: https://github.com/apache/arrow/pull/35457#discussion_r1187616418
##########
go/arrow/example_test.go:
##########
@@ -252,7 +252,7 @@ func Example_fixedSizeListArray() {
// NullN() = 2
// Len() = 5
// Type() = fixed_size_list<item: int64>[3]
- // List = [[0 1 2] (null) [3 4 5] [6 7 8] (null)]
+ // List = [[0 1 2] (null) [-1 -1 -1] [3 4 5] (null)]
Review Comment:
@zeroshade I've extracted the change into https://github.com/apache/arrow/pull/35481.
I'll update the code after that one is merged.
##########
go/arrow/array/fixed_size_list.go:
##########
@@ -207,6 +207,10 @@ func (b *FixedSizeListBuilder) Append(v bool) {
func (b *FixedSizeListBuilder) AppendNull() {
b.Reserve(1)
b.unsafeAppendBoolToBitmap(false)
+ // require to append this due to value indexes
+ for i := int32(0); i < b.n; i++ {
+ b.values.AppendNull()
+ }
Review Comment:
@zeroshade I've extracted the change into https://github.com/apache/arrow/pull/35481.
I'll update the code after that one is merged.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] candiduslynx commented on a diff in pull request #35457: GH-35421: [Go] Ensure interface contract between `array.X.ValueStr` & `array.XBuilder.AppendValueFromString`
Posted by "candiduslynx (via GitHub)" <gi...@apache.org>.
candiduslynx commented on code in PR #35457:
URL: https://github.com/apache/arrow/pull/35457#discussion_r1187604197
##########
go/arrow/array/fixed_size_list.go:
##########
@@ -207,6 +207,10 @@ func (b *FixedSizeListBuilder) Append(v bool) {
func (b *FixedSizeListBuilder) AppendNull() {
b.Reserve(1)
b.unsafeAppendBoolToBitmap(false)
+ // require to append this due to value indexes
+ for i := int32(0); i < b.n; i++ {
+ b.values.AppendNull()
+ }
Review Comment:
I think it might be extracted into a separate PR.
But, it'll have to be merged prior to this one then, as the quirky code was found during the testing.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] candiduslynx commented on a diff in pull request #35457: GH-35421: [Go] Ensure interface contract between `array.X.ValueStr` & `array.XBuilder.AppendValueFromString`
Posted by "candiduslynx (via GitHub)" <gi...@apache.org>.
candiduslynx commented on code in PR #35457:
URL: https://github.com/apache/arrow/pull/35457#discussion_r1187628331
##########
go/internal/types/extension_types.go:
##########
@@ -82,14 +105,6 @@ func (b *UUIDBuilder) UnmarshalOne(dec *json.Decoder) error {
}
}
- if len(val) != 16 {
- return &json.UnmarshalTypeError{
- Value: fmt.Sprint(val),
- Type: reflect.TypeOf([]byte{}),
- Offset: dec.InputOffset(),
- Struct: fmt.Sprintf("FixedSizeBinary[%d]", 16),
- }
- }
Review Comment:
this part is unreachable (uuid.UUID is defined as [16]byte)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] candiduslynx commented on pull request #35457: GH-35421: [Go] Ensure interface contract between `array.X.ValueStr` & `array.XBuilder.AppendValueFromString`
Posted by "candiduslynx (via GitHub)" <gi...@apache.org>.
candiduslynx commented on PR #35457:
URL: https://github.com/apache/arrow/pull/35457#issuecomment-1538836080
Removed the extra piece in 2bf65c58573f6ddbdc2e0f6fed13dd79516e6bac, thanks for the review!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] candiduslynx commented on a diff in pull request #35457: GH-35421: [Go] Ensure interface contract between `array.X.ValueStr` & `array.XBuilder.AppendValueFromString`
Posted by "candiduslynx (via GitHub)" <gi...@apache.org>.
candiduslynx commented on code in PR #35457:
URL: https://github.com/apache/arrow/pull/35457#discussion_r1213612529
##########
go/internal/types/extension_types.go:
##########
@@ -82,14 +105,6 @@ func (b *UUIDBuilder) UnmarshalOne(dec *json.Decoder) error {
}
}
- if len(val) != 16 {
- return &json.UnmarshalTypeError{
- Value: fmt.Sprint(val),
- Type: reflect.TypeOf([]byte{}),
- Offset: dec.InputOffset(),
- Struct: fmt.Sprintf("FixedSizeBinary[%d]", 16),
- }
- }
Review Comment:
the type is defined as `[16]byte`
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] zeroshade commented on a diff in pull request #35457: GH-35421: [Go] Ensure interface contract between `array.X.ValueStr` & `array.XBuilder.AppendValueFromString`
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #35457:
URL: https://github.com/apache/arrow/pull/35457#discussion_r1187611874
##########
.gitignore:
##########
@@ -91,4 +91,6 @@ java-native-cpp/
# archery files
dev/archery/build
-swift/Arrow/.build
\ No newline at end of file
+swift/Arrow/.build
+
+go/vendor
Review Comment:
You're correct that vendoring and committing them isn't required. I'm more confused why you ran `go mod vendor` in the first place :smile: There's already the go module cache so why did you need to run `go mod vendor`?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] candiduslynx commented on a diff in pull request #35457: GH-35421: [Go] Ensure interface contract between `array.X.ValueStr` & `array.XBuilder.AppendValueFromString`
Posted by "candiduslynx (via GitHub)" <gi...@apache.org>.
candiduslynx commented on code in PR #35457:
URL: https://github.com/apache/arrow/pull/35457#discussion_r1187636494
##########
go/internal/types/extension_types.go:
##########
@@ -32,26 +32,45 @@ import (
"golang.org/x/xerrors"
)
+var UUID = NewUUIDType()
+
type UUIDBuilder struct {
*array.ExtensionBuilder
}
-func NewUUIDBuilder(bldr *array.ExtensionBuilder) *UUIDBuilder {
- b := &UUIDBuilder{
- ExtensionBuilder: bldr,
- }
- return b
+func NewUUIDBuilder(builder *array.ExtensionBuilder) *UUIDBuilder {
+ return &UUIDBuilder{ExtensionBuilder: builder}
}
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) AppendValueFromString(s string) error {
+ if s == array.NullValueStr {
+ b.AppendNull()
+ return nil
+ }
+
+ b.Append(uuid.MustParse(s))
Review Comment:
Done in 3643d9b61176744c84e0061bc2e99815f659e3db
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] candiduslynx commented on a diff in pull request #35457: GH-35421: [Go] Ensure interface contract between `array.X.ValueStr` & `array.XBuilder.AppendValueFromString`
Posted by "candiduslynx (via GitHub)" <gi...@apache.org>.
candiduslynx commented on code in PR #35457:
URL: https://github.com/apache/arrow/pull/35457#discussion_r1187641029
##########
go/arrow/array/numericbuilder.gen.go:
##########
@@ -3534,7 +3534,29 @@ func (b *DurationBuilder) AppendValueFromString(s string) error {
b.AppendNull()
return nil
}
- return fmt.Errorf("%w: AppendValueFromString not implemented for Duration", arrow.ErrNotImplemented)
+ var value arrow.Duration
+ var unitStr string
+ n, err := fmt.Sscanf(s, "%d%s", &value, &unitStr)
+ if err != nil {
+ b.AppendNull()
+ return err
+ }
+ if n != 2 {
+ b.AppendNull()
+ return fmt.Errorf("failed to parse Duration from %q", s)
+ }
+
+ // to ns & back
+ unit, err := arrow.TimeUnitFromString(unitStr)
+ if err != nil {
+ b.AppendNull()
+ return err
+ }
+
+ value *= arrow.Duration(unit.Multiplier())
+ value /= arrow.Duration(b.dtype.Unit.Multiplier())
+
+ b.Append(value)
Review Comment:
Done in d16fc4cdd02fcfed26de2b76dfcd3fb14635b260
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] candiduslynx commented on a diff in pull request #35457: GH-35421: [Go] Ensure interface contract between `array.X.ValueStr` & `array.XBuilder.AppendValueFromString`
Posted by "candiduslynx (via GitHub)" <gi...@apache.org>.
candiduslynx commented on code in PR #35457:
URL: https://github.com/apache/arrow/pull/35457#discussion_r1187634887
##########
go/arrow/array/array.go:
##########
@@ -27,7 +27,7 @@ import (
const (
// UnknownNullCount specifies the NullN should be calculated from the null bitmap buffer.
UnknownNullCount = -1
- NullValueStr = "(null)"
+ NullValueStr = "(null)"
Review Comment:
Done in 92ed97ce5b7782443655a344c916f8668a688092
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] candiduslynx commented on a diff in pull request #35457: GH-35421: [Go] Ensure interface contract between `array.X.ValueStr` & `array.XBuilder.AppendValueFromString`
Posted by "candiduslynx (via GitHub)" <gi...@apache.org>.
candiduslynx commented on code in PR #35457:
URL: https://github.com/apache/arrow/pull/35457#discussion_r1187748838
##########
go/arrow/datatype_fixedwidth.go:
##########
@@ -325,6 +325,22 @@ func (u TimeUnit) Multiplier() time.Duration {
func (u TimeUnit) String() string { return [...]string{"s", "ms", "us", "ns"}[uint(u)&3] }
+var _strToTimeUnit = map[string]TimeUnit{
+ "s": Second,
+ "ms": Millisecond,
+ "us": Microsecond,
+ "ns": Nanosecond,
+}
+
+// TimeUnitFromString is the reverse of TimeUnit.String
+func TimeUnitFromString(s string) (TimeUnit, error) {
+ if v, ok := _strToTimeUnit[s]; ok {
+ return v, nil
+ }
+
+ return 0, fmt.Errorf("unsupported time unit: %q", s)
+}
Review Comment:
Removed in 2bf65c58573f6ddbdc2e0f6fed13dd79516e6bac
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #35457: GH-35421: [Go] Ensure interface contract between `array.X.ValueStr` & `array.XBuilder.AppendValueFromString`
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #35457:
URL: https://github.com/apache/arrow/pull/35457#issuecomment-1539449831
Benchmark runs are scheduled for baseline = b211c1896916e3d0bbad1ffc88a30ddb9c90a5f0 and contender = 05a57dece1d093281fb04e4a0f9a1289ad4ed325. 05a57dece1d093281fb04e4a0f9a1289ad4ed325 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/0d890abb5ba445d6ab7876317d1da025...96a980bce7ab4e448e360a8408891ce6/)
[Failed :arrow_down:4.44% :arrow_up:0.0%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/a9c6baa5522547a79724e41d0b0a6006...9b9f7dae1e27445fac9f54e8c814677d/)
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/90dd37ec63614570b94f7ffabdf59452...68ccb25023814d3cacc9eef22fd6a1cd/)
[Failed :arrow_down:0.0% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/ad5f5f2da12c42d890bae5aac1dd7b42...6e4ddb403d03439db46ad59fb407fe1e/)
Buildkite builds:
[Finished] [`05a57dec` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2830)
[Failed] [`05a57dec` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2867)
[Finished] [`05a57dec` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2830)
[Failed] [`05a57dec` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2857)
[Finished] [`b211c189` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2829)
[Failed] [`b211c189` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2866)
[Finished] [`b211c189` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2829)
[Failed] [`b211c189` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2856)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #35457: GH-35421: [Go] Ensure interface contract between `array.X.ValueStr` & `array.XBuilder.AppendValueFromString`
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #35457:
URL: https://github.com/apache/arrow/pull/35457#issuecomment-1539450085
['Python', 'R'] benchmarks have high level of regressions.
[test-mac-arm](https://conbench.ursa.dev/compare/runs/a9c6baa5522547a79724e41d0b0a6006...9b9f7dae1e27445fac9f54e8c814677d/)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] candiduslynx commented on a diff in pull request #35457: GH-35421: [Go] Ensure interface contract between `array.X.ValueStr` & `array.XBuilder.AppendValueFromString`
Posted by "candiduslynx (via GitHub)" <gi...@apache.org>.
candiduslynx commented on code in PR #35457:
URL: https://github.com/apache/arrow/pull/35457#discussion_r1187605623
##########
go/internal/types/extension_types.go:
##########
@@ -208,37 +199,49 @@ type UUIDType struct {
// NewUUIDType is a convenience function to create an instance of UuidType
// with the correct storage type
func NewUUIDType() *UUIDType {
- return &UUIDType{
- ExtensionBase: arrow.ExtensionBase{
- Storage: &arrow.FixedSizeBinaryType{ByteWidth: 16}}}
+ return &UUIDType{ExtensionBase: arrow.ExtensionBase{Storage: &arrow.FixedSizeBinaryType{ByteWidth: 16}}}
}
-// ArrayType returns TypeOf(UuidArray) for constructing uuid arrays
-func (UUIDType) ArrayType() reflect.Type { return reflect.TypeOf(UUIDArray{}) }
+// ArrayType returns TypeOf(UUIDArray{}) for constructing UUID arrays
+func (*UUIDType) ArrayType() reflect.Type {
+ return reflect.TypeOf(UUIDArray{})
+}
+
+func (*UUIDType) ExtensionName() string {
+ return "uuid"
+}
+
+func (e *UUIDType) String() string {
+ return fmt.Sprintf("extension_type<storage=%s>", e.Storage)
+}
-func (UUIDType) ExtensionName() string { return "uuid" }
+func (e *UUIDType) MarshalJSON() ([]byte, error) {
+ return []byte(fmt.Sprintf(`{"name":"%s","metadata":%s}`, e.ExtensionName(), e.Serialize())), nil
+}
// Serialize returns "uuid-serialized" for testing proper metadata passing
-func (UUIDType) Serialize() string { return "uuid-serialized" }
+func (*UUIDType) Serialize() string {
+ return "uuid-serialized"
+}
// Deserialize expects storageType to be FixedSizeBinaryType{ByteWidth: 16} and the data to be
-// "uuid-serialized" in order to correctly create a UUIDType for testing deserialize.
-func (UUIDType) Deserialize(storageType arrow.DataType, data string) (arrow.ExtensionType, error) {
- if string(data) != "uuid-serialized" {
- return nil, fmt.Errorf("type identifier did not match: '%s'", string(data))
+// "uuid-serialized" in order to correctly create a UuidType for testing deserialize.
+func (*UUIDType) Deserialize(storageType arrow.DataType, data string) (arrow.ExtensionType, error) {
+ if data != "uuid-serialized" {
+ return nil, fmt.Errorf("type identifier did not match: '%s'", data)
}
if !arrow.TypeEqual(storageType, &arrow.FixedSizeBinaryType{ByteWidth: 16}) {
- return nil, fmt.Errorf("invalid storage type for UUIDType: %s", storageType.Name())
+ return nil, fmt.Errorf("invalid storage type for UuidType: %s", storageType.Name())
}
return NewUUIDType(), nil
}
-// UuidTypes are equal if both are named "uuid"
-func (u UUIDType) ExtensionEquals(other arrow.ExtensionType) bool {
- return u.ExtensionName() == other.ExtensionName()
+// ExtensionEquals returns true if both extensions have the same name
+func (e *UUIDType) ExtensionEquals(other arrow.ExtensionType) bool {
Review Comment:
consistency only
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] candiduslynx commented on a diff in pull request #35457: GH-35421: [Go] Ensure interface contract between `array.X.ValueStr` & `array.XBuilder.AppendValueFromString`
Posted by "candiduslynx (via GitHub)" <gi...@apache.org>.
candiduslynx commented on code in PR #35457:
URL: https://github.com/apache/arrow/pull/35457#discussion_r1187602176
##########
go/arrow/array/numericbuilder.gen.go.tmpl:
##########
@@ -203,6 +203,21 @@ func (b *{{.Name}}Builder) AppendValueFromString(s string) error {
return err
}
b.Append(arrow.Date64FromTime(tm))
+ {{else if or (eq .Name "Dictionary") -}}
Review Comment:
Yep, seems like it. I'll remove the references for dictionary
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] zeroshade commented on a diff in pull request #35457: GH-35421: [Go] Ensure interface contract between `array.X.ValueStr` & `array.XBuilder.AppendValueFromString`
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #35457:
URL: https://github.com/apache/arrow/pull/35457#discussion_r1187739558
##########
go/arrow/datatype_fixedwidth.go:
##########
@@ -325,6 +325,22 @@ func (u TimeUnit) Multiplier() time.Duration {
func (u TimeUnit) String() string { return [...]string{"s", "ms", "us", "ns"}[uint(u)&3] }
+var _strToTimeUnit = map[string]TimeUnit{
+ "s": Second,
+ "ms": Millisecond,
+ "us": Microsecond,
+ "ns": Nanosecond,
+}
+
+// TimeUnitFromString is the reverse of TimeUnit.String
+func TimeUnitFromString(s string) (TimeUnit, error) {
+ if v, ok := _strToTimeUnit[s]; ok {
+ return v, nil
+ }
+
+ return 0, fmt.Errorf("unsupported time unit: %q", s)
+}
Review Comment:
is this in use anymore? Is it worthwhile to keep?
##########
go/internal/types/extension_types.go:
##########
@@ -82,14 +105,6 @@ func (b *UUIDBuilder) UnmarshalOne(dec *json.Decoder) error {
}
}
- if len(val) != 16 {
- return &json.UnmarshalTypeError{
- Value: fmt.Sprint(val),
- Type: reflect.TypeOf([]byte{}),
- Offset: dec.InputOffset(),
- Struct: fmt.Sprintf("FixedSizeBinary[%d]", 16),
- }
- }
Review Comment:
Just to confirm, basically the calls to `uuid.Parse` and `uuid.ParseBytes` do this validation for us?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] candiduslynx commented on pull request #35457: GH-35421: [Go] Ensure interface contract between `array.X.ValueStr` & `array.XBuilder.AppendValueFromString`
Posted by "candiduslynx (via GitHub)" <gi...@apache.org>.
candiduslynx commented on PR #35457:
URL: https://github.com/apache/arrow/pull/35457#issuecomment-1537068133
@zeroshade I see that some tests fail due to unavailability of generics (Go 1.17).
Is this something critical to move to non-generics implementation?
I assumed generics would be fine considering Go 1.20 targeted in go.mod
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] candiduslynx commented on a diff in pull request #35457: GH-35421: [Go] Ensure interface contract between `array.X.ValueStr` & `array.XBuilder.AppendValueFromString`
Posted by "candiduslynx (via GitHub)" <gi...@apache.org>.
candiduslynx commented on code in PR #35457:
URL: https://github.com/apache/arrow/pull/35457#discussion_r1187600184
##########
.gitignore:
##########
@@ -91,4 +91,6 @@ java-native-cpp/
# archery files
dev/archery/build
-swift/Arrow/.build
\ No newline at end of file
+swift/Arrow/.build
+
+go/vendor
Review Comment:
I used `go mod vendor` and it introduced a lot of files to stage.
I presume vendoring the dependencies and committing them into the repo isn't required.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] candiduslynx commented on a diff in pull request #35457: GH-35421: [Go] Ensure interface contract between `array.X.ValueStr` & `array.XBuilder.AppendValueFromString`
Posted by "candiduslynx (via GitHub)" <gi...@apache.org>.
candiduslynx commented on code in PR #35457:
URL: https://github.com/apache/arrow/pull/35457#discussion_r1187630328
##########
go/internal/types/extension_types.go:
##########
@@ -157,46 +147,47 @@ func (a UUIDArray) String() string {
}
switch {
case a.IsNull(i):
- o.WriteString("(null)")
+ o.WriteString(array.NullValueStr)
default:
- uuidStr, err := uuid.FromBytes(arr.Value(i))
- if err != nil {
- panic(fmt.Errorf("invalid uuid: %w", err))
- }
- fmt.Fprintf(o, "%q", uuidStr)
+ fmt.Fprintf(o, "%q", a.Value(i))
}
}
o.WriteString("]")
return o.String()
}
+func (a *UUIDArray) Value(i int) uuid.UUID {
+ if a.IsNull(i) {
+ return uuid.Nil
+ }
+ return uuid.Must(uuid.FromBytes(a.Storage().(*array.FixedSizeBinary).Value(i)))
+}
+
+func (a *UUIDArray) ValueStr(i int) string {
+ switch {
+ case a.IsNull(i):
+ return array.NullValueStr
+ default:
+ return a.Value(i).String()
+ }
+}
+
func (a *UUIDArray) MarshalJSON() ([]byte, error) {
arr := a.Storage().(*array.FixedSizeBinary)
- vals := make([]interface{}, a.Len())
+ values := make([]interface{}, a.Len())
for i := 0; i < a.Len(); i++ {
if a.IsValid(i) {
- uuidStr, err := uuid.FromBytes(arr.Value(i))
- if err != nil {
- panic(fmt.Errorf("invalid uuid: %w", err))
- }
- vals[i] = uuidStr
- } else {
- vals[i] = nil
+ values[i] = uuid.Must(uuid.FromBytes(arr.Value(i))).String()
}
}
- return json.Marshal(vals)
+ return json.Marshal(values)
}
func (a *UUIDArray) GetOneForMarshal(i int) interface{} {
- arr := a.Storage().(*array.FixedSizeBinary)
- if a.IsValid(i) {
- uuidObj, err := uuid.FromBytes(arr.Value(i))
- if err != nil {
- panic(fmt.Errorf("invalid uuid: %w", err))
- }
- return uuidObj
+ if a.IsNull(i) {
+ return nil
}
- return nil
+ return uuid.Must(uuid.FromBytes(a.Storage().(*array.FixedSizeBinary).Value(i)))
Review Comment:
Done in 3e314138add5174886bb3c7c507d7ac0ae4f104b
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] candiduslynx commented on a diff in pull request #35457: GH-35421: [Go] Ensure interface contract between `array.X.ValueStr` & `array.XBuilder.AppendValueFromString`
Posted by "candiduslynx (via GitHub)" <gi...@apache.org>.
candiduslynx commented on code in PR #35457:
URL: https://github.com/apache/arrow/pull/35457#discussion_r1187622750
##########
go/arrow/array/numericbuilder.gen.go.tmpl:
##########
@@ -203,6 +203,21 @@ func (b *{{.Name}}Builder) AppendValueFromString(s string) error {
return err
}
b.Append(arrow.Date64FromTime(tm))
+ {{else if or (eq .Name "Dictionary") -}}
Review Comment:
removed in 720ce704e0b6fa4f6bd2f063bdae0a6b94d34b3f
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #35457: GH-35421: [Go] Ensure interface contract between `array.X.ValueStr` & `array.XBuilder.AppendValueFromString`
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35457:
URL: https://github.com/apache/arrow/pull/35457#issuecomment-1537066002
:warning: GitHub issue #35421 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] zeroshade commented on a diff in pull request #35457: GH-35421: [Go] Ensure interface contract between `array.X.ValueStr` & `array.XBuilder.AppendValueFromString`
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #35457:
URL: https://github.com/apache/arrow/pull/35457#discussion_r1187545816
##########
go/arrow/array/dictionary.go:
##########
@@ -742,11 +745,29 @@ func (b *dictionaryBuilder) Unmarshal(dec *json.Decoder) error {
}
func (b *dictionaryBuilder) AppendValueFromString(s string) error {
Review Comment:
you should update the comment on the `UnmarshalJSON` method to indicate that it will no longer error.
##########
go/arrow/array/encoded.go:
##########
@@ -280,7 +280,10 @@ type RunEndEncodedBuilder struct {
values Builder
maxRunEnd uint64
+ // currently, mixing AppendValueFromString & UnmarshalOne is unsupported
Review Comment:
add this to the docstring of both AppendValueFromString and UnmarshalOne please.
##########
go/arrow/array/numericbuilder.gen.go.tmpl:
##########
@@ -203,6 +203,21 @@ func (b *{{.Name}}Builder) AppendValueFromString(s string) error {
return err
}
b.Append(arrow.Date64FromTime(tm))
+ {{else if or (eq .Name "Dictionary") -}}
+ if d.IsNull(i) {
+ return NullValueStr
+ }
+ return d.Dictionary().ValueStr(d.GetValueIndex(i))
Review Comment:
this return can't be in the right spot... either that or everything below this is wrong
##########
go/arrow/array/numericbuilder.gen.go:
##########
@@ -3534,7 +3534,29 @@ func (b *DurationBuilder) AppendValueFromString(s string) error {
b.AppendNull()
return nil
}
- return fmt.Errorf("%w: AppendValueFromString not implemented for Duration", arrow.ErrNotImplemented)
+ var value arrow.Duration
+ var unitStr string
+ n, err := fmt.Sscanf(s, "%d%s", &value, &unitStr)
+ if err != nil {
+ b.AppendNull()
+ return err
+ }
+ if n != 2 {
+ b.AppendNull()
+ return fmt.Errorf("failed to parse Duration from %q", s)
+ }
+
+ // to ns & back
+ unit, err := arrow.TimeUnitFromString(unitStr)
+ if err != nil {
+ b.AppendNull()
+ return err
+ }
+
+ value *= arrow.Duration(unit.Multiplier())
+ value /= arrow.Duration(b.dtype.Unit.Multiplier())
+
+ b.Append(value)
Review Comment:
can you just use `https://pkg.go.dev/time#ParseDuration` ?
##########
go/arrow/array/union.go:
##########
@@ -1110,10 +1133,15 @@ func NewEmptyDenseUnionBuilder(mem memory.Allocator) *DenseUnionBuilder {
// children and type codes. Builders will be constructed for each child
// using the fields in typ
func NewDenseUnionBuilder(mem memory.Allocator, typ *arrow.DenseUnionType) *DenseUnionBuilder {
- children := make([]Builder, len(typ.Fields()))
- for i, f := range typ.Fields() {
- children[i] = NewBuilder(mem, f.Type)
- defer children[i].Release()
+ children := make([]Builder, 0, len(typ.Fields()))
+ defer func() {
+ for _, child := range children {
+ child.Release()
+ }
+ }()
Review Comment:
is this just to condense everything into a single defer rather than multiple defers?
##########
go/internal/types/extension_types.go:
##########
@@ -82,14 +105,6 @@ func (b *UUIDBuilder) UnmarshalOne(dec *json.Decoder) error {
}
}
- if len(val) != 16 {
- return &json.UnmarshalTypeError{
- Value: fmt.Sprint(val),
- Type: reflect.TypeOf([]byte{}),
- Offset: dec.InputOffset(),
- Struct: fmt.Sprintf("FixedSizeBinary[%d]", 16),
- }
- }
Review Comment:
Why remove this?
##########
go/arrow/array/array.go:
##########
@@ -27,7 +27,7 @@ import (
const (
// UnknownNullCount specifies the NullN should be calculated from the null bitmap buffer.
UnknownNullCount = -1
- NullValueStr = "(null)"
+ NullValueStr = "(null)"
Review Comment:
Can you add a docstring here since now this is getting used as the official string expected for appending a null if you are using `AppendValueFromString`, so we should give this a docstring for the documentation to explain this.
##########
go/internal/types/extension_types.go:
##########
@@ -208,37 +199,49 @@ type UUIDType struct {
// NewUUIDType is a convenience function to create an instance of UuidType
// with the correct storage type
func NewUUIDType() *UUIDType {
- return &UUIDType{
- ExtensionBase: arrow.ExtensionBase{
- Storage: &arrow.FixedSizeBinaryType{ByteWidth: 16}}}
+ return &UUIDType{ExtensionBase: arrow.ExtensionBase{Storage: &arrow.FixedSizeBinaryType{ByteWidth: 16}}}
}
-// ArrayType returns TypeOf(UuidArray) for constructing uuid arrays
-func (UUIDType) ArrayType() reflect.Type { return reflect.TypeOf(UUIDArray{}) }
+// ArrayType returns TypeOf(UUIDArray{}) for constructing UUID arrays
+func (*UUIDType) ArrayType() reflect.Type {
+ return reflect.TypeOf(UUIDArray{})
+}
+
+func (*UUIDType) ExtensionName() string {
+ return "uuid"
+}
+
+func (e *UUIDType) String() string {
+ return fmt.Sprintf("extension_type<storage=%s>", e.Storage)
+}
-func (UUIDType) ExtensionName() string { return "uuid" }
+func (e *UUIDType) MarshalJSON() ([]byte, error) {
+ return []byte(fmt.Sprintf(`{"name":"%s","metadata":%s}`, e.ExtensionName(), e.Serialize())), nil
+}
// Serialize returns "uuid-serialized" for testing proper metadata passing
-func (UUIDType) Serialize() string { return "uuid-serialized" }
+func (*UUIDType) Serialize() string {
+ return "uuid-serialized"
+}
// Deserialize expects storageType to be FixedSizeBinaryType{ByteWidth: 16} and the data to be
-// "uuid-serialized" in order to correctly create a UUIDType for testing deserialize.
-func (UUIDType) Deserialize(storageType arrow.DataType, data string) (arrow.ExtensionType, error) {
- if string(data) != "uuid-serialized" {
- return nil, fmt.Errorf("type identifier did not match: '%s'", string(data))
+// "uuid-serialized" in order to correctly create a UuidType for testing deserialize.
+func (*UUIDType) Deserialize(storageType arrow.DataType, data string) (arrow.ExtensionType, error) {
+ if data != "uuid-serialized" {
+ return nil, fmt.Errorf("type identifier did not match: '%s'", data)
}
if !arrow.TypeEqual(storageType, &arrow.FixedSizeBinaryType{ByteWidth: 16}) {
- return nil, fmt.Errorf("invalid storage type for UUIDType: %s", storageType.Name())
+ return nil, fmt.Errorf("invalid storage type for UuidType: %s", storageType.Name())
}
return NewUUIDType(), nil
}
-// UuidTypes are equal if both are named "uuid"
-func (u UUIDType) ExtensionEquals(other arrow.ExtensionType) bool {
- return u.ExtensionName() == other.ExtensionName()
+// ExtensionEquals returns true if both extensions have the same name
+func (e *UUIDType) ExtensionEquals(other arrow.ExtensionType) bool {
Review Comment:
again, is this change in the receiver just for consistency? or was there actually an issue?
##########
go/arrow/array/numericbuilder.gen.go.tmpl:
##########
@@ -203,6 +203,21 @@ func (b *{{.Name}}Builder) AppendValueFromString(s string) error {
return err
}
b.Append(arrow.Date64FromTime(tm))
+ {{else if or (eq .Name "Dictionary") -}}
Review Comment:
no need for the `or` here
##########
go/arrow/array/numericbuilder.gen.go.tmpl:
##########
@@ -203,6 +203,21 @@ func (b *{{.Name}}Builder) AppendValueFromString(s string) error {
return err
}
b.Append(arrow.Date64FromTime(tm))
+ {{else if or (eq .Name "Dictionary") -}}
Review Comment:
why don't I see these changes in the `numericbuilder.gen.go` file? do you need to re-run go generate? Also, I don't think Dictionary goes through this code path at all? Or am I wrong?
##########
.gitignore:
##########
@@ -91,4 +91,6 @@ java-native-cpp/
# archery files
dev/archery/build
-swift/Arrow/.build
\ No newline at end of file
+swift/Arrow/.build
+
+go/vendor
Review Comment:
why the addition here?
##########
go/internal/types/extension_types.go:
##########
@@ -32,26 +32,45 @@ import (
"golang.org/x/xerrors"
)
+var UUID = NewUUIDType()
+
type UUIDBuilder struct {
*array.ExtensionBuilder
}
-func NewUUIDBuilder(bldr *array.ExtensionBuilder) *UUIDBuilder {
- b := &UUIDBuilder{
- ExtensionBuilder: bldr,
- }
- return b
+func NewUUIDBuilder(builder *array.ExtensionBuilder) *UUIDBuilder {
+ return &UUIDBuilder{ExtensionBuilder: builder}
}
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) AppendValueFromString(s string) error {
+ if s == array.NullValueStr {
+ b.AppendNull()
+ return nil
+ }
+
+ b.Append(uuid.MustParse(s))
Review Comment:
this should use `uuid.Parse` and return the `err` from it rather than panic'ing by using `MustParse`
##########
go/arrow/example_test.go:
##########
@@ -252,7 +252,7 @@ func Example_fixedSizeListArray() {
// NullN() = 2
// Len() = 5
// Type() = fixed_size_list<item: int64>[3]
- // List = [[0 1 2] (null) [3 4 5] [6 7 8] (null)]
+ // List = [[0 1 2] (null) [-1 -1 -1] [3 4 5] (null)]
Review Comment:
Ah, I see what's going on here. The test should be modified and have the appending of -1's removed to maintain the same output. But should be shifted into a separate change/PR with the change to FixedSizeListBuilder.AppendNull which we can mark as a breaking change.
##########
go/arrow/array/fixed_size_list.go:
##########
@@ -207,6 +207,10 @@ func (b *FixedSizeListBuilder) Append(v bool) {
func (b *FixedSizeListBuilder) AppendNull() {
b.Reserve(1)
b.unsafeAppendBoolToBitmap(false)
+ // require to append this due to value indexes
+ for i := int32(0); i < b.n; i++ {
+ b.values.AppendNull()
+ }
Review Comment:
This is a breaking change, though I can agree with the sentiment in doing this since it'll make it easier to use FixedSizeList builders to avoid this gotcha.
Can this be done as a separate PR/change which we can mark as a breaking change?
##########
go/internal/types/extension_types.go:
##########
@@ -111,43 +126,18 @@ func (b *UUIDBuilder) UnmarshalJSON(data []byte) error {
}
if delim, ok := t.(json.Delim); !ok || delim != '[' {
- return fmt.Errorf("fixed size binary builder must unpack from json array, found %s", delim)
+ return fmt.Errorf("uuid builder must unpack from json array, found %s", delim)
}
return b.Unmarshal(dec)
}
-func (b *UUIDBuilder) AppendValueFromString(s string) error {
- if s == array.NullValueStr {
- b.AppendNull()
- return nil
- }
- u, err := uuid.Parse(s)
- if err != nil {
- return fmt.Errorf("%w: invalid uuid: %v", arrow.ErrInvalid, err)
- }
- b.Append(u)
- return nil
-}
Review Comment:
why are we shifting this around and replacing it with something that doesn't return the error?
##########
go/internal/types/extension_types.go:
##########
@@ -208,37 +199,49 @@ type UUIDType struct {
// NewUUIDType is a convenience function to create an instance of UuidType
// with the correct storage type
func NewUUIDType() *UUIDType {
- return &UUIDType{
- ExtensionBase: arrow.ExtensionBase{
- Storage: &arrow.FixedSizeBinaryType{ByteWidth: 16}}}
+ return &UUIDType{ExtensionBase: arrow.ExtensionBase{Storage: &arrow.FixedSizeBinaryType{ByteWidth: 16}}}
}
-// ArrayType returns TypeOf(UuidArray) for constructing uuid arrays
-func (UUIDType) ArrayType() reflect.Type { return reflect.TypeOf(UUIDArray{}) }
+// ArrayType returns TypeOf(UUIDArray{}) for constructing UUID arrays
+func (*UUIDType) ArrayType() reflect.Type {
+ return reflect.TypeOf(UUIDArray{})
+}
+
+func (*UUIDType) ExtensionName() string {
+ return "uuid"
+}
+
+func (e *UUIDType) String() string {
+ return fmt.Sprintf("extension_type<storage=%s>", e.Storage)
+}
-func (UUIDType) ExtensionName() string { return "uuid" }
+func (e *UUIDType) MarshalJSON() ([]byte, error) {
+ return []byte(fmt.Sprintf(`{"name":"%s","metadata":%s}`, e.ExtensionName(), e.Serialize())), nil
+}
// Serialize returns "uuid-serialized" for testing proper metadata passing
-func (UUIDType) Serialize() string { return "uuid-serialized" }
+func (*UUIDType) Serialize() string {
Review Comment:
is this change just for consistency of the receiver? or was there actually an issue?
##########
go/internal/types/extension_types.go:
##########
@@ -157,46 +147,47 @@ func (a UUIDArray) String() string {
}
switch {
case a.IsNull(i):
- o.WriteString("(null)")
+ o.WriteString(array.NullValueStr)
default:
- uuidStr, err := uuid.FromBytes(arr.Value(i))
- if err != nil {
- panic(fmt.Errorf("invalid uuid: %w", err))
- }
- fmt.Fprintf(o, "%q", uuidStr)
+ fmt.Fprintf(o, "%q", a.Value(i))
}
}
o.WriteString("]")
return o.String()
}
+func (a *UUIDArray) Value(i int) uuid.UUID {
+ if a.IsNull(i) {
+ return uuid.Nil
+ }
+ return uuid.Must(uuid.FromBytes(a.Storage().(*array.FixedSizeBinary).Value(i)))
+}
+
+func (a *UUIDArray) ValueStr(i int) string {
+ switch {
+ case a.IsNull(i):
+ return array.NullValueStr
+ default:
+ return a.Value(i).String()
+ }
+}
+
func (a *UUIDArray) MarshalJSON() ([]byte, error) {
arr := a.Storage().(*array.FixedSizeBinary)
- vals := make([]interface{}, a.Len())
+ values := make([]interface{}, a.Len())
for i := 0; i < a.Len(); i++ {
if a.IsValid(i) {
- uuidStr, err := uuid.FromBytes(arr.Value(i))
- if err != nil {
- panic(fmt.Errorf("invalid uuid: %w", err))
- }
- vals[i] = uuidStr
- } else {
- vals[i] = nil
+ values[i] = uuid.Must(uuid.FromBytes(arr.Value(i))).String()
}
}
- return json.Marshal(vals)
+ return json.Marshal(values)
}
func (a *UUIDArray) GetOneForMarshal(i int) interface{} {
- arr := a.Storage().(*array.FixedSizeBinary)
- if a.IsValid(i) {
- uuidObj, err := uuid.FromBytes(arr.Value(i))
- if err != nil {
- panic(fmt.Errorf("invalid uuid: %w", err))
- }
- return uuidObj
+ if a.IsNull(i) {
+ return nil
}
- return nil
+ return uuid.Must(uuid.FromBytes(a.Storage().(*array.FixedSizeBinary).Value(i)))
Review Comment:
couldn't this just be `a.Value(i)`?
##########
go/internal/types/extension_types.go:
##########
@@ -208,37 +199,49 @@ type UUIDType struct {
// NewUUIDType is a convenience function to create an instance of UuidType
// with the correct storage type
func NewUUIDType() *UUIDType {
- return &UUIDType{
- ExtensionBase: arrow.ExtensionBase{
- Storage: &arrow.FixedSizeBinaryType{ByteWidth: 16}}}
+ return &UUIDType{ExtensionBase: arrow.ExtensionBase{Storage: &arrow.FixedSizeBinaryType{ByteWidth: 16}}}
}
-// ArrayType returns TypeOf(UuidArray) for constructing uuid arrays
-func (UUIDType) ArrayType() reflect.Type { return reflect.TypeOf(UUIDArray{}) }
+// ArrayType returns TypeOf(UUIDArray{}) for constructing UUID arrays
+func (*UUIDType) ArrayType() reflect.Type {
+ return reflect.TypeOf(UUIDArray{})
+}
+
+func (*UUIDType) ExtensionName() string {
+ return "uuid"
+}
+
+func (e *UUIDType) String() string {
+ return fmt.Sprintf("extension_type<storage=%s>", e.Storage)
+}
-func (UUIDType) ExtensionName() string { return "uuid" }
+func (e *UUIDType) MarshalJSON() ([]byte, error) {
+ return []byte(fmt.Sprintf(`{"name":"%s","metadata":%s}`, e.ExtensionName(), e.Serialize())), nil
+}
// Serialize returns "uuid-serialized" for testing proper metadata passing
-func (UUIDType) Serialize() string { return "uuid-serialized" }
+func (*UUIDType) Serialize() string {
+ return "uuid-serialized"
+}
// Deserialize expects storageType to be FixedSizeBinaryType{ByteWidth: 16} and the data to be
-// "uuid-serialized" in order to correctly create a UUIDType for testing deserialize.
-func (UUIDType) Deserialize(storageType arrow.DataType, data string) (arrow.ExtensionType, error) {
- if string(data) != "uuid-serialized" {
- return nil, fmt.Errorf("type identifier did not match: '%s'", string(data))
+// "uuid-serialized" in order to correctly create a UuidType for testing deserialize.
+func (*UUIDType) Deserialize(storageType arrow.DataType, data string) (arrow.ExtensionType, error) {
+ if data != "uuid-serialized" {
+ return nil, fmt.Errorf("type identifier did not match: '%s'", data)
}
if !arrow.TypeEqual(storageType, &arrow.FixedSizeBinaryType{ByteWidth: 16}) {
- return nil, fmt.Errorf("invalid storage type for UUIDType: %s", storageType.Name())
+ return nil, fmt.Errorf("invalid storage type for UuidType: %s", storageType.Name())
Review Comment:
this change is incorrect.
##########
go/internal/types/extension_types.go:
##########
@@ -208,37 +199,49 @@ type UUIDType struct {
// NewUUIDType is a convenience function to create an instance of UuidType
// with the correct storage type
func NewUUIDType() *UUIDType {
- return &UUIDType{
- ExtensionBase: arrow.ExtensionBase{
- Storage: &arrow.FixedSizeBinaryType{ByteWidth: 16}}}
+ return &UUIDType{ExtensionBase: arrow.ExtensionBase{Storage: &arrow.FixedSizeBinaryType{ByteWidth: 16}}}
}
-// ArrayType returns TypeOf(UuidArray) for constructing uuid arrays
-func (UUIDType) ArrayType() reflect.Type { return reflect.TypeOf(UUIDArray{}) }
+// ArrayType returns TypeOf(UUIDArray{}) for constructing UUID arrays
+func (*UUIDType) ArrayType() reflect.Type {
+ return reflect.TypeOf(UUIDArray{})
+}
+
+func (*UUIDType) ExtensionName() string {
+ return "uuid"
+}
+
+func (e *UUIDType) String() string {
+ return fmt.Sprintf("extension_type<storage=%s>", e.Storage)
+}
-func (UUIDType) ExtensionName() string { return "uuid" }
+func (e *UUIDType) MarshalJSON() ([]byte, error) {
+ return []byte(fmt.Sprintf(`{"name":"%s","metadata":%s}`, e.ExtensionName(), e.Serialize())), nil
+}
// Serialize returns "uuid-serialized" for testing proper metadata passing
-func (UUIDType) Serialize() string { return "uuid-serialized" }
+func (*UUIDType) Serialize() string {
+ return "uuid-serialized"
+}
// Deserialize expects storageType to be FixedSizeBinaryType{ByteWidth: 16} and the data to be
-// "uuid-serialized" in order to correctly create a UUIDType for testing deserialize.
-func (UUIDType) Deserialize(storageType arrow.DataType, data string) (arrow.ExtensionType, error) {
- if string(data) != "uuid-serialized" {
- return nil, fmt.Errorf("type identifier did not match: '%s'", string(data))
+// "uuid-serialized" in order to correctly create a UuidType for testing deserialize.
Review Comment:
this correction is incorrect, the type is called `UUIDType` and the comment should reflect that.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] candiduslynx commented on a diff in pull request #35457: GH-35421: [Go] Ensure interface contract between `array.X.ValueStr` & `array.XBuilder.AppendValueFromString`
Posted by "candiduslynx (via GitHub)" <gi...@apache.org>.
candiduslynx commented on code in PR #35457:
URL: https://github.com/apache/arrow/pull/35457#discussion_r1187601530
##########
go/arrow/array/numericbuilder.gen.go:
##########
@@ -3534,7 +3534,29 @@ func (b *DurationBuilder) AppendValueFromString(s string) error {
b.AppendNull()
return nil
}
- return fmt.Errorf("%w: AppendValueFromString not implemented for Duration", arrow.ErrNotImplemented)
+ var value arrow.Duration
+ var unitStr string
+ n, err := fmt.Sscanf(s, "%d%s", &value, &unitStr)
+ if err != nil {
+ b.AppendNull()
+ return err
+ }
+ if n != 2 {
+ b.AppendNull()
+ return fmt.Errorf("failed to parse Duration from %q", s)
+ }
+
+ // to ns & back
+ unit, err := arrow.TimeUnitFromString(unitStr)
+ if err != nil {
+ b.AppendNull()
+ return err
+ }
+
+ value *= arrow.Duration(unit.Multiplier())
+ value /= arrow.Duration(b.dtype.Unit.Multiplier())
+
+ b.Append(value)
Review Comment:
Possibly, I could (as the ValueStr should only provide the `ns, us, ms, s` suffixes. I'll give it a go
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] candiduslynx commented on a diff in pull request #35457: GH-35421: [Go] Ensure interface contract between `array.X.ValueStr` & `array.XBuilder.AppendValueFromString`
Posted by "candiduslynx (via GitHub)" <gi...@apache.org>.
candiduslynx commented on code in PR #35457:
URL: https://github.com/apache/arrow/pull/35457#discussion_r1187636135
##########
go/internal/types/extension_types.go:
##########
@@ -111,43 +126,18 @@ func (b *UUIDBuilder) UnmarshalJSON(data []byte) error {
}
if delim, ok := t.(json.Delim); !ok || delim != '[' {
- return fmt.Errorf("fixed size binary builder must unpack from json array, found %s", delim)
+ return fmt.Errorf("uuid builder must unpack from json array, found %s", delim)
}
return b.Unmarshal(dec)
}
-func (b *UUIDBuilder) AppendValueFromString(s string) error {
- if s == array.NullValueStr {
- b.AppendNull()
- return nil
- }
- u, err := uuid.Parse(s)
- if err != nil {
- return fmt.Errorf("%w: invalid uuid: %v", arrow.ErrInvalid, err)
- }
- b.Append(u)
- return nil
-}
Review Comment:
Fixed in 3643d9b61176744c84e0061bc2e99815f659e3db
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] candiduslynx commented on a diff in pull request #35457: GH-35421: [Go] Ensure interface contract between `array.X.ValueStr` & `array.XBuilder.AppendValueFromString`
Posted by "candiduslynx (via GitHub)" <gi...@apache.org>.
candiduslynx commented on code in PR #35457:
URL: https://github.com/apache/arrow/pull/35457#discussion_r1187682008
##########
go/arrow/example_test.go:
##########
@@ -252,7 +252,7 @@ func Example_fixedSizeListArray() {
// NullN() = 2
// Len() = 5
// Type() = fixed_size_list<item: int64>[3]
- // List = [[0 1 2] (null) [3 4 5] [6 7 8] (null)]
+ // List = [[0 1 2] (null) [-1 -1 -1] [3 4 5] (null)]
Review Comment:
Rebased
##########
go/arrow/array/fixed_size_list.go:
##########
@@ -207,6 +207,10 @@ func (b *FixedSizeListBuilder) Append(v bool) {
func (b *FixedSizeListBuilder) AppendNull() {
b.Reserve(1)
b.unsafeAppendBoolToBitmap(false)
+ // require to append this due to value indexes
+ for i := int32(0); i < b.n; i++ {
+ b.values.AppendNull()
+ }
Review Comment:
rebased
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #35457: GH-35421: [Go] Ensure interface contract between `array.X.ValueStr` & `array.XBuilder.AppendValueFromString`
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35457:
URL: https://github.com/apache/arrow/pull/35457#issuecomment-1552913644
Revision: 2bf65c58573f6ddbdc2e0f6fed13dd79516e6bac
Submitted crossbow builds: [ursacomputing/crossbow @ actions-0f9abaae22](https://github.com/ursacomputing/crossbow/branches/all?query=actions-0f9abaae22)
|Task|Status|
|----|------|
|verify-rc-source-integration-linux-almalinux-8-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/5013433244/jobs/8986501566)|
|verify-rc-source-integration-linux-conda-latest-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/5013432914/jobs/8986500802)|
|verify-rc-source-integration-linux-ubuntu-20.04-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/5013432784/jobs/8986500534)|
|verify-rc-source-integration-linux-ubuntu-22.04-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/5013432588/jobs/8986500288)|
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] raulcd commented on pull request #35457: GH-35421: [Go] Ensure interface contract between `array.X.ValueStr` & `array.XBuilder.AppendValueFromString`
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on PR #35457:
URL: https://github.com/apache/arrow/pull/35457#issuecomment-1552911481
@github-actions crossbow submit verify-rc-source-integration-linux*
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #35457: GH-35421: [Go] Ensure interface contract between `array.X.ValueStr` & `array.XBuilder.AppendValueFromString`
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35457:
URL: https://github.com/apache/arrow/pull/35457#issuecomment-1537065995
* Closes: #35421
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] candiduslynx commented on a diff in pull request #35457: GH-35421: [Go] Ensure interface contract between `array.X.ValueStr` & `array.XBuilder.AppendValueFromString`
Posted by "candiduslynx (via GitHub)" <gi...@apache.org>.
candiduslynx commented on code in PR #35457:
URL: https://github.com/apache/arrow/pull/35457#discussion_r1187605102
##########
go/internal/types/extension_types.go:
##########
@@ -208,37 +199,49 @@ type UUIDType struct {
// NewUUIDType is a convenience function to create an instance of UuidType
// with the correct storage type
func NewUUIDType() *UUIDType {
- return &UUIDType{
- ExtensionBase: arrow.ExtensionBase{
- Storage: &arrow.FixedSizeBinaryType{ByteWidth: 16}}}
+ return &UUIDType{ExtensionBase: arrow.ExtensionBase{Storage: &arrow.FixedSizeBinaryType{ByteWidth: 16}}}
}
-// ArrayType returns TypeOf(UuidArray) for constructing uuid arrays
-func (UUIDType) ArrayType() reflect.Type { return reflect.TypeOf(UUIDArray{}) }
+// ArrayType returns TypeOf(UUIDArray{}) for constructing UUID arrays
+func (*UUIDType) ArrayType() reflect.Type {
+ return reflect.TypeOf(UUIDArray{})
+}
+
+func (*UUIDType) ExtensionName() string {
+ return "uuid"
+}
+
+func (e *UUIDType) String() string {
+ return fmt.Sprintf("extension_type<storage=%s>", e.Storage)
+}
-func (UUIDType) ExtensionName() string { return "uuid" }
+func (e *UUIDType) MarshalJSON() ([]byte, error) {
+ return []byte(fmt.Sprintf(`{"name":"%s","metadata":%s}`, e.ExtensionName(), e.Serialize())), nil
+}
// Serialize returns "uuid-serialized" for testing proper metadata passing
-func (UUIDType) Serialize() string { return "uuid-serialized" }
+func (*UUIDType) Serialize() string {
Review Comment:
just for the consistency
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] candiduslynx commented on pull request #35457: GH-35421: [Go] Ensure interface contract between `array.X.ValueStr` & `array.XBuilder.AppendValueFromString`
Posted by "candiduslynx (via GitHub)" <gi...@apache.org>.
candiduslynx commented on PR #35457:
URL: https://github.com/apache/arrow/pull/35457#issuecomment-1538894655
@zeroshade the checks are finally done (I wonder if the largest one (conda) has to run the whole test suite, 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] candiduslynx commented on a diff in pull request #35457: GH-35421: [Go] Ensure interface contract between `array.X.ValueStr` & `array.XBuilder.AppendValueFromString`
Posted by "candiduslynx (via GitHub)" <gi...@apache.org>.
candiduslynx commented on code in PR #35457:
URL: https://github.com/apache/arrow/pull/35457#discussion_r1187623231
##########
go/arrow/array/numericbuilder.gen.go.tmpl:
##########
@@ -203,6 +203,21 @@ func (b *{{.Name}}Builder) AppendValueFromString(s string) error {
return err
}
b.Append(arrow.Date64FromTime(tm))
+ {{else if or (eq .Name "Dictionary") -}}
+ if d.IsNull(i) {
+ return NullValueStr
+ }
+ return d.Dictionary().ValueStr(d.GetValueIndex(i))
Review Comment:
removed in 720ce704e0b6fa4f6bd2f063bdae0a6b94d34b3f
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] candiduslynx commented on a diff in pull request #35457: GH-35421: [Go] Ensure interface contract between `array.X.ValueStr` & `array.XBuilder.AppendValueFromString`
Posted by "candiduslynx (via GitHub)" <gi...@apache.org>.
candiduslynx commented on code in PR #35457:
URL: https://github.com/apache/arrow/pull/35457#discussion_r1187627107
##########
go/arrow/array/encoded.go:
##########
@@ -280,7 +280,10 @@ type RunEndEncodedBuilder struct {
values Builder
maxRunEnd uint64
+ // currently, mixing AppendValueFromString & UnmarshalOne is unsupported
Review Comment:
Done in 86050d4bcbb48a5f29ed6a61bea708f7847f3431
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org