----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/8577/#review14876 -----------------------------------------------------------
In general, I'm just repeating the same comment a bunch of times here -- the new copy method in Aggregator isn't calling the copy method on the underlying aggregator(s). I'm not totally sure if this will cause problems right now, but it just feels safer to call copy all the way down. I also have some comments to add on the approach in general, but I'll add those in JIRA. crunch/src/main/java/org/apache/crunch/fn/Aggregators.java <https://reviews.apache.org/r/8577/#comment31766> These should probably be calls to aggregators.get(0).copy() and aggregators.get(1).copy() crunch/src/main/java/org/apache/crunch/fn/Aggregators.java <https://reviews.apache.org/r/8577/#comment31767> Inner aggregators aren't being copied, but probably should be crunch/src/main/java/org/apache/crunch/fn/Aggregators.java <https://reviews.apache.org/r/8577/#comment31768> Inner aggregators aren't being copied, but probably should be crunch/src/main/java/org/apache/crunch/fn/Aggregators.java <https://reviews.apache.org/r/8577/#comment31769> Inner aggregators aren't being copied, but probably should be crunch/src/main/java/org/apache/crunch/fn/Aggregators.java <https://reviews.apache.org/r/8577/#comment31770> Inner aggregator isn't being copied, but probably should be crunch/src/main/java/org/apache/crunch/fn/Aggregators.java <https://reviews.apache.org/r/8577/#comment31771> Inner aggregator isn't being copied, but probably should be - Gabriel Reid On Dec. 13, 2012, 10:13 p.m., Josh Wills wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/8577/ > ----------------------------------------------------------- > > (Updated Dec. 13, 2012, 10:13 p.m.) > > > Review request for crunch and Matthias Friedrich. > > > Description > ------- > > Adds support for Aggregators that can be used as part of combineValues ops > that include collections and maps. The cost of the approach is two new > methods that were needed on the Aggregator interface: copy(), to create new > instances of a given Aggregator (e.g., one for each new key that is put into > the Map), and arity(), which indicates how many values will be in the > Iterable returned by the results() method if known ahead of time. > > > This addresses bug CRUNCH-133. > https://issues.apache.org/jira/browse/CRUNCH-133 > > > Diffs > ----- > > > crunch-contrib/src/main/java/org/apache/crunch/contrib/bloomfilter/BloomFilterFactory.java > 9191a6c > crunch/src/main/java/org/apache/crunch/Aggregator.java 432452b > crunch/src/main/java/org/apache/crunch/fn/Aggregators.java 0ac79e2 > crunch/src/test/java/org/apache/crunch/fn/AggregatorsTest.java 6ee1972 > > Diff: https://reviews.apache.org/r/8577/diff/ > > > Testing > ------- > > Unit tests on the collections and maps types included. > > > Thanks, > > Josh Wills > >
