GH-32832: [Go] support building with tinygo #35723

chriscasola · 2023-05-23T12:48:13Z

Rationale for this change

To support compiling with tinygo which enables use of arrow in environments where binary size is important, like web assembly.

What changes are included in this PR?

Using an internal JSON package that uses goccy/go-json for regular builds as it does currently, but uses the native encoding/json for tinygo builds. This is necessary because go-json has a lot of code that is incompatible with tinygo.

Remove dependency on parquet package from non-parquet code since it is also incompatible with tinygo.

Other minor tweaks for compatibility with tinygo.

Are these changes tested?

Should we add a build step that compiles the example with tinygo?

Are there any user-facing changes?

None.

Closes: [Go] Unable to build with tinygo #32832

github-actions · 2023-05-23T12:48:37Z

Closes: [Go] Unable to build with tinygo #32832

github-actions · 2023-05-23T12:48:40Z

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

chriscasola · 2023-05-25T13:32:08Z

@zeroshade we're still testing this on our end, but it does work. I'm curious what your thoughts are.

zeroshade

In general this LGTM, just a few nit picks

dev/release/rat_exclude_files.txt

go/arrow/array/string.go

zeroshade · 2023-05-25T16:39:01Z

go/arrow/array/string.go

 	beg := a.array.data.offset
 	end := beg + a.array.data.length
 	data := a.values[a.offsets[beg]:a.offsets[end]]

-	s := (*reflect.SliceHeader)(unsafe.Pointer(&ret))
-	s.Data = (*reflect.StringHeader)(unsafe.Pointer(&data)).Data
-	s.Len = len(data)
-	s.Cap = len(data)
-	return
+	return []byte(data)


same comment as above

This still needs to get updated to no longer be copying the string (like the equivalent function for (*String)

go/arrow/bitutil/bitutil.go

go/arrow/compute/arithmetic_test.go

go/arrow/compute/cast_test.go

go/internal/hashing/xxh3_memo_table.go

chriscasola · 2023-05-30T19:11:52Z

@zeroshade thanks for the review, I think I addressed everything.

What are you thoughts on building with tinygo in CI? The CI setup on this repo seems complex so I'm not sure what that would entail, but it would be nice to avoid new code being introduced that does not build.

zeroshade · 2023-06-01T16:02:02Z

go/internal/types/extension_types.go

-	"github.com/goccy/go-json"
+	"github.com/apache/arrow/go/v13/internal/json"

 	"github.com/apache/arrow/go/v13/arrow"
 	"github.com/apache/arrow/go/v13/arrow/array"


the failed dev scripts test is because when we run the auto update scripts this gets updated as a separate block. Can you remove the empty space below the updated import and just group all the arrow imports together (alphabetically)?

i.e.:

... "strings" "github.com/apache/arrow/go/v13/arrow" "github.com/apache/arrow/go/v13/arrow/array" "github.com/apache/arrow/go/v13/internal/json" "github.com/google/uuid" "golang.org/x/xerrors"

thanks.

zeroshade · 2023-06-01T16:08:18Z

@chriscasola Do you think it's sufficient to just have a single x86-64/linux CI run with tinygo? Or do you think we'd need for arm and other cases too?

chriscasola · 2023-06-01T16:49:01Z

@chriscasola Do you think it's sufficient to just have a single x86-64/linux CI run with tinygo? Or do you think we'd need for arm and other cases too?

I think a single architecture is fine.

remove parquet import in hashing cleanup type check fix uintptr usage exclude goccy/go-json for tinygo builds last uintptr problem

zeroshade · 2023-06-06T16:36:40Z

.github/workflows/go.yml

+    name: TinyGo
+    runs-on: ubuntu-latest
+    if: ${{ !contains(github.event.pull_request.title, 'WIP') }}
+    env:
+      TINYGO_VERSION: 0.27.0


i would suggest adding an image to the docker-compose.yml and a dockerfile to ci/docker/ which is an image with tinygo installed and then use archery docker run like other examples.

zeroshade · 2023-06-06T16:37:00Z

ci/scripts/go_tinygo_example.sh

+cd ~
+pushd /src
+tinygo build -tags noasm -o ~/example_tinygo arrow/_examples/helloworld/main.go
+popd
+
+./example_tinygo


any reason to not just have it run the unit tests?

Unfortunately tinygo can't compile/run unit tests right now. At least it can't handle the ones in this project. See this issue.

zeroshade · 2023-06-06T16:38:01Z

ci/scripts/go_tinygo_example.sh

+
+cd ~
+pushd /src
+tinygo build -tags noasm -o ~/example_tinygo arrow/_examples/helloworld/main.go


should we update all of the go build constraints for noasm to be noasm || tinygo or whatever the equivalent is?

@zeroshade I attempted to do this, but it seems like package outside arrow might also have a noasm flag. Even after adding the tinygo condition everywhere there was a noasm condition, I still couldn't build with out the noasm tag. Are you okay omitting this for now?

yea that's fine for now. We should probably figure out which package it is that has the noasm condition etc.

Unfortunately tinygo is just erroring without enough context to find the file at fault. I'm not sure if there's a way to find it with the standard go toolchain.

chriscasola · 2023-06-12T15:49:00Z

@zeroshade this is ready for review and has been fully tested to work with tinygo in our app's environment.

zeroshade

Just one request: can you add to the go/arrow/doc.go file's package comment documentation the info about building with tinygo like needing the noasm tag, etc.?

Otherwise this looks good to me.

ursabot · 2023-06-13T01:05:17Z

Benchmark runs are scheduled for baseline = b642707 and contender = 245404e. 245404e is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️25.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️1.24% ⬆️0.12%] test-mac-arm
[Finished ⬇️0.0% ⬆️0.65%] ursa-i9-9960x
[Finished ⬇️0.06% ⬆️0.48%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 245404e2 ec2-t3-xlarge-us-east-2
[Finished] 245404e2 test-mac-arm
[Finished] 245404e2 ursa-i9-9960x
[Finished] 245404e2 ursa-thinkcentre-m75q
[Finished] b642707f ec2-t3-xlarge-us-east-2
[Finished] b642707f test-mac-arm
[Finished] b642707f ursa-i9-9960x
[Finished] b642707f 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

tschaub · 2023-09-21T02:13:38Z

Cool to see this got in. I wonder if anybody looked into what it would take to get the Parquet packages working as well. I may give it another try at some point.

zeroshade · 2023-09-21T14:34:48Z

I believe the current blocker on the parquet packages is that something in the Thrift dependency is not compatible with tinygo

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

chriscasola force-pushed the choff--wasm branch from 88283c3 to 5423f0a Compare May 23, 2023 13:45

zeroshade requested changes May 25, 2023

View reviewed changes

chriscasola force-pushed the choff--wasm branch 3 times, most recently from 5f41f87 to 66cacfb Compare May 30, 2023 18:53

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

chriscasola force-pushed the choff--wasm branch from 66cacfb to db145f4 Compare May 30, 2023 19:09

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

zeroshade reviewed Jun 1, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Jun 1, 2023

chriscasola force-pushed the choff--wasm branch from db145f4 to 0306402 Compare June 1, 2023 16:48

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jun 1, 2023

feat(build): attempt to work with tinygo

d185e58

remove parquet import in hashing cleanup type check fix uintptr usage exclude goccy/go-json for tinygo builds last uintptr problem

chriscasola force-pushed the choff--wasm branch from 0306402 to d185e58 Compare June 2, 2023 12:42

chriscasola force-pushed the choff--wasm branch 2 times, most recently from d066919 to b2fb989 Compare June 2, 2023 16:24

Build with tinygo in CI

9d3d102

chriscasola force-pushed the choff--wasm branch from b2fb989 to 9d3d102 Compare June 2, 2023 17:29

zeroshade reviewed Jun 6, 2023

View reviewed changes

Merge branch 'main' into choff--wasm

e1f3ac8

chriscasola marked this pull request as ready for review June 12, 2023 15:48

chriscasola requested review from assignUser, kou and raulcd as code owners June 12, 2023 15:48

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Jun 12, 2023

zeroshade requested changes Jun 12, 2023

View reviewed changes

Add note about noasm build tag.

f1b4891

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jun 12, 2023

zeroshade approved these changes Jun 12, 2023

View reviewed changes

zeroshade merged commit 245404e into apache:main Jun 12, 2023

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Jun 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-32832: [Go] support building with tinygo #35723

GH-32832: [Go] support building with tinygo #35723

chriscasola commented May 23, 2023 •

edited by github-actions bot

Loading

github-actions bot commented May 23, 2023

github-actions bot commented May 23, 2023

chriscasola commented May 25, 2023

zeroshade left a comment

zeroshade May 25, 2023

zeroshade Jun 1, 2023

chriscasola commented May 30, 2023

zeroshade Jun 1, 2023

zeroshade commented Jun 1, 2023

chriscasola commented Jun 1, 2023

zeroshade Jun 6, 2023

zeroshade Jun 6, 2023

chriscasola Jun 7, 2023

zeroshade Jun 6, 2023

chriscasola Jun 8, 2023

zeroshade Jun 12, 2023

chriscasola Jun 12, 2023

chriscasola commented Jun 12, 2023

zeroshade left a comment

ursabot commented Jun 13, 2023

tschaub commented Sep 21, 2023

zeroshade commented Sep 21, 2023

GH-32832: [Go] support building with tinygo #35723

GH-32832: [Go] support building with tinygo #35723

Conversation

chriscasola commented May 23, 2023 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented May 23, 2023

github-actions bot commented May 23, 2023

chriscasola commented May 25, 2023

zeroshade left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriscasola commented May 30, 2023

Choose a reason for hiding this comment

zeroshade commented Jun 1, 2023

chriscasola commented Jun 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriscasola commented Jun 12, 2023

zeroshade left a comment

Choose a reason for hiding this comment

ursabot commented Jun 13, 2023

tschaub commented Sep 21, 2023

zeroshade commented Sep 21, 2023

chriscasola commented May 23, 2023 •

edited by github-actions bot

Loading