Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade arrow dependency and unskip arrow tests #1209

Closed
mark-rushakoff opened this issue Apr 24, 2019 · 2 comments · Fixed by #1220
Closed

Upgrade arrow dependency and unskip arrow tests #1209

mark-rushakoff opened this issue Apr 24, 2019 · 2 comments · Fixed by #1220

Comments

@mark-rushakoff
Copy link
Contributor

There are 3 tests in arrow/arrow_test.go that are skipped with a link to https://issues.apache.org/jira/browse/ARROW-4081.

flux/arrow/arrow_test.go

Lines 13 to 65 in 4c6a7d4

func TestSum_Float64_Empty(t *testing.T) {
t.Skip("https://issues.apache.org/jira/browse/ARROW-4081")
b := array.NewFloat64Builder(arrowmemory.NewGoAllocator())
vs := b.NewFloat64Array()
b.Release()
defer func() {
if err := recover(); err != nil {
t.Errorf("unexpected panic: %s", err)
}
}()
if got, want := math.Float64.Sum(vs), float64(0); got != want {
t.Errorf("unexpected sum: %v != %v", got, want)
}
}
func TestSum_Int64_Empty(t *testing.T) {
t.Skip("https://issues.apache.org/jira/browse/ARROW-4081")
b := array.NewInt64Builder(arrowmemory.NewGoAllocator())
vs := b.NewInt64Array()
b.Release()
defer func() {
if err := recover(); err != nil {
t.Errorf("unexpected panic: %s", err)
}
}()
if got, want := math.Int64.Sum(vs), int64(0); got != want {
t.Errorf("unexpected sum: %v != %v", got, want)
}
}
func TestSum_Uint64_Empty(t *testing.T) {
t.Skip("https://issues.apache.org/jira/browse/ARROW-4081")
b := array.NewUint64Builder(arrowmemory.NewGoAllocator())
vs := b.NewUint64Array()
b.Release()
defer func() {
if err := recover(); err != nil {
t.Errorf("unexpected panic: %s", err)
}
}()
if got, want := math.Uint64.Sum(vs), uint64(0); got != want {
t.Errorf("unexpected sum: %v != %v", got, want)
}
}

Following that jira link, the issue is listed as having been resolved by apache/arrow#3906, which was merged a little over a month ago.

@jsternberg
Copy link
Contributor

There is also this issue so that we can optimize constructing the group key columns: https://issues.apache.org/jira/browse/ARROW-5212

The pull request to fix that is apache/arrow#4204.

@jsternberg
Copy link
Contributor

I looked at the places where we added a workaround and I think the current code is correct. We can call sum when the array is empty, but I'm not sure why we would. It doesn't save us any time since we need to do the same code anyway so I'm just going to remove the comment since the current code is correct.

In each of the places where we do this check, we have to check to see if the array is empty anyway because there's something special. In sum, we set ok to true only if the length is greater than zero. In mean, we add the number of points for the total count of points.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants