feat(functions): converting all aggregates to use arrow arrays #532
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jsternberg
force-pushed
the
feat/arrow-aggregates
branch
from
a656db9 to
fa05d78
Compare
|
@jsternberg Can you make sure that Flux tests exist for each one of the aggregates this fixes? As well as confirm that the spec is up to date? Since we have to touch every transformation the plan is to take the time to make sure they all have tests and are documentation. |
nathanielc
reviewed
Dec 14, 2018
functions/transformations/sum.go
Outdated
| func (a *SumIntAgg) DoInt(vs []int64) { | ||
| for _, v := range vs { | ||
| func (a *SumIntAgg) DoInt(vs *array.Int64) { | ||
| for _, v := range vs.Int64Values() { |
There was a problem hiding this comment.
The arrow arrays have special methods for computing the sum. Can you use those methods here? Those methods are written using SIMD instructions.
wolffcm
approved these changes
Dec 14, 2018
jsternberg
force-pushed
the
feat/arrow-aggregates
branch
2 times, most recently
from
6ecb419 to
11a06a8
Compare
|
@nathanielc there is at least one test for each of the aggregates that are touched and each of the tests uses all of the possible data types for full test coverage. At the moment, using an incompatible data type fails the entire query rather than a specific table so I skipped invalid data types. In the future, we should test those too. |
nathanielc
approved these changes
Dec 19, 2018
| for _, v := range vs { | ||
| func (a *MeanAgg) DoInt(vs *array.Int64) { | ||
| a.count += float64(vs.Len()) | ||
| for _, v := range vs.Int64Values() { | ||
| //TODO handle overflow | ||
| a.sum += float64(v) | ||
| } |
There was a problem hiding this comment.
This should also use the math.Sum functions.
jsternberg
force-pushed
the
feat/arrow-aggregates
branch
from
11a06a8 to
b323c67
Compare
All aggregates will now use the arrow interface. They do not support nil at the moment.
|
I might be doing something wrong (or my hardware can't be consistent enough), but I'm not seeing a huge difference. |
jsternberg
force-pushed
the
feat/arrow-aggregates
branch
from
b323c67 to
ba7c765
Compare
|
I pushed the updated benchmark code and fixed an issue with |
|
@jsternberg This biggest perf advantage is when we use the special math.* functions. Only the Sum and Mean aggregates do that currently. I do see a strong improvement for Although I do not see benchmarks for BenchmarkSum or BenchmarkMean which do exist. Can you add those benchmarks? Also |
|
The reason why |
|
|
The speedup is now there and the PR is approved so I'm going to merge. |
This was referenced Dec 19, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
All aggregates will now use the arrow interface. They do not support nil
at the moment.
Fixes #491, #525, #511, #520, and #522.
This also implements part of #513. The other part uses the selector interface which is not included in this change.