ARROW-17871: [Go] initial binary arithmetic implementation by zeroshade · Pull Request #14255 · apache/arrow

zeroshade · 2022-09-27T21:38:27Z

Only implements Addition and Subtraction for integral types and float32/float64. Temporal types and others will come afterwards.

go/arrow/compute/arithmetic_test.go

go/arrow/compute/internal/kernels/_lib/base_arithmetic_avx2_amd64.s

go/arrow/compute/internal/kernels/_lib/base_arithmetic.cc

go/arrow/compute/arithmetic.go

go/arrow/compute/arithmetic_test.go

pitrou · 2022-09-28T10:58:40Z

go/arrow/compute/internal/kernels/base_arithmetic_amd64.go

+		return getSSE4ArithmeticBinaryNumeric[T](op)
+	}
+
+	return getGoArithmeticBinaryOpsFloating[T](op)


I'm curious, why not also compile this one from C++? 1) the C++ optimizer is probably better than the Go one, 2) less code to write and maintain?

The fallback pure-go implementation is needed for various cases and as of Go1.19, Go still can't inline assembly implemented functions. So it's worthwhile to fallback to the pure-go implementations when we aren't benefiting from the SIMD implementations to allow for whatever optimizations Go can do and so on along with portability.

As you prefer, though you might want compare the relative performance of those different implementations :-)

go/arrow/compute/internal/kernels/helpers.go

pitrou · 2022-09-28T11:01:21Z

go/arrow/compute/utils.go

