ARROW-3121: [C++] Mean aggregate kernel by fsaintjacques · Pull Request #3708 · apache/arrow

fsaintjacques · 2019-02-20T00:00:34Z

Implements the mean (average) kernel aggregates on numeric columns. The final type is always a double. Refactored the Sum kernel implementation to share common parts, notably the consume part is identical. Only the Finalize and output type differ.

codecov-io · 2019-02-22T04:31:35Z

Codecov Report

Merging #3708 into master will increase coverage by 0.02%.
The diff coverage is 94.21%.

@@ Coverage Diff @@## master #3708 +/- ##
==========================================
+ Coverage 87.65% 87.68% +0.02% 
==========================================
Files 701 703 +2 Lines 85747 85824 +77 Branches 1200 1200 ==========================================
+ Hits 75162 75254 +92 + Misses 10466 10456 -10 + Partials 119 114 -5

Impacted Files	Coverage Δ
cpp/src/arrow/util/bit-util.h	`100% <100%> (ø)`	⬆️
cpp/src/arrow/util/bit-util-test.cc	`98.9% <100%> (+0.01%)`	⬆️
cpp/src/arrow/compute/kernels/aggregate-test.cc	`100% <100%> (ø)`	⬆️
cpp/src/arrow/compute/test-util.h	`94.73% <100%> (+4.73%)`	⬆️
cpp/src/arrow/compute/kernels/sum.cc	`87.8% <66.66%> (-4.79%)`	⬇️
cpp/src/arrow/compute/kernels/mean.cc	`87.8% <87.8%> (ø)`
cpp/src/arrow/compute/kernels/sum-internal.h	`93.84% <93.84%> (ø)`
go/arrow/math/uint64_sse4_amd64.go	`0% <0%> (-100%)`	⬇️
go/arrow/math/float64_sse4_amd64.go	`0% <0%> (-100%)`	⬇️
go/arrow/memory/memory_sse4_amd64.go	`0% <0%> (-100%)`	⬇️
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9814605...2d4c2ba. Read the comment docs.

fsaintjacques · 2019-02-22T15:35:15Z

I think the nodejs failure is unrelated and this is ready to merge.

fsaintjacques · 2019-02-22T15:35:49Z

@bkietz @pitrou @emkornfield This is ready to review.

pitrou

LGTM. Just one comment.

pitrou · 2019-02-25T12:30:16Z

Should assert false or abort on other datum kinds, no?

https://issues.apache.org/jira/browse/ARROW-4673

I'll also try to include this while implementing ARROW-3123.

bkietz · 2019-02-25T19:12:45Z

😄
I think you want typename std::enable_if<!IsFloatingPoint<Type>::value>::type, though

ugh, shame on me! Clearly something is wrong then.

bkietz · 2019-02-25T19:20:52Z

This (and ConsumeSparse, ConsumeTiny) looks pretty handy, maybe later we could refactor it into BitUtil::VisitBitmap() or so

Soon the complexity of future needs will outgrow the current implementation, e.g. we'll need to mix with selection vectors/bitmap, groups, etc. I intend to open a discussion in the mailing list about providing iterator facilities with many options (batch, masked, groups).

bkietz · 2019-02-25T19:51:16Z

crazy edge case: if we're summing floating point and one of the null masked values happens to be NaN then multiplying by zero will not prevent the sum from also being NaN

Good catch, I'll see what I can do without too much change.

What is the performance diff of doing ternary operator?

Also would it pay to have a constant expression (or just static array) in bit utils that precomputes all the shifts similar to the popcount array below

emkornfield

I don't think I saw any blocking issues mostly style stuff and documentation suggestions (please do fix the NaN issue raised).

emkornfield · 2019-02-26T04:09:46Z

What is the performance diff of doing ternary operator?

emkornfield · 2019-02-26T04:27:51Z

pitrou · 2019-02-26T13:29:23Z

Le 26/02/2019 à 14:27, François Saint-Jacques a écrit :

Soon the complexity of future needs will outgrow the current implementation, e.g. we'll need to mix with selection vectors/bitmap, groups, etc. I intend to open a discussion in the mailing list about providing iterator facilities with many options (batch, masked, groups).

Also take a look at existing facilities (ArrayDataVisitor) in visitor-inline.h Regards Antoine.

fsaintjacques · 2019-02-26T14:18:06Z

@pitrou, the initial (sum) implementation used the visitor facility.

fsaintjacques · 2019-02-27T14:21:46Z

This is good to go, the failure is from python https://issues.apache.org/jira/browse/ARROW-4684

wesm

+1

wesm · 2019-02-28T01:31:40Z

+ Datum(std::make_shared<ScalarType>(0.0)));
+
+ ValidateMean<TypeParam>(&this->ctx_, "[1, 1, 1, 1, 1, 1, 1, 1]",
+ Datum(std::make_shared<ScalarType>(1.0)));


For self-documenting code you could do:
auto null = std::make_shared<ScalarType>(0.0, false);
Also keep in mind that Datum has implicit ctors, so these Datum(...) are unneeded

wesm · 2019-02-28T01:33:12Z

+ if (data_type == nullptr)
+ return Status::Invalid("Datum must be array-like");
+ else if (!is_integer(data_type->id()) && !is_floating(data_type->id()))
+ return Status::Invalid("Datum must contain a NumericType");


I prefer to always use braces

fsaintjacques force-pushed the ARROW-3121-mean-aggregate branch 2 times, most recently from 7f31a91 to 91d9b63Compare February 22, 2019 02:21

fsaintjacques force-pushed the ARROW-3121-mean-aggregate branch from 02a0bcf to 2d4c2baCompare February 22, 2019 12:33

pitrou approved these changes Feb 25, 2019
View reviewed changes

bkietz reviewed Feb 25, 2019
View reviewed changes

bkietz approved these changes Feb 25, 2019
View reviewed changes

fsaintjacques added 3 commits February 25, 2019 21:13

Move TypeTraits into sum-internal.h
8bc293f

Refactor sum implementation
3a1a0cd

Implement mean aggregate
7929140

fsaintjacques force-pushed the ARROW-3121-mean-aggregate branch from 2d4c2ba to 7929140Compare February 26, 2019 02:40

emkornfield approved these changes Feb 26, 2019
View reviewed changes

fsaintjacques added 4 commits February 26, 2019 11:57

Add documentation per review
c448bcb

Deal with NaN values in sum
d1191fd

Refactor with ternary
d41db22

reformat
0d39c1f

wesm approved these changes Feb 28, 2019
View reviewed changes

wesm closed this in 29aa925Feb 28, 2019

asfimport mentioned this pull request Feb 28, 2019
[C++] Mean kernel aggregate #19474
Closed

Uh oh!

Conversation

fsaintjacques commented Feb 20, 2019

Uh oh!

codecov-io commented Feb 22, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

fsaintjacques commented Feb 22, 2019

Uh oh!

fsaintjacques commented Feb 22, 2019

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emkornfield left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pitrou commented Feb 26, 2019 via email

Uh oh!

fsaintjacques commented Feb 26, 2019

Uh oh!

fsaintjacques commented Feb 27, 2019

Uh oh!

wesm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-io commented Feb 22, 2019•
edited
Loading