arrow icon indicating copy to clipboard operation
arrow copied to clipboard

ARROW-17871: [Go] initial binary arithmetic implementation

Open zeroshade opened this issue 3 years ago • 1 comments

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

zeroshade avatar Sep 27 '22 21:09 zeroshade

@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 avatar Oct 11 '22 16:10 zeroshade

@cyb70289 any other comments or questions here?

zeroshade avatar Oct 13 '22 18:10 zeroshade

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.

zeroshade avatar Oct 17 '22 20:10 zeroshade

I restarted the failed build.

pitrou avatar Oct 17 '22 20:10 pitrou

@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 avatar Oct 17 '22 20:10 zeroshade

@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 https://github.com/awslabs/aws-c-io/commit/550e3192b9d1c7ac865d3f71273bfd7729460900 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

zeroshade avatar Oct 18 '22 20:10 zeroshade

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

cyb70289 avatar Oct 19 '22 01:10 cyb70289

I think that this is related to MSYS2:

  • https://github.com/msys2/MINGW-packages/pull/13618
  • https://github.com/msys2/MINGW-packages/pull/13619
  • https://github.com/msys2/MINGW-packages/pull/13620

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

kou avatar Oct 19 '22 07:10 kou

@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! :smile:

zeroshade avatar Oct 19 '22 14:10 zeroshade

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

zeroshade avatar Oct 19 '22 15:10 zeroshade

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

pitrou avatar Oct 19 '22 15:10 pitrou

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?

pitrou avatar Oct 19 '22 15:10 pitrou

@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".

zeroshade avatar Oct 19 '22 16:10 zeroshade

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.

pitrou avatar Oct 19 '22 16:10 pitrou

@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! :smile:

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

kou avatar Oct 19 '22 23:10 kou