Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Go] Some types don't follow the ValueStr <-> AppendValueFromStr convention #35421

Closed
candiduslynx opened this issue May 3, 2023 · 1 comment · Fixed by #35457
Closed

[Go] Some types don't follow the ValueStr <-> AppendValueFromStr convention #35421

candiduslynx opened this issue May 3, 2023 · 1 comment · Fixed by #35457
Assignees
@candiduslynx
Labels
Component: Go Type: bug
Milestone
13.0.0

Comments

@candiduslynx
Copy link
Contributor

Describe the bug, including details regarding any error messages, version, and platform.

It's stated in the array.Builder interface description that AppendValueFromString adds a new value from a string. Inverse of array.ValueStr(i int) string.

However, some types fail to adhere to this rule.
One example I found was array.DayTimeInterval (along with array.DayTimeIntervalBuilder):

The issue was likely introduced in #34986

Component(s)

Go
@candiduslynx
Copy link
Contributor Author

I think to properly fix this the tests for Append/ValueStr should be added

@github-actions github-actions bot mentioned this issue May 6, 2023
Merged
@zeroshade zeroshade added this to the 13.0.0 milestone May 8, 2023
liujiacheng777 pushed a commit to LoongArch-Python/arrow that referenced this issue May 11, 2023
@candiduslynx @liujiacheng777
apacheGH-35421: [Go] Ensure interface contract between `array.X.Value…
8b95025 
…Str` & `array.XBuilder.AppendValueFromString` (apache#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 cloudquery/cloudquery#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 `XStringRoundTrip` tests to test that the resulting array will be equal in the Arrow sense (using [`array.Equal`](https://pkg.go.dev/github.com/apache/arrow/go/v12/arrow/array#Equal))
2. Fix any discrepancies

### Are there any user-facing changes?

Some fixes to the accepted values & parse logic to correspond to the interface contract.

* Closes: apache#35421

Authored-by: candiduslynx <candiduslynx@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
ArgusLi pushed a commit to Bit-Quill/arrow that referenced this issue May 15, 2023
@candiduslynx @ArgusLi
apacheGH-35421: [Go] Ensure interface contract between `array.X.Value…
f19fbd3 
…Str` & `array.XBuilder.AppendValueFromString` (apache#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 cloudquery/cloudquery#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 `XStringRoundTrip` tests to test that the resulting array will be equal in the Arrow sense (using [`array.Equal`](https://pkg.go.dev/github.com/apache/arrow/go/v12/arrow/array#Equal))
2. Fix any discrepancies

### Are there any user-facing changes?

Some fixes to the accepted values & parse logic to correspond to the interface contract.

* Closes: apache#35421

Authored-by: candiduslynx <candiduslynx@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
rtpsw pushed a commit to rtpsw/arrow that referenced this issue May 16, 2023
@candiduslynx @rtpsw
apacheGH-35421: [Go] Ensure interface contract between `array.X.Value…
23ba44a 
…Str` & `array.XBuilder.AppendValueFromString` (apache#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 cloudquery/cloudquery#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 `XStringRoundTrip` tests to test that the resulting array will be equal in the Arrow sense (using [`array.Equal`](https://pkg.go.dev/github.com/apache/arrow/go/v12/arrow/array#Equal))
2. Fix any discrepancies

### Are there any user-facing changes?

Some fixes to the accepted values & parse logic to correspond to the interface contract.

* Closes: apache#35421

Authored-by: candiduslynx <candiduslynx@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@candiduslynx candiduslynx

Labels
Component: Go Type: bug
Projects
None yet
Milestone
13.0.0
2 participants
@zeroshade @candiduslynx