Github user vasia commented on the pull request:
https://github.com/apache/flink/pull/576#issuecomment-93996607
Hi @andralungu! Thanks for the quick update ^^
The result looks good, but I have a few more comments/suggestions (apart
from the inline ones).
- +1 for the `groupReduce*` implementations. These look great :-)
- Regarding the combinable versions, I think that the current API is not
very intuitive. I understand that you did it like this because you were
restricted by the return type of the `ReduceFunction`. I think that it would be
better if we simplify these to operate directly on the values, instead of
tuples with nested Edge and Vertex data.
For example, say you want to compute the sum of all out-neighbors of a
vertex. The value types to combine inside the reduce function should be the
same as the vertex value type, instead of a Tuple3.
In the case of reducing on edges, the type should be the same as the edge
value type.
Thus, I would make `reduceOnEdges` return `DataSet<Tuple2<K, EV>` and
`reduceOnNeighbors` return `DataSet<Tuple2<K, VV>`, i.e. one value per vertex
ID.
- There is some inconsistency in the naming of the methods, for example
the user-defined methods in the group case are called `iterateOn...` and in the
reduce case they are called `reduce...`. We should stick to one of these :-)
Also, maybe we could find a better name for `ReduceEdgesFunction` and
`ReduceNeighborsFunction`. I don't have a suggestion right now, but I'll think
about it.
- Finally, there are several warnings in your code for unchecked and raw
types. Could you turn on warning notifications in your IDE and try to add
suppress annotations to get of them?
Let me know what you think and whether you have any questions!
Thanks again!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
