Skip to content

Commit

Permalink
apacheGH-35337: [Go] ASAN tests fail with Go1.20+ (apache#35338)
Browse files Browse the repository at this point in the history
### Rationale for this change
Fixing crashes when using ASAN to run unit tests under Go1.20+, requires features that were only added in go1.20. For versions <=go.19, the code remains unchanged.

The only way to properly test these changes would be to increase the matrix of jobs for Go, but we already have quite a few jobs. @ assignUser Do you have any issues with adding go1.20 (or maybe just telling it to use "latest") as another set of Go jobs to the GHA config?
* Closes: apache#35337

Lead-authored-by: Matt Topol <zotthewizard@gmail.com>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
  • Loading branch information
2 people authored and ArgusLi committed May 15, 2023
1 parent 070b253 commit a9ec156
Show file tree
Hide file tree
Showing 31 changed files with 223 additions and 156 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/go.yml
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ jobs:
name: AMD64 Debian 11 Go ${{ matrix.go }} - CGO
runs-on: ubuntu-latest
if: ${{ !contains(github.event.pull_request.title, 'WIP') }}
timeout-minutes: 15
timeout-minutes: 20
strategy:
fail-fast: false
matrix:
Expand Down
7 changes: 5 additions & 2 deletions dev/tasks/tasks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1411,14 +1411,17 @@ tasks:
R_PRUNE_DEPS: TRUE
image: fedora-r-clang-sanitizer

test-debian-11-go-1.17:
{% for go_version, staticcheck in [("1.17", "v0.2.2"), ("1.20", "latest")] %}
test-debian-11-go-{{ go_version }}:
ci: azure
template: docker-tests/azure.linux.yml
params:
env:
DEBIAN: 11
GO: 1.17
GO: "{{go_version}}"
STATICCHECK: "{{ staticcheck }}"
image: debian-go
{% endfor %}

test-ubuntu-default-docs:
ci: azure
Expand Down
6 changes: 4 additions & 2 deletions docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1508,6 +1508,7 @@ services:
volumes: *debian-volumes
command: &go-command >
/bin/bash -c "
git config --global --add safe.directory /arrow &&
/arrow/ci/scripts/go_build.sh /arrow &&
/arrow/ci/scripts/go_test.sh /arrow"

Expand Down Expand Up @@ -1544,8 +1545,9 @@ services:
shm_size: *shm-size
volumes: *debian-volumes
command: &go-cgo-python-command >
/bin/bash -c "
/arrow/ci/scripts/go_cgo_python_test.sh /arrow"
/bin/bash -c "
git config --global --add safe.directory /arrow &&
/arrow/ci/scripts/go_cgo_python_test.sh /arrow"

############################# JavaScript ####################################

Expand Down
11 changes: 5 additions & 6 deletions go/arrow/array/numeric.gen.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

21 changes: 11 additions & 10 deletions go/arrow/array/numericbuilder.gen.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 4 additions & 4 deletions go/arrow/array/numericbuilder.gen.go.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -225,23 +225,23 @@ func (b *{{.Name}}Builder) AppendValueFromString(s string) error {
}
b.Append(v)
{{else if (eq .Name "Duration") -}}
return fmt.Errorf("%w: AppendValueFromString not implemented for Duration", ErrNotImplemented)
return fmt.Errorf("%w: AppendValueFromString not implemented for Duration", arrow.ErrNotImplemented)
{{else if or (eq .Name "Int8") (eq .Name "Int16") (eq .Name "Int32") (eq .Name "Int64") -}}
v, err := strconv.ParseInt(s, 10, {{.Size}})
v, err := strconv.ParseInt(s, 10, {{.Size}} * 8)
if err != nil {
b.AppendNull()
return err
}
b.Append({{.name}}(v))
{{else if or (eq .Name "Uint8") (eq .Name "Uint16") (eq .Name "Uint32") (eq .Name "Uint64") -}}
v, err := strconv.ParseUint(s, 10, {{.Size}})
v, err := strconv.ParseUint(s, 10, {{.Size}} * 8)
if err != nil {
b.AppendNull()
return err
}
b.Append({{.name}}(v))
{{else if or (eq .Name "Float32") (eq .Name "Float64") -}}
v, err := strconv.ParseFloat(s, {{.Size}})
v, err := strconv.ParseFloat(s, {{.Size}} * 8)
if err != nil {
b.AppendNull()
return err
Expand Down
5 changes: 2 additions & 3 deletions go/arrow/arrio/arrio_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ package arrio_test
import (
"fmt"
"io"
"io/ioutil"
"os"
"testing"

Expand Down Expand Up @@ -93,13 +92,13 @@ func TestCopy(t *testing.T) {
mem := memory.NewCheckedAllocator(memory.NewGoAllocator())
defer mem.AssertSize(t, 0)

f, err := ioutil.TempFile(tempDir, "go-arrow-copy-")
f, err := os.CreateTemp(tempDir, "go-arrow-copy-")
if err != nil {
t.Fatal(err)
}
defer f.Close()

o, err := ioutil.TempFile(tempDir, "go-arrow-copy-")
o, err := os.CreateTemp(tempDir, "go-arrow-copy-")
if err != nil {
t.Fatal(err)
}
Expand Down
9 changes: 4 additions & 5 deletions go/arrow/compute/arithmetic.go
Original file line number Diff line number Diff line change
Expand Up @@ -909,7 +909,7 @@ func RegisterScalarArithmetic(reg FunctionRegistry) {
reg.AddFunction(fn, false)
}

fn = &arithmeticFunction{*NewScalarFunction("bit_wise_not", Unary(), EmptyFuncDoc), decPromoteNone}
fn = &arithmeticFunction{*NewScalarFunction("bit_wise_not", Unary(), bitWiseNotDoc), decPromoteNone}
for _, k := range kernels.GetBitwiseUnaryKernels() {
if err := fn.AddKernel(k); err != nil {
panic(err)
Expand Down Expand Up @@ -1087,10 +1087,9 @@ func Negate(ctx context.Context, opts ArithmeticOptions, input Datum) (Datum, er
// Sign returns -1, 0, or 1 depending on the sign of each element in the
// input. For x in the input:
//
// if x > 0: 1
// if x < 0: -1
// if x == 0: 0
//
// if x > 0: 1
// if x < 0: -1
// if x == 0: 0
func Sign(ctx context.Context, input Datum) (Datum, error) {
return CallFunction(ctx, "sign", nil, input)
}
Expand Down
5 changes: 2 additions & 3 deletions go/arrow/csv/reader_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (
"bytes"
stdcsv "encoding/csv"
"fmt"
"io/ioutil"
"log"
"os"
"strings"
Expand Down Expand Up @@ -329,7 +328,7 @@ func testCSVReader(t *testing.T, filepath string, withHeader bool, stringsCanBeN
mem := memory.NewCheckedAllocator(memory.NewGoAllocator())
defer mem.AssertSize(t, 0)

raw, err := ioutil.ReadFile(filepath)
raw, err := os.ReadFile(filepath)
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -469,7 +468,7 @@ func TestCSVReaderWithChunk(t *testing.T) {
mem := memory.NewCheckedAllocator(memory.NewGoAllocator())
defer mem.AssertSize(t, 0)

raw, err := ioutil.ReadFile("testdata/simple.csv")
raw, err := os.ReadFile("testdata/simple.csv")
if err != nil {
t.Fatal(err)
}
Expand Down
4 changes: 2 additions & 2 deletions go/arrow/csv/writer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import (
"bytes"
ecsv "encoding/csv"
"fmt"
"io/ioutil"
"io"
"log"
"testing"

Expand Down Expand Up @@ -379,7 +379,7 @@ func BenchmarkWrite(b *testing.B) {
rec := bldr.NewRecord()
defer rec.Release()

w := csv.NewWriter(ioutil.Discard, schema, csv.WithComma(';'), csv.WithCRLF(false))
w := csv.NewWriter(io.Discard, schema, csv.WithComma(';'), csv.WithCRLF(false))

b.ResetTimer()
for i := 0; i < b.N; i++ {
Expand Down
6 changes: 4 additions & 2 deletions go/arrow/flight/flightsql/example/sqlite_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,9 @@ import (
func genRandomString() []byte {
const length = 16
max := int('z')
min := int('0')
// don't include ':' as a valid byte to generate
// because we use it as a separator for the transactions
min := int('<')

out := make([]byte, length)
for i := range out {
Expand Down Expand Up @@ -249,7 +251,7 @@ func (s *SQLiteFlightSQLServer) DoGetStatement(ctx context.Context, cmd flightsq
if txnid != "" {
tx, loaded := s.openTransactions.Load(txnid)
if !loaded {
return nil, nil, fmt.Errorf("%w: invalid transaction id specified", arrow.ErrInvalid)
return nil, nil, fmt.Errorf("%w: invalid transaction id specified: %s", arrow.ErrInvalid, txnid)
}
db = tx.(*sql.Tx)
}
Expand Down
8 changes: 4 additions & 4 deletions go/arrow/internal/arrjson/arrjson_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ package arrjson
import (
"errors"
"io"
"io/ioutil"
"os"
"testing"

"github.com/apache/arrow/go/v12/arrow/array"
Expand Down Expand Up @@ -53,7 +53,7 @@ func TestReadWrite(t *testing.T) {
mem := memory.NewCheckedAllocator(memory.NewGoAllocator())
defer mem.AssertSize(t, 0)

f, err := ioutil.TempFile(tempDir, "go-arrow-read-write-")
f, err := os.CreateTemp(tempDir, "go-arrow-read-write-")
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -82,7 +82,7 @@ func TestReadWrite(t *testing.T) {
t.Fatalf("could not sync data to disk: %v", err)
}

fileBytes, _ := ioutil.ReadFile(f.Name())
fileBytes, _ := os.ReadFile(f.Name())
assert.JSONEq(t, wantJSONs[name], string(fileBytes))

_, err = f.Seek(0, io.SeekStart)
Expand All @@ -92,7 +92,7 @@ func TestReadWrite(t *testing.T) {

r, err := NewReader(f, WithAllocator(mem), WithSchema(recs[0].Schema()))
if err != nil {
raw, _ := ioutil.ReadFile(f.Name())
raw, _ := os.ReadFile(f.Name())
t.Fatalf("could not read JSON file: %v\n%v\n", err, string(raw))
}
defer r.Release()
Expand Down
5 changes: 2 additions & 3 deletions go/arrow/ipc/cmd/arrow-cat/main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (
"bytes"
"fmt"
"io"
"io/ioutil"
"os"
"testing"

Expand Down Expand Up @@ -176,7 +175,7 @@ record 3...
defer mem.AssertSize(t, 0)

fname := func() string {
f, err := ioutil.TempFile(tempDir, "go-arrow-cat-stream-")
f, err := os.CreateTemp(tempDir, "go-arrow-cat-stream-")
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -518,7 +517,7 @@ record 3/3...
defer mem.AssertSize(t, 0)

fname := func() string {
f, err := ioutil.TempFile(tempDir, "go-arrow-cat-file-")
f, err := os.CreateTemp(tempDir, "go-arrow-cat-file-")
if err != nil {
t.Fatal(err)
}
Expand Down

0 comments on commit a9ec156

Please sign in to comment.