+func ensureDictionaryDecoded(vals ...arrow.DataType) {
+	for i, v := range vals {
+		if v.ID() == arrow.DICTIONARY {
+			vals[i] = v.(*arrow.DictionaryType).ValueType


For the record, where does the actual dictionary decoding happen? (add a comment?)

currently it's not implemented and will return an ErrNotImplemented before it gets here, but when I implement it i'll update this with a comment.

comment added

go/arrow/compute/utils.go

go/arrow/compute/arithmetic_test.go

go/arrow/compute/internal/kernels/_lib/types.h

go/arrow/compute/internal/kernels/_lib/base_arithmetic.cc

go/arrow/compute/internal/kernels/_lib/types.h

zeroshade · 2022-10-11T16:41:22Z

@pitrou Can you re-review when you get a chance and let me know if there's any other changes you request / if you're good with the current version?

zeroshade · 2022-10-13T18:13:26Z

@cyb70289 any other comments or questions here?

cyb70289

CI errors to be fixed

zeroshade · 2022-10-17T20:08:12Z

Anyone familiar enough with MSYS2 and MinGW to help out here? The error is:

:: Proceed with installation? [Y/n] 
:: Retrieving packages...
error: failed retrieving file 'mingw-w64-x86_64-gcc-objc-12.2.0-4-any.pkg.tar.zst' from mirror.msys2.org : Operation too slow. Less than 1 bytes/sec transferred the last 10 seconds
 mingw-w64-x86_64-gcc-12.2.0-4-any downloading...
error: failed retrieving file 'mingw-w64-x86_64-gmp-6.2.1-3-any.pkg.tar.zst.sig' from mirror2.sandyriver.net : Operation too slow. Less than 1 bytes/sec transferred the last 10 seconds
 mingw-w64-x86_64-arrow-9.0.0-7-any downloading...
warning: failed to retrieve some files
 mingw-w64-x86_64-icu-71.1-1-any downloading...
error: failed to commit transaction (unexpected error)

It looks like something is having an issue downloading the standard packages.

pitrou · 2022-10-17T20:24:39Z

I restarted the failed build.

zeroshade · 2022-10-17T20:53:05Z

@pitrou looks like it still failed to load those packages via mingw which then caused the test failures. The error exit status 0xc0000139 is Windows MinGW saying that it couldn't load the dynamic library at runtime, which this test is testing using CGO to use the Arrow C++ memory pool as the allocator for Go's memory. One of the packages it failed to install and load was arrow-9.0.0 which would explain why it couldn't load the dynamic library at runtime. I'm not quite sure what to do here as this is the third restarted build in a row that has experienced this issue with MSYS2

zeroshade · 2022-10-18T20:29:50Z

@cyb70289 @pitrou

It looks like the current failure on the mingw64 cgo build is attributable to an existing issue with the MSYS2 mingw64 build having an issue loading libaws dlls and not something caused by the Go changes. So it is unrelated here but should likely be looked into.

It's most likely related to awslabs/aws-c-io@550e319 due to the following erors:

Procedure entry point aws_byte_buf_append_encoding_uri_param could not be located in dynamic link library C:\msys64\mingw64\bin\libaws-c-s3.dll
Procedure entry point aws_byte_buf_append_decoding_uri could not be located in dynamic link library C:\msys64\mingw64\bin\libaws-c-auth.dll
Procedure entry point aws_uri_clean_up could not be located in dynamic link library C:\msys64\mingw64\bin\libaws-crt-cpp.dll

cyb70289 · 2022-10-19T01:37:16Z

Similar error from Windows MinGW 64 C++ job.
https://github.com/apache/arrow/actions/runs/3274965496/jobs/5389199341

~~Maybe create a jira issue (if not created yet)?~~
related issue: https://issues.apache.org/jira/browse/ARROW-18095
@kou

kou · 2022-10-19T07:41:09Z

I think that this is related to MSYS2:

BTW...
@zeroshade How did you generate the error messages!? I want to know how to detect which symbol is missing to load a DLL when DLL load is failed!!!

zeroshade · 2022-10-19T14:43:11Z

@kou If you add the mingw64 path to your System level path, reboot, and then try running an exe through the Run Dialog (windows key+R) or through file explorer, then it'll give you those error messages as popup dialog boxes. I don't know how to get it to do so from the command line unfortunately. But this worked out pretty well! 😄

zeroshade · 2022-10-19T15:03:29Z

@pitrou if you don't have any objections, i'll merge this tomorrow.

pitrou · 2022-10-19T15:07:28Z

Can we wait until after the 10.0.0 release? Or will this only be in 11.0.0?

pitrou · 2022-10-19T15:13:29Z

Question: is it useful to have both go/arrow/compute/internal/kernels/_lib/base_arithmetic_avx2_amd64.s and go/arrow/compute/internal/kernels/base_arithmetic_avx2_amd64.s? Is the latter generated from the former?

zeroshade · 2022-10-19T16:14:34Z

@pitrou The plan is that this would only go to 11.0.0, the maint-10.0.0 branch was already created right? So merges to master won't get included in the RC?

Question: is it useful to have both go/arrow/compute/internal/kernels/_lib/base_arithmetic_avx2_amd64.s and go/arrow/compute/internal/kernels/base_arithmetic_avx2_amd64.s? Is the latter generated from the former?

Yes, the latter is generated from the former, able to be regenerated via make assembly in the go/arrow/compute/internal/kernels/ directory. That said, the reason to keep the former around is for easier regeneration of the Go assembly allowing it to be generated without needing a C++ compiler/clang/etc. It ensures consistency where possible (and follows the pattern used elsewhere in the repo for our other use cases of this). Technically it isn't strictly necessary to keep it, I just do for the aforementioned reasons.

The latter must be kept around as it needs to be accessible when a user runs go get so that Go can assemble it during build. When running go get it only builds the files that are there, there is no expectation for a consumer to have to run make before building, it "should just work".

pitrou · 2022-10-19T16:19:07Z

The plan is that this would only go to 11.0.0, the maint-10.0.0 branch was already created right? So merges to master won't get included in the RC?

Oh, you're right. Nevermind.

kou · 2022-10-19T23:41:22Z

@kou If you add the mingw64 path to your System level path, reboot, and then try running an exe through the Run Dialog (windows key+R) or through file explorer, then it'll give you those error messages as popup dialog boxes. I don't know how to get it to do so from the command line unfortunately. But this worked out pretty well! 😄

Thanks!!! It's what I want to know!!!

zeroshade added 2 commits September 27, 2022 17:34

ARROW-17871: [Go] Initial binary arithmetic

691db47

some extra comments

555bbca

zeroshade requested review from cyb70289 and pitrou September 27, 2022 21:38

zeroshade added 2 commits September 27, 2022 17:41

some more comments and move to vendored dir

af43948

forgot my break statements, that's what i get for refactoring

4753398

github-actions bot added the Component: Go label Sep 27, 2022

cyb70289 reviewed Sep 28, 2022

View reviewed changes

pitrou requested changes Sep 28, 2022

View reviewed changes

respond to feedback with tests and overflow handling

7c9c52e

zeroshade requested a review from pitrou September 28, 2022 21:34

zeroshade added 3 commits September 28, 2022 18:27

more tests and some comments

1121a7c

add a benchmark plus some minor rearranging for inline and memory usage

0c1f144

fix null compute SetSlice

c4a205e

zeroshade requested a review from cyb70289 September 30, 2022 15:35

cyb70289 reviewed Oct 3, 2022

View reviewed changes

go/arrow/compute/internal/kernels/_lib/types.h Show resolved Hide resolved

cyb70289 reviewed Oct 7, 2022

View reviewed changes

go/arrow/compute/internal/kernels/_lib/base_arithmetic.cc Show resolved Hide resolved

go/arrow/compute/internal/kernels/_lib/types.h Show resolved Hide resolved

zeroshade added 2 commits October 12, 2022 17:21

Merge branch 'master' into arrow-17871-initial-binary-arithmetic

5f48748

Merge branch 'master' into arrow-17871-initial-binary-arithmetic

dba546f

cyb70289 approved these changes Oct 14, 2022

View reviewed changes

cyb70289 requested changes Oct 14, 2022

View reviewed changes

fix merge update

f0f0164

zeroshade requested a review from cyb70289 October 17, 2022 15:46

cyb70289 approved these changes Oct 19, 2022

View reviewed changes

zeroshade merged commit 24c0fce into apache:master Oct 20, 2022

zeroshade deleted the arrow-17871-initial-binary-arithmetic branch October 20, 2022 15:18

asfimport mentioned this pull request Oct 20, 2022

[Go] Implement Initial Scalar Binary Arithmetic Infrastructure #20434

Closed

Conversation

zeroshade commented Sep 27, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pitrou Sep 28, 2022

Choose a reason for hiding this comment

Uh oh!

zeroshade Sep 28, 2022

Choose a reason for hiding this comment

Uh oh!

pitrou Sep 28, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pitrou Sep 28, 2022

Choose a reason for hiding this comment

Uh oh!

zeroshade Sep 28, 2022

Choose a reason for hiding this comment

Uh oh!

zeroshade Sep 28, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zeroshade commented Oct 11, 2022

Uh oh!

zeroshade commented Oct 13, 2022

Uh oh!

cyb70289 left a comment

Choose a reason for hiding this comment

Uh oh!

zeroshade commented Oct 17, 2022

Uh oh!

pitrou commented Oct 17, 2022

Uh oh!

zeroshade commented Oct 17, 2022

Uh oh!

zeroshade commented Oct 18, 2022

Uh oh!

cyb70289 commented Oct 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kou commented Oct 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zeroshade commented Oct 19, 2022

Uh oh!

zeroshade commented Oct 19, 2022

Uh oh!

pitrou commented Oct 19, 2022

Uh oh!

pitrou commented Oct 19, 2022

Uh oh!

zeroshade commented Oct 19, 2022

Uh oh!

pitrou commented Oct 19, 2022

Uh oh!

kou commented Oct 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cyb70289 commented Oct 19, 2022 •

edited

Loading

kou commented Oct 19, 2022 •

edited

Loading