You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/08/19 19:33:27 UTC
[GitHub] [arrow] vivkong opened a new pull request #8011: ARROW-9803: [Go] Add initial support for s390x
vivkong opened a new pull request #8011:
URL: https://github.com/apache/arrow/pull/8011
The initial support will allow the unit tests to pass on s390x.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kiszk commented on a change in pull request #8011: ARROW-9803: [Go] Add initial support for s390x
Posted by GitBox <gi...@apache.org>.
kiszk commented on a change in pull request #8011:
URL: https://github.com/apache/arrow/pull/8011#discussion_r473904805
##########
File path: go/arrow/type_traits_decimal128.go
##########
@@ -39,8 +40,13 @@ func (decimal128Traits) BytesRequired(n int) int { return Decimal128SizeBytes *
// PutValue
func (decimal128Traits) PutValue(b []byte, v decimal128.Num) {
- binary.LittleEndian.PutUint64(b[:8], uint64(v.LowBits()))
- binary.LittleEndian.PutUint64(b[8:], uint64(v.HighBits()))
+ if runtime.GOARCH == "s390x" {
+ binary.BigEndian.PutUint64(b[:8], uint64(v.LowBits()))
Review comment:
Can we use [this convention](https://github.com/apache/arrow/blob/master/cpp/src/arrow/util/basic_decimal.cc#L145-L154) across languages? In other words, `LowBits()` always keeps lower 64-bits regardless of endian. `HighBits()` keeps higher 64-bits.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] vivkong commented on a change in pull request #8011: ARROW-9803: [Go] Add initial support for s390x
Posted by GitBox <gi...@apache.org>.
vivkong commented on a change in pull request #8011:
URL: https://github.com/apache/arrow/pull/8011#discussion_r488042158
##########
File path: go/arrow/type_traits_decimal128.go
##########
@@ -39,8 +40,13 @@ func (decimal128Traits) BytesRequired(n int) int { return Decimal128SizeBytes *
// PutValue
func (decimal128Traits) PutValue(b []byte, v decimal128.Num) {
- binary.LittleEndian.PutUint64(b[:8], uint64(v.LowBits()))
- binary.LittleEndian.PutUint64(b[8:], uint64(v.HighBits()))
+ if runtime.GOARCH == "s390x" {
Review comment:
Thanks @mundaym I'll update the commit
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #8011: ARROW-9803: [Go] Add initial support for s390x
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #8011:
URL: https://github.com/apache/arrow/pull/8011#issuecomment-677453542
cc @kiszk
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] vivkong commented on a change in pull request #8011: ARROW-9803: [Go] Add initial support for s390x
Posted by GitBox <gi...@apache.org>.
vivkong commented on a change in pull request #8011:
URL: https://github.com/apache/arrow/pull/8011#discussion_r473925904
##########
File path: go/arrow/type_traits_decimal128.go
##########
@@ -39,8 +40,13 @@ func (decimal128Traits) BytesRequired(n int) int { return Decimal128SizeBytes *
// PutValue
func (decimal128Traits) PutValue(b []byte, v decimal128.Num) {
- binary.LittleEndian.PutUint64(b[:8], uint64(v.LowBits()))
- binary.LittleEndian.PutUint64(b[8:], uint64(v.HighBits()))
+ if runtime.GOARCH == "s390x" {
+ binary.BigEndian.PutUint64(b[:8], uint64(v.LowBits()))
Review comment:
Thanks for the review. I'll fix this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mundaym commented on a change in pull request #8011: ARROW-9803: [Go] Add initial support for s390x
Posted by GitBox <gi...@apache.org>.
mundaym commented on a change in pull request #8011:
URL: https://github.com/apache/arrow/pull/8011#discussion_r487034442
##########
File path: go/arrow/type_traits_decimal128.go
##########
@@ -39,8 +40,13 @@ func (decimal128Traits) BytesRequired(n int) int { return Decimal128SizeBytes *
// PutValue
func (decimal128Traits) PutValue(b []byte, v decimal128.Num) {
- binary.LittleEndian.PutUint64(b[:8], uint64(v.LowBits()))
- binary.LittleEndian.PutUint64(b[8:], uint64(v.HighBits()))
+ if runtime.GOARCH == "s390x" {
Review comment:
This package would also be a good place for a `IsBigEndian` constant.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8011: ARROW-9803: [Go] Add initial support for s390x
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8011:
URL: https://github.com/apache/arrow/pull/8011#issuecomment-676625517
https://issues.apache.org/jira/browse/ARROW-9803
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mundaym commented on a change in pull request #8011: ARROW-9803: [Go] Add initial support for s390x
Posted by GitBox <gi...@apache.org>.
mundaym commented on a change in pull request #8011:
URL: https://github.com/apache/arrow/pull/8011#discussion_r487033507
##########
File path: go/arrow/type_traits_decimal128.go
##########
@@ -39,8 +40,13 @@ func (decimal128Traits) BytesRequired(n int) int { return Decimal128SizeBytes *
// PutValue
func (decimal128Traits) PutValue(b []byte, v decimal128.Num) {
- binary.LittleEndian.PutUint64(b[:8], uint64(v.LowBits()))
- binary.LittleEndian.PutUint64(b[8:], uint64(v.HighBits()))
+ if runtime.GOARCH == "s390x" {
Review comment:
A simple 'endian' (or another name) package containing some files like this might help reduce the if statement spam, and address @emkornfield's concerns (though the compiler is pretty good at removing those branches):
big.go:
```go
// +build s390x
package endian
import "encoding/binary"
var Native = binary.BigEndian
```
little.go:
```go
// +build !s390x
package endian
import "encoding/binary"
var Native = binary.LittleEndian
```
Then you can just use `endian.Native.*` functions instead of having if statements. Plus support for other big endian platforms becomes easy.
(Note that even though Native is a variable the methods called on it will still be resolved at compile time because the type is constant).
##########
File path: go/arrow/type_traits_decimal128.go
##########
@@ -39,8 +40,13 @@ func (decimal128Traits) BytesRequired(n int) int { return Decimal128SizeBytes *
// PutValue
func (decimal128Traits) PutValue(b []byte, v decimal128.Num) {
- binary.LittleEndian.PutUint64(b[:8], uint64(v.LowBits()))
- binary.LittleEndian.PutUint64(b[8:], uint64(v.HighBits()))
+ if runtime.GOARCH == "s390x" {
Review comment:
This package would also be a good place for a `IsBigEndian` constant.
##########
File path: go/arrow/type_traits_decimal128.go
##########
@@ -39,8 +40,13 @@ func (decimal128Traits) BytesRequired(n int) int { return Decimal128SizeBytes *
// PutValue
func (decimal128Traits) PutValue(b []byte, v decimal128.Num) {
- binary.LittleEndian.PutUint64(b[:8], uint64(v.LowBits()))
- binary.LittleEndian.PutUint64(b[8:], uint64(v.HighBits()))
+ if runtime.GOARCH == "s390x" {
Review comment:
A simple 'endian' (or another name) package containing some files like this might help reduce the if statement spam, and address @emkornfield's concerns (though the compiler is pretty good at removing those branches):
big.go:
```go
// +build s390x
package endian
import "encoding/binary"
var Native = binary.BigEndian
```
little.go:
```go
// +build !s390x
package endian
import "encoding/binary"
var Native = binary.LittleEndian
```
Then you can just use `endian.Native.*` functions instead of having if statements. Plus support for other big endian platforms becomes easy.
(Note that even though Native is a variable the methods called on it will still be resolved at compile time because the type is constant).
##########
File path: go/arrow/type_traits_decimal128.go
##########
@@ -39,8 +40,13 @@ func (decimal128Traits) BytesRequired(n int) int { return Decimal128SizeBytes *
// PutValue
func (decimal128Traits) PutValue(b []byte, v decimal128.Num) {
- binary.LittleEndian.PutUint64(b[:8], uint64(v.LowBits()))
- binary.LittleEndian.PutUint64(b[8:], uint64(v.HighBits()))
+ if runtime.GOARCH == "s390x" {
Review comment:
This package would also be a good place for a `IsBigEndian` constant.
##########
File path: go/arrow/type_traits_decimal128.go
##########
@@ -39,8 +40,13 @@ func (decimal128Traits) BytesRequired(n int) int { return Decimal128SizeBytes *
// PutValue
func (decimal128Traits) PutValue(b []byte, v decimal128.Num) {
- binary.LittleEndian.PutUint64(b[:8], uint64(v.LowBits()))
- binary.LittleEndian.PutUint64(b[8:], uint64(v.HighBits()))
+ if runtime.GOARCH == "s390x" {
Review comment:
A simple 'endian' (or another name) package containing some files like this might help reduce the if statement spam, and address @emkornfield's concerns (though the compiler is pretty good at removing those branches):
big.go:
```go
// +build s390x
package endian
import "encoding/binary"
var Native = binary.BigEndian
```
little.go:
```go
// +build !s390x
package endian
import "encoding/binary"
var Native = binary.LittleEndian
```
Then you can just use `endian.Native.*` functions instead of having if statements. Plus support for other big endian platforms becomes easy.
(Note that even though Native is a variable the methods called on it will still be resolved at compile time because the type is constant).
##########
File path: go/arrow/type_traits_decimal128.go
##########
@@ -39,8 +40,13 @@ func (decimal128Traits) BytesRequired(n int) int { return Decimal128SizeBytes *
// PutValue
func (decimal128Traits) PutValue(b []byte, v decimal128.Num) {
- binary.LittleEndian.PutUint64(b[:8], uint64(v.LowBits()))
- binary.LittleEndian.PutUint64(b[8:], uint64(v.HighBits()))
+ if runtime.GOARCH == "s390x" {
Review comment:
This package would also be a good place for a `IsBigEndian` constant.
##########
File path: go/arrow/type_traits_decimal128.go
##########
@@ -39,8 +40,13 @@ func (decimal128Traits) BytesRequired(n int) int { return Decimal128SizeBytes *
// PutValue
func (decimal128Traits) PutValue(b []byte, v decimal128.Num) {
- binary.LittleEndian.PutUint64(b[:8], uint64(v.LowBits()))
- binary.LittleEndian.PutUint64(b[8:], uint64(v.HighBits()))
+ if runtime.GOARCH == "s390x" {
Review comment:
A simple 'endian' (or another name) package containing some files like this might help reduce the if statement spam, and address @emkornfield's concerns (though the compiler is pretty good at removing those branches):
big.go:
```go
// +build s390x
package endian
import "encoding/binary"
var Native = binary.BigEndian
```
little.go:
```go
// +build !s390x
package endian
import "encoding/binary"
var Native = binary.LittleEndian
```
Then you can just use `endian.Native.*` functions instead of having if statements. Plus support for other big endian platforms becomes easy.
(Note that even though Native is a variable the methods called on it will still be resolved at compile time because the type is constant).
##########
File path: go/arrow/type_traits_decimal128.go
##########
@@ -39,8 +40,13 @@ func (decimal128Traits) BytesRequired(n int) int { return Decimal128SizeBytes *
// PutValue
func (decimal128Traits) PutValue(b []byte, v decimal128.Num) {
- binary.LittleEndian.PutUint64(b[:8], uint64(v.LowBits()))
- binary.LittleEndian.PutUint64(b[8:], uint64(v.HighBits()))
+ if runtime.GOARCH == "s390x" {
Review comment:
This package would also be a good place for a `IsBigEndian` constant.
##########
File path: go/arrow/type_traits_decimal128.go
##########
@@ -39,8 +40,13 @@ func (decimal128Traits) BytesRequired(n int) int { return Decimal128SizeBytes *
// PutValue
func (decimal128Traits) PutValue(b []byte, v decimal128.Num) {
- binary.LittleEndian.PutUint64(b[:8], uint64(v.LowBits()))
- binary.LittleEndian.PutUint64(b[8:], uint64(v.HighBits()))
+ if runtime.GOARCH == "s390x" {
Review comment:
A simple 'endian' (or another name) package containing some files like this might help reduce the if statement spam, and address @emkornfield's concerns (though the compiler is pretty good at removing those branches):
big.go:
```go
// +build s390x
package endian
import "encoding/binary"
var Native = binary.BigEndian
```
little.go:
```go
// +build !s390x
package endian
import "encoding/binary"
var Native = binary.LittleEndian
```
Then you can just use `endian.Native.*` functions instead of having if statements. Plus support for other big endian platforms becomes easy.
(Note that even though Native is a variable the methods called on it will still be resolved at compile time because the type is constant).
##########
File path: go/arrow/type_traits_decimal128.go
##########
@@ -39,8 +40,13 @@ func (decimal128Traits) BytesRequired(n int) int { return Decimal128SizeBytes *
// PutValue
func (decimal128Traits) PutValue(b []byte, v decimal128.Num) {
- binary.LittleEndian.PutUint64(b[:8], uint64(v.LowBits()))
- binary.LittleEndian.PutUint64(b[8:], uint64(v.HighBits()))
+ if runtime.GOARCH == "s390x" {
Review comment:
This package would also be a good place for a `IsBigEndian` constant.
##########
File path: go/arrow/type_traits_decimal128.go
##########
@@ -39,8 +40,13 @@ func (decimal128Traits) BytesRequired(n int) int { return Decimal128SizeBytes *
// PutValue
func (decimal128Traits) PutValue(b []byte, v decimal128.Num) {
- binary.LittleEndian.PutUint64(b[:8], uint64(v.LowBits()))
- binary.LittleEndian.PutUint64(b[8:], uint64(v.HighBits()))
+ if runtime.GOARCH == "s390x" {
Review comment:
A simple 'endian' (or another name) package containing some files like this might help reduce the if statement spam, and address @emkornfield's concerns (though the compiler is pretty good at removing those branches):
big.go:
```go
// +build s390x
package endian
import "encoding/binary"
var Native = binary.BigEndian
```
little.go:
```go
// +build !s390x
package endian
import "encoding/binary"
var Native = binary.LittleEndian
```
Then you can just use `endian.Native.*` functions instead of having if statements. Plus support for other big endian platforms becomes easy.
(Note that even though Native is a variable the methods called on it will still be resolved at compile time because the type is constant).
##########
File path: go/arrow/type_traits_decimal128.go
##########
@@ -39,8 +40,13 @@ func (decimal128Traits) BytesRequired(n int) int { return Decimal128SizeBytes *
// PutValue
func (decimal128Traits) PutValue(b []byte, v decimal128.Num) {
- binary.LittleEndian.PutUint64(b[:8], uint64(v.LowBits()))
- binary.LittleEndian.PutUint64(b[8:], uint64(v.HighBits()))
+ if runtime.GOARCH == "s390x" {
Review comment:
This package would also be a good place for a `IsBigEndian` constant.
##########
File path: go/arrow/type_traits_decimal128.go
##########
@@ -39,8 +40,13 @@ func (decimal128Traits) BytesRequired(n int) int { return Decimal128SizeBytes *
// PutValue
func (decimal128Traits) PutValue(b []byte, v decimal128.Num) {
- binary.LittleEndian.PutUint64(b[:8], uint64(v.LowBits()))
- binary.LittleEndian.PutUint64(b[8:], uint64(v.HighBits()))
+ if runtime.GOARCH == "s390x" {
Review comment:
A simple 'endian' (or another name) package containing some files like this might help reduce the if statement spam, and address @emkornfield's concerns (though the compiler is pretty good at removing those branches):
big.go:
```go
// +build s390x
package endian
import "encoding/binary"
var Native = binary.BigEndian
```
little.go:
```go
// +build !s390x
package endian
import "encoding/binary"
var Native = binary.LittleEndian
```
Then you can just use `endian.Native.*` functions instead of having if statements. Plus support for other big endian platforms becomes easy.
(Note that even though Native is a variable the methods called on it will still be resolved at compile time because the type is constant).
##########
File path: go/arrow/type_traits_decimal128.go
##########
@@ -39,8 +40,13 @@ func (decimal128Traits) BytesRequired(n int) int { return Decimal128SizeBytes *
// PutValue
func (decimal128Traits) PutValue(b []byte, v decimal128.Num) {
- binary.LittleEndian.PutUint64(b[:8], uint64(v.LowBits()))
- binary.LittleEndian.PutUint64(b[8:], uint64(v.HighBits()))
+ if runtime.GOARCH == "s390x" {
Review comment:
This package would also be a good place for a `IsBigEndian` constant.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] vivkong commented on a change in pull request #8011: ARROW-9803: [Go] Add initial support for s390x
Posted by GitBox <gi...@apache.org>.
vivkong commented on a change in pull request #8011:
URL: https://github.com/apache/arrow/pull/8011#discussion_r476454888
##########
File path: go/arrow/type_traits_decimal128.go
##########
@@ -39,8 +40,13 @@ func (decimal128Traits) BytesRequired(n int) int { return Decimal128SizeBytes *
// PutValue
func (decimal128Traits) PutValue(b []byte, v decimal128.Num) {
- binary.LittleEndian.PutUint64(b[:8], uint64(v.LowBits()))
- binary.LittleEndian.PutUint64(b[8:], uint64(v.HighBits()))
+ if runtime.GOARCH == "s390x" {
Review comment:
`runtime.GOARCH` is "recorded at compile time and made available by constants or functions in this package, but they do not influence the execution of the run-time system" - according to https://golang.org/pkg/runtime/
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou closed pull request #8011: ARROW-9803: [Go] Add initial support for s390x
Posted by GitBox <gi...@apache.org>.
kou closed pull request #8011:
URL: https://github.com/apache/arrow/pull/8011
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kiszk commented on a change in pull request #8011: ARROW-9803: [Go] Add initial support for s390x
Posted by GitBox <gi...@apache.org>.
kiszk commented on a change in pull request #8011:
URL: https://github.com/apache/arrow/pull/8011#discussion_r473781339
##########
File path: go/arrow/type_traits_decimal128.go
##########
@@ -39,8 +40,13 @@ func (decimal128Traits) BytesRequired(n int) int { return Decimal128SizeBytes *
// PutValue
func (decimal128Traits) PutValue(b []byte, v decimal128.Num) {
- binary.LittleEndian.PutUint64(b[:8], uint64(v.LowBits()))
- binary.LittleEndian.PutUint64(b[8:], uint64(v.HighBits()))
+ if runtime.GOARCH == "s390x" {
Review comment:
Good point.
Can we use `const` at the package level to evaluate this once? For example,
```
package ...
const BigEndian = runtime.GOARCH == "s390x"
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] emkornfield commented on a change in pull request #8011: ARROW-9803: [Go] Add initial support for s390x
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #8011:
URL: https://github.com/apache/arrow/pull/8011#discussion_r473424122
##########
File path: go/arrow/type_traits_decimal128.go
##########
@@ -39,8 +40,13 @@ func (decimal128Traits) BytesRequired(n int) int { return Decimal128SizeBytes *
// PutValue
func (decimal128Traits) PutValue(b []byte, v decimal128.Num) {
- binary.LittleEndian.PutUint64(b[:8], uint64(v.LowBits()))
- binary.LittleEndian.PutUint64(b[8:], uint64(v.HighBits()))
+ if runtime.GOARCH == "s390x" {
Review comment:
Is this a compile time check? Will it cause a branch at runtime?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] emkornfield commented on a change in pull request #8011: ARROW-9803: [Go] Add initial support for s390x
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #8011:
URL: https://github.com/apache/arrow/pull/8011#discussion_r476565438
##########
File path: go/arrow/type_traits_decimal128.go
##########
@@ -39,8 +40,13 @@ func (decimal128Traits) BytesRequired(n int) int { return Decimal128SizeBytes *
// PutValue
func (decimal128Traits) PutValue(b []byte, v decimal128.Num) {
- binary.LittleEndian.PutUint64(b[:8], uint64(v.LowBits()))
- binary.LittleEndian.PutUint64(b[8:], uint64(v.HighBits()))
+ if runtime.GOARCH == "s390x" {
Review comment:
I found this an issue discussing [DCE](https://github.com/golang/go/issues/21424) in Go. Skimming the thread it seems that using build tags if feasible might be a better way to support multiple architectures.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] vivkong commented on pull request #8011: ARROW-9803: [Go] Add initial support for s390x
Posted by GitBox <gi...@apache.org>.
vivkong commented on pull request #8011:
URL: https://github.com/apache/arrow/pull/8011#issuecomment-725525017
Hello @emkornfield, wondering if this can be considered for merging? I've updated the PR to use a constant to check for endianess. Thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mundaym commented on a change in pull request #8011: ARROW-9803: [Go] Add initial support for s390x
Posted by GitBox <gi...@apache.org>.
mundaym commented on a change in pull request #8011:
URL: https://github.com/apache/arrow/pull/8011#discussion_r487033507
##########
File path: go/arrow/type_traits_decimal128.go
##########
@@ -39,8 +40,13 @@ func (decimal128Traits) BytesRequired(n int) int { return Decimal128SizeBytes *
// PutValue
func (decimal128Traits) PutValue(b []byte, v decimal128.Num) {
- binary.LittleEndian.PutUint64(b[:8], uint64(v.LowBits()))
- binary.LittleEndian.PutUint64(b[8:], uint64(v.HighBits()))
+ if runtime.GOARCH == "s390x" {
Review comment:
A simple 'endian' (or another name) package containing some files like this might help reduce the if statement spam, and address @emkornfield's concerns (though the compiler is pretty good at removing those branches):
big.go:
```go
// +build s390x
package endian
import "encoding/binary"
var Native = binary.BigEndian
```
little.go:
```go
// +build !s390x
package endian
import "encoding/binary"
var Native = binary.LittleEndian
```
Then you can just use `endian.Native.*` functions instead of having if statements. Plus support for other big endian platforms becomes easy.
(Note that even though Native is a variable the methods called on it will still be resolved at compile time because the type is constant).
##########
File path: go/arrow/type_traits_decimal128.go
##########
@@ -39,8 +40,13 @@ func (decimal128Traits) BytesRequired(n int) int { return Decimal128SizeBytes *
// PutValue
func (decimal128Traits) PutValue(b []byte, v decimal128.Num) {
- binary.LittleEndian.PutUint64(b[:8], uint64(v.LowBits()))
- binary.LittleEndian.PutUint64(b[8:], uint64(v.HighBits()))
+ if runtime.GOARCH == "s390x" {
Review comment:
This package would also be a good place for a `IsBigEndian` constant.
##########
File path: go/arrow/type_traits_decimal128.go
##########
@@ -39,8 +40,13 @@ func (decimal128Traits) BytesRequired(n int) int { return Decimal128SizeBytes *
// PutValue
func (decimal128Traits) PutValue(b []byte, v decimal128.Num) {
- binary.LittleEndian.PutUint64(b[:8], uint64(v.LowBits()))
- binary.LittleEndian.PutUint64(b[8:], uint64(v.HighBits()))
+ if runtime.GOARCH == "s390x" {
Review comment:
A simple 'endian' (or another name) package containing some files like this might help reduce the if statement spam, and address @emkornfield's concerns (though the compiler is pretty good at removing those branches):
big.go:
```go
// +build s390x
package endian
import "encoding/binary"
var Native = binary.BigEndian
```
little.go:
```go
// +build !s390x
package endian
import "encoding/binary"
var Native = binary.LittleEndian
```
Then you can just use `endian.Native.*` functions instead of having if statements. Plus support for other big endian platforms becomes easy.
(Note that even though Native is a variable the methods called on it will still be resolved at compile time because the type is constant).
##########
File path: go/arrow/type_traits_decimal128.go
##########
@@ -39,8 +40,13 @@ func (decimal128Traits) BytesRequired(n int) int { return Decimal128SizeBytes *
// PutValue
func (decimal128Traits) PutValue(b []byte, v decimal128.Num) {
- binary.LittleEndian.PutUint64(b[:8], uint64(v.LowBits()))
- binary.LittleEndian.PutUint64(b[8:], uint64(v.HighBits()))
+ if runtime.GOARCH == "s390x" {
Review comment:
This package would also be a good place for a `IsBigEndian` constant.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kiszk commented on a change in pull request #8011: ARROW-9803: [Go] Add initial support for s390x
Posted by GitBox <gi...@apache.org>.
kiszk commented on a change in pull request #8011:
URL: https://github.com/apache/arrow/pull/8011#discussion_r473781339
##########
File path: go/arrow/type_traits_decimal128.go
##########
@@ -39,8 +40,13 @@ func (decimal128Traits) BytesRequired(n int) int { return Decimal128SizeBytes *
// PutValue
func (decimal128Traits) PutValue(b []byte, v decimal128.Num) {
- binary.LittleEndian.PutUint64(b[:8], uint64(v.LowBits()))
- binary.LittleEndian.PutUint64(b[8:], uint64(v.HighBits()))
+ if runtime.GOARCH == "s390x" {
Review comment:
Good point.
Can we use `const` at the package level? For example,
```
package ...
const BigEndian = runtime.GOARCH == "s390x"
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] vivkong commented on pull request #8011: ARROW-9803: [Go] Add initial support for s390x
Posted by GitBox <gi...@apache.org>.
vivkong commented on pull request #8011:
URL: https://github.com/apache/arrow/pull/8011#issuecomment-677622070
> How about adding a CI job for s390x + Go on Travis CI as the first step like #7938 ?
I can definitely look into adding that. Will contact @kiszk.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on pull request #8011: ARROW-9803: [Go] Add initial support for s390x
Posted by GitBox <gi...@apache.org>.
kou commented on pull request #8011:
URL: https://github.com/apache/arrow/pull/8011#issuecomment-676697902
How about adding a CI job for s390x + Go on Travis CI as the first step like https://github.com/apache/arrow/pull/7938 ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mundaym commented on a change in pull request #8011: ARROW-9803: [Go] Add initial support for s390x
Posted by GitBox <gi...@apache.org>.
mundaym commented on a change in pull request #8011:
URL: https://github.com/apache/arrow/pull/8011#discussion_r487033507
##########
File path: go/arrow/type_traits_decimal128.go
##########
@@ -39,8 +40,13 @@ func (decimal128Traits) BytesRequired(n int) int { return Decimal128SizeBytes *
// PutValue
func (decimal128Traits) PutValue(b []byte, v decimal128.Num) {
- binary.LittleEndian.PutUint64(b[:8], uint64(v.LowBits()))
- binary.LittleEndian.PutUint64(b[8:], uint64(v.HighBits()))
+ if runtime.GOARCH == "s390x" {
Review comment:
A simple 'endian' (or another name) package containing some files like this might help reduce the if statement spam, and address @emkornfield's concerns (though the compiler is pretty good at removing those branches):
big.go:
```go
// +build s390x
package endian
import "encoding/binary"
var Native = binary.BigEndian
```
little.go:
```go
// +build !s390x
package endian
import "encoding/binary"
var Native = binary.LittleEndian
```
Then you can just use `endian.Native.*` functions instead of having if statements. Plus support for other big endian platforms becomes easy.
(Note that even though Native is a variable the methods called on it will still be resolved at compile time because the type is constant).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org