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

cpp/src/arrow/compute/test-util.h

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

cpp/src/arrow/compute/test-util.h

😄

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

cpp/src/arrow/compute/kernels/sum-internal.h

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

cpp/src/arrow/compute/kernels/sum-internal.h

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).

cpp/src/arrow/compute/kernels/mean.h

cpp/src/arrow/compute/kernels/sum-internal.h

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

cpp/src/arrow/compute/kernels/sum-internal.h

What is the performance diff of doing ternary operator?

cpp/src/arrow/util/bit-util.h

cpp/src/arrow/compute/kernels/mean.cc

cpp/src/arrow/compute/kernels/sum-internal.h

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

cpp/src/arrow/compute/kernels/sum-internal.h

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

cpp/src/arrow/compute/kernels/aggregate-test.cc

+                          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

cpp/src/arrow/compute/kernels/mean.cc

+  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 91d9b63 Compare February 22, 2019 02:21

fsaintjacques force-pushed the ARROW-3121-mean-aggregate branch from 02a0bcf to 2d4c2ba Compare 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 7929140 Compare 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 29aa925 Feb 28, 2019

asfimport mentioned this pull request Feb 28, 2019

[C++] Mean kernel aggregate #19474

Closed

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