A happy +1. This simplifies the code base, and if we find a compelling use, it shouldn't be too bad to add it back in.
On Fri, Apr 21, 2017 at 10:24 AM, Kenneth Knowles <k...@google.com.invalid> wrote: > Hi all, > > I propose that we remove KeyedCombineFn before the first stable release. > > I don't think it adds enough value for the complexity it adds to e.g. > CombineWithContext [1] and state [2, 3], and it doesn't seem to me that > users really use it when we might expect. I am happy to be demonstrated > wrong. > > It is very likely that you have never written [4, 5] or thought about > KeyedCombineFn. So for context, here are excepts from signatures just to > show the difference from CombineFn: > > CombineFn<InputT, AccumT, OutputT> { > AccumT createAccumulator(); > AccumT addInput(AccumT accum, InputT input); > AccumT mergeAccumulators(Iterable<AccumT> accums); > OutputT extractOutput(AccumT accum); > } > > KeyedCombineFn<K, InputT, AccumT, OutputT> { > AccumT createAccumulator(K key); > AccumT addInput(K key, AccumT accum, InputT input); > AccumT mergeAccumulators(K key, Iterable<AccumT> accums); > OutputT extractOutput(K key, AccumT accum); > } > > So what are the particular reasons for this, versus a CombineFn that has > KVs as its input and accumulator types? > > - There are some performance improvements potentially from not passing > keys around, based on the assumption they are always available. > > - There is also a spec difference because it only has to be associative > and commutative per key, cannot be applied in a global combine, and > addInput is automatically key preserving. > > But in fact, in all of my code crawling the class is almost never used > (even over the course of its history at Google) and even the few uses I > found were often mistakes where the key is totally ignored, probably > because a user thinks "I am doing a keyed combine so I need a keyed combine > function". So the number of users actually affected is about zero. > > I would be curious if anyone has a compelling case for keeping > KeyedCombineFn. > > Kenn > > [1] > https://github.com/yafengguo/Apache-beam/blob/master/sdks/ > java/core/src/main/java/org/apache/beam/sdk/transforms/ > CombineWithContext.java > [2] https://issues.apache.org/jira/browse/BEAM-1336 > [3] https://github.com/apache/beam/pull/2627 > [4] > https://github.com/search?l=Java&q=KeyedCombineFn&ref= > advsearch&type=Code&utf8=%E2%9C%93 > [5] https://www.google.com/search?q=KeyedCombineFn >