GH-37789: [Integration][Go] Go C Data Interface integration testing #37788

pitrou · 2023-09-19T14:44:40Z

Rationale for this change

We want to enable integration testing of the Arrow Go implementation of the C Data Interface, so as to ensure interoperability.

What changes are included in this PR?

Enable C Data Interface integration testing for the Arrow Go implementation
Fix compatibility issues found by the integration tests

Are these changes tested?

Yes, by construction.

Are there any user-facing changes?

Bugfixes in the Arrow Go C Data Interface implementation.

Closes: [Integration][Go] Implement Go C Data Interface integration testing #37789

github-actions · 2023-09-19T14:45:03Z

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

pitrou · 2023-09-19T14:45:42Z

@zeroshade It would be nice if you could take a look at this. This is draft as my Go skills are almost non-existent :-)

zeroshade · 2023-09-19T15:07:37Z

@pitrou I'll definitely take a look at this today. thanks for drafting it out in the first place, I was planning on doing this myself after i saw your other PR get merged!

github-actions · 2023-09-19T15:46:47Z

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

pitrou · 2023-09-19T16:07:44Z

go/arrow/internal/cdata_integration/entrypoints.go

+}
+
+func newJsonReader(cJsonPath *C.char) (*arrjson.Reader, error) {
+	jsonPath := C.GoString(cJsonPath)


@zeroshade I don't really understand our Go coding style. Should I use camelCase or snake_case for variables?

the general style for go is camelCase

Ok, thanks for confirming my intuition. Is there any way to enforce that?

I'll see if there's an option i'm missing in the linter workflow that would enforce it

pitrou · 2023-09-20T16:55:15Z

@zeroshade This is ready for review now.

zeroshade

Overall LGTM just some stylistic nit picks

zeroshade · 2023-09-22T19:53:51Z

ci/scripts/go_build.sh

+            go_lib="arrow_go_integration.so"
+            ;;
+        Darwin)
+            go_lib="arrow_go_integration.so"


we don't use .dylib for mac?

Ahah, yes, we might!
Though, admittedly this entire condition is a bit pedantic as the library is only loaded explicitly using its full pathname :-)

Fair, though looking at my existing cdata/test/test_export_to_cgo.py it looks like I do the same thing as you're doing here. I guess the default output name is still .so on mac unless you provide a -o option to give it the dylib explicitly. 😄

Ok, according to https://stackoverflow.com/a/29226313, there's a theoretical difference between the two which doesn't seem to matter in practice:

The difference between .dylib and .so on mac os x is how they are compiled. For .so files you use -shared and for .dylib you use -dynamiclib. Both .so and .dylib are interchangeable as dynamic library files and either have a type as DYLIB or BUNDLE. [...] The reason the two are equivalent on Mac OS X is for backwards compatibility with other UNIX OS programs that compile to the .so file type.

dev/archery/archery/integration/tester_go.py

zeroshade · 2023-09-22T20:01:45Z

dev/archery/archery/integration/tester_go.py

+    # Note: the Arrow Go C Data export functions expect their output
+    # ArrowStream or ArrowArray argument to be zero-initialized.
+    # This is currently ensured through the use of `ffi.new`.


huh, i thought trampoline.c zero-initialized everything but you're right, we're only trampolining to zero-initialize for streamGetSchema and streamGetNext, darn.

I'm not sure what trampoline.c is, but I directly call into CGo-exported functions here.

See apache/arrow-adbc#729 for the full discussion, but essentially the callbacks that were populated into the C Data ArrowArrayStream struct for getting the schema and getting the next record batch had the same assumption that the output arguments were zero-initialized. So @lidavidm created trampoline.c which essentially provided wrappers around the Go exported streamGetSchema and streamGetNext function pointers, and we use the trampoline methods as the function pointers we use (see exportStream in cdata/exports.go)

But like i said, it looks like we only did that for the function pointers we set for the callbacks in the ArrowArrayStream struct and not for the other exported functions for regular import/export.

It would be good to apply the same protections for the rest.

Notably, Arrow-C++ does NOT zero-initialize structs before using them so we should try to reflect reality here.

I agree. I was just thinking it should be done in a different PR than this one, unless @pitrou feels like adding the same protections to the existing exported funcs here 😄

No, let's defer this to another issue and PR.

zeroshade · 2023-09-22T20:05:31Z

go/arrow/cdata/cdata.go

-	defaulttz := "UTC"
+	defaulttz := ""


was this incorrect by the spec? I thought I pulled this from the c bridge which had the same default? (I could be misremembering though)

Yes, it was incorrect and failing the integration tests between C++ and Go.
A timestamp with an missing timezone is not the same as a timestamp with the UTC timezone, as explained in the format spec:

arrow/format/Schema.fbs

Line 264 in 772a01c

/// Timestamp is a 64-bit signed integer representing an elapsed time since a

