crunch-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Gabriel Reid" <gr...@apache.org>
Subject Re: Review Request: Add Aggregators that can operate over Collections and Maps
Date Sun, 23 Dec 2012 08:48:18 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message