Add MonoidAggregator combinators #659
Conversation
|
it looks like internally |
Codecov Report
@@ Coverage Diff @@
## develop #659 +/- ##
===========================================
+ Coverage 89.44% 89.55% +0.11%
===========================================
Files 113 118 +5
Lines 8945 9078 +133
Branches 490 491 +1
===========================================
+ Hits 8001 8130 +129
- Misses 944 948 +4
Continue to review full report at Codecov.
|
| * val b: MonoidAggregator[Animal, ShapeStats, Result] = ... | ||
| * | ||
| * val m1: Aggregator[Animal, ColorStats :: ShapeStats :: HNil, Result] = | ||
| * CombinedAggregators(a :: b :: HNil) |
There was a problem hiding this comment.
Not sure about all the HList stuff but this seems like a combinator on ApplicativeAggregators. I would think that if I have:
val a: MonoidAggregator[Animal, ColorStats, Result] = ...
val b: MonoidAggregator[Animal, ShapeStats, Result]= ..I should be able to do something like this
ApplicativeAggregators(a::b::HNil).andThenPresent{ case (a,b) => implicitly[Semigroup[Result]].combine(a,b) }
There was a problem hiding this comment.
yeah, it's the same concept, it is just implemented directly, which is actually in a way easier due to all the shapeless work.
| * We can also say: | ||
| * | ||
| * val m3: MonoidAggregator[Animal, DogStats :: CatStats :: HNil, Result] = | ||
| * JoinedAggregators(a :: b :: HNil).composePrepare(Generic[Animal].to) |
There was a problem hiding this comment.
Same comment as above, why cant be implement this as a combinator using ApplicativeAggregators?
There was a problem hiding this comment.
Can you show me how? Suggest a change?
There was a problem hiding this comment.
@johnynek are these compatible with algebird-spark? I wonder if we will run into Serialization issues when HList based aggregators cross execution boundaries
|
Good point! We also have kryo serializers for HLists (easy to do, but a good idea to add). We don’t customize our kryo serializers internally as far as I know (much less spark than scalding) but we did set up for scalding. |
|
@johnynek can we get this merged? Would love to use it at my work |
|
|
this some code we have been using at Stripe written by @non and myself.
We should support probably non-monoid (that is Semigroup based Aggregator) as well, although we try to use MonoidAggregator primarily at Stripe.
cc @erik-stripe