Bah, must have mistaken that when I originally coded this 😦

go/arrow/cdata/cdata_exports.go

go/arrow/internal/cdata_integration/entrypoints.go

…ting

pitrou · 2023-09-25T13:12:26Z

The CI failures are unrelated. The Go and integration tests are all green, and I believe I've addressed all your comments @zeroshade . Is it ok if I merge this?

zeroshade · 2023-09-26T15:02:40Z

Sorry for not responding yesterday, I was out sick. But this all looked good to me! Thanks!

conbench-apache-arrow · 2023-09-27T15:14:54Z

After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit cc51e68.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about possible false positives for unstable benchmarks that are known to sometimes produce them.

…ting (apache#37788) ### Rationale for this change We want to enable integration testing of the Arrow Go implementation of the C Data Interface, so as to ensure interoperability. ### What changes are included in this PR? 1. Enable C Data Interface integration testing for the Arrow Go implementation 2. Fix compatibility issues found by the integration tests ### Are these changes tested? Yes, by construction. ### Are there any user-facing changes? Bugfixes in the Arrow Go C Data Interface implementation. * Closes: apache#37789 Authored-by: Antoine Pitrou <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>

github-actions bot added Component: Go awaiting review Awaiting review labels Sep 19, 2023

pitrou changed the title ~~WIP: Go C Data Integration~~ GH-37789: [Integration][Go] Go C Data Interface integration testing Sep 19, 2023

pitrou force-pushed the go-c-data-integration branch from 3d145ff to 365abee Compare September 19, 2023 15:50

pitrou commented Sep 19, 2023

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Sep 19, 2023

pitrou force-pushed the go-c-data-integration branch from 365abee to d2eceb5 Compare September 20, 2023 14:29

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Sep 20, 2023

pitrou marked this pull request as ready for review September 20, 2023 14:59

pitrou requested review from assignUser, kou and raulcd as code owners September 20, 2023 14:59

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Sep 20, 2023

zeroshade requested changes Sep 22, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Sep 22, 2023

pitrou added 6 commits September 25, 2023 13:39

apacheGH-37789: [Integration][Go] Go C Data Interface integration tes…

c5fc628

…ting

Move build step to go_build.sh

f15726c

Slightly better error propagation

713af85

go fmt + improved error reporting

5cd05b1

Re-enable other integration tests

76d6063

Bump timeout for Windows Go builds

ddee666

Address review comments

d6dd8bb

pitrou force-pushed the go-c-data-integration branch from bf9afb9 to d6dd8bb Compare September 25, 2023 12:07

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Sep 25, 2023

pitrou merged commit cc51e68 into apache:main Sep 26, 2023
52 of 56 checks passed

pitrou removed the awaiting change review Awaiting change review label Sep 26, 2023

pitrou deleted the go-c-data-integration branch September 26, 2023 07:14

pitrou mentioned this pull request Sep 27, 2023

[Integration][Java] Implement C Data Interface integration testing for Java #37910

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-37789: [Integration][Go] Go C Data Interface integration testing #37788

GH-37789: [Integration][Go] Go C Data Interface integration testing #37788

pitrou commented Sep 19, 2023 •

edited by github-actions bot

Loading

github-actions bot commented Sep 19, 2023

pitrou commented Sep 19, 2023

zeroshade commented Sep 19, 2023

github-actions bot commented Sep 19, 2023

pitrou Sep 19, 2023

zeroshade Sep 20, 2023

pitrou Sep 20, 2023

zeroshade Sep 22, 2023

pitrou commented Sep 20, 2023

zeroshade left a comment

zeroshade Sep 22, 2023

pitrou Sep 22, 2023

zeroshade Sep 22, 2023

pitrou Sep 25, 2023

zeroshade Sep 22, 2023

pitrou Sep 22, 2023

zeroshade Sep 22, 2023

lidavidm Sep 22, 2023

zeroshade Sep 22, 2023

pitrou Sep 25, 2023

zeroshade Sep 22, 2023

pitrou Sep 22, 2023

zeroshade Sep 22, 2023

pitrou commented Sep 25, 2023

zeroshade commented Sep 26, 2023

conbench-apache-arrow bot commented Sep 27, 2023

GH-37789: [Integration][Go] Go C Data Interface integration testing #37788

GH-37789: [Integration][Go] Go C Data Interface integration testing #37788

Conversation

pitrou commented Sep 19, 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 Sep 19, 2023

pitrou commented Sep 19, 2023

zeroshade commented Sep 19, 2023

github-actions bot commented Sep 19, 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

pitrou commented Sep 20, 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

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

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

pitrou commented Sep 25, 2023

zeroshade commented Sep 26, 2023

conbench-apache-arrow bot commented Sep 27, 2023

pitrou commented Sep 19, 2023 •

edited by github-actions bot

Loading