GH-35421: [Go] Ensure interface contract between `array.X.ValueStr` & `array.XBuilder.AppendValueFromString` by candiduslynx · Pull Request #35457 · apache/arrow

candiduslynx · 2023-05-06T06:26:33Z

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:

Introduce XStringRoundTrip tests to test that the resulting array will be equal in the Arrow sense (using array.Equal)
Fix any discrepancies

Are there any user-facing changes?

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

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

github-actions · 2023-05-06T06:26:54Z

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

github-actions · 2023-05-06T06:26:57Z

⚠️ GitHub issue #35421 has been automatically assigned in GitHub to PR creator.

candiduslynx · 2023-05-06T06:39:31Z

@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

UPD: done in 29a1181181fbfc3f8ff244227c9b0dc6aaa50c67

candiduslynx

OK, seems like now all workflows should pass

candiduslynx · 2023-05-08T09:26:55Z

@zeroshade, could you please tell me if there is a chance to get review sooner?

I'd like to sync this to cloudquery/cloudquery#10284 and use the ValueStr instead of JSON marshaling by default.

…r` & `types.XBuilder.AppendValueFromString` (#849) Follow-up to apache/arrow#35457 BEGIN_COMMIT_OVERRIDE chore(tests): Ensure interface contract between `types.XArray.ValueStr` & `types.XBuilder.AppendValueFromString` (#849) feat(arrow): Add `types.XBuilder.NewXArray` helpers END_COMMIT_OVERRIDE

zeroshade

Overall this looks good to me, just a few questions and some nitpicks.

.gitignore

go/arrow/array/dictionary.go

go/arrow/array/encoded.go

go/arrow/array/numericbuilder.gen.go

go/arrow/array/numericbuilder.gen.go.tmpl

go/internal/types/extension_types.go

go/arrow/array/array.go

Based on apache/arrow#35457 review

candiduslynx

@zeroshade so, I guess I addressed everything that's been asked for.
Can you take another look?

zeroshade

Just one question to confirm, otherwise LGTM

go/arrow/datatype_fixedwidth.go

go/internal/types/extension_types.go

zeroshade

Just one question to confirm, otherwise LGTM

candiduslynx · 2023-05-08T18:21:56Z

Removed the extra piece in 2bf65c5, thanks for the review!

candiduslynx · 2023-05-08T19:05:43Z

@zeroshade the checks are finally done (I wonder if the largest one (conda) has to run the whole test suite, though)

ursabot · 2023-05-09T05:51:54Z

Benchmark runs are scheduled for baseline = b211c18 and contender = 05a57de. 05a57de is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️4.44% ⬆️0.0%] test-mac-arm
[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Failed ⬇️0.0% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 05a57dec ec2-t3-xlarge-us-east-2
[Failed] 05a57dec test-mac-arm
[Finished] 05a57dec ursa-i9-9960x
[Failed] 05a57dec ursa-thinkcentre-m75q
[Finished] b211c189 ec2-t3-xlarge-us-east-2
[Failed] b211c189 test-mac-arm
[Finished] b211c189 ursa-i9-9960x
[Failed] b211c189 ursa-thinkcentre-m75q
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

ursabot · 2023-05-09T05:52:16Z

['Python', 'R'] benchmarks have high level of regressions.
test-mac-arm

…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>

raulcd · 2023-05-18T11:20:27Z

@github-actions crossbow submit verify-rc-source-integration-linux*

github-actions · 2023-05-18T11:22:52Z

Revision: 2bf65c5

Submitted crossbow builds: ursacomputing/crossbow @ actions-0f9abaae22

Task	Status
verify-rc-source-integration-linux-almalinux-8-amd64
verify-rc-source-integration-linux-conda-latest-amd64
verify-rc-source-integration-linux-ubuntu-20.04-amd64
verify-rc-source-integration-linux-ubuntu-22.04-amd64

candiduslynx requested a review from zeroshade as a code owner May 6, 2023 06:26

github-actions bot added Component: Go awaiting review Awaiting review labels May 6, 2023

candiduslynx force-pushed the fix/array/valuestr-appendvaluefromstring-contract branch 2 times, most recently from 0533bda to ea7e636 Compare May 6, 2023 08:25

candiduslynx commented May 6, 2023

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels May 6, 2023

candiduslynx mentioned this pull request May 8, 2023

chore(tests): Ensure interface contract between types.XArray.ValueStr & types.XBuilder.AppendValueFromString cloudquery/plugin-sdk#849

Merged

candiduslynx force-pushed the fix/array/valuestr-appendvaluefromstring-contract branch 2 times, most recently from df8a331 to 4e2b295 Compare May 8, 2023 12:28

zeroshade requested changes May 8, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels May 8, 2023

candiduslynx force-pushed the fix/array/valuestr-appendvaluefromstring-contract branch from 4e2b295 to f765a9a Compare May 8, 2023 16:01

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels May 8, 2023

candiduslynx requested a review from zeroshade May 8, 2023 16:26

candiduslynx mentioned this pull request May 8, 2023

chore(UUID): Streamline UUID implementation cloudquery/plugin-sdk#851

Merged

kodiakhq bot pushed a commit to cloudquery/plugin-sdk that referenced this pull request May 8, 2023

chore(UUID): Streamline UUID implementation (#851)

7fe9990

Based on apache/arrow#35457 review

candiduslynx added 5 commits May 8, 2023 20:07

ignore vendor

a3bfb1c

binary & binarybuilder

e9c72f3

copy extension UUID code from CQ

26bb3b2

use array.NullValueStr

5998385

boolean

c655157

candiduslynx added 11 commits May 8, 2023 20:09

numeric AppendValueFromString test

d40064a

fix go 1.17

10d9107

add license header to float16_test.go

f0a7f3f

shorten

df8337b

remove extra ref for dictionary

a5149bf

rm doc about error

09927b1

add doc note

0d1da43

uuid fixes

ae087fd

doc string about array.NullValueStr

c6ba842

fix UUIDBuilder.AppendValueFromString

aaf9766

use time.ParseDuration

cd9de70

candiduslynx force-pushed the fix/array/valuestr-appendvaluefromstring-contract branch from d16fc4c to cd9de70 Compare May 8, 2023 17:09

fix test

398bb40

candiduslynx commented May 8, 2023

View reviewed changes

zeroshade approved these changes May 8, 2023

View reviewed changes

go/arrow/datatype_fixedwidth.go Outdated Show resolved Hide resolved

go/internal/types/extension_types.go Outdated Show resolved Hide resolved

zeroshade approved these changes May 8, 2023

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels May 8, 2023

rm extra func

2bf65c5

zeroshade merged commit 05a57de into apache:main May 8, 2023

candiduslynx deleted the fix/array/valuestr-appendvaluefromstring-contract branch May 9, 2023 04:48

Conversation

candiduslynx commented May 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions bot commented May 6, 2023

Uh oh!

github-actions bot commented May 6, 2023

Uh oh!

candiduslynx commented May 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

candiduslynx left a comment

Choose a reason for hiding this comment

Uh oh!

candiduslynx commented May 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zeroshade left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

candiduslynx left a comment

Choose a reason for hiding this comment

Uh oh!

zeroshade left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zeroshade left a comment

Choose a reason for hiding this comment

Uh oh!

candiduslynx commented May 8, 2023

Uh oh!

candiduslynx commented May 8, 2023

Uh oh!

ursabot commented May 9, 2023

Uh oh!

ursabot commented May 9, 2023

Uh oh!

raulcd commented May 18, 2023

Uh oh!

github-actions bot commented May 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

candiduslynx commented May 6, 2023 •

edited

Loading

candiduslynx commented May 6, 2023 •

edited

Loading

candiduslynx commented May 8, 2023 •

edited

Loading