[GitHub] incubator-flink pull request: [FLINK-758] Add initial value to Gen...

[zentol]

GitHub user uce opened a pull request:

    https://github.com/apache/incubator-flink/pull/20

    [FLINK-758] Add initial value to GenericReduce and adjust AllReduceDriver

    This is an initial version of introducing an initial value to the reduce functions. Thanks to @fhueske for suggesting to add the initial value to `GenericReduce`.
   
    It is only implemented for `AllReduce` (ungrouped reduce). Please give feedback. If everything is OK, I would continue (~ 1 hr) with the remaining reduce variants (grouped reduce, ungrouped groupreduce, and grouped groupreduce).

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/uce/incubator-flink FLINK-758

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/incubator-flink/pull/20.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #20
   
----
commit 9ef87ff169f602ad41f61192c2872bfadaaa94d9
Author: uce <[hidden email]>
Date:   2014-06-15T18:24:17Z

    [FLINK-758] Add initial value to GenericReduce and adjust AllReduceDriver (first shot at reduce with initial value)

----


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---

[zentol]

Github user uce commented on the pull request:

    https://github.com/apache/incubator-flink/pull/20#issuecomment-46131099
 
    PS: currently this is not working correctly.


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---

[zentol]

Github user rmetzger commented on the pull request:

    https://github.com/apache/incubator-flink/pull/20#issuecomment-46152513
 
    Looks good to me


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---

[zentol]

Github user fhueske commented on the pull request:

    https://github.com/apache/incubator-flink/pull/20#issuecomment-46162167
 
    We don't need the initial value for GroupReduce ( != grouped Reduce).
    If an initial value is required for GroupReduce, it can be coded in the GroupReduceFunction.


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---

[zentol]

Github user uce commented on the pull request:

    https://github.com/apache/incubator-flink/pull/20#issuecomment-46170145
 
    True, but it would be more consistent, no?


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---

[zentol]

Github user StephanEwen commented on the pull request:

    https://github.com/apache/incubator-flink/pull/20#issuecomment-46177245
 
    I don't like that initial value think for group reduce too much.
   
    We have it in the interface GroupReduceFunction and only provide it to the AllGroupReduce? Not the grouped one? I would rather leave it then, and only use it for the fold style reduce.


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---

[zentol]

Github user uce commented on the pull request:

    https://github.com/apache/incubator-flink/pull/20#issuecomment-46177838
 
    No, the idea was to provide it to the grouped one as well (I think I've enumerated all reduce variants in the initial issue description). But I'm unsure if it makes much sense to have an initial value per group. I guess that should be the discussion point. ;-)


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---

[zentol]

Github user fhueske commented on the pull request:

    https://github.com/apache/incubator-flink/pull/20#issuecomment-46186920
 
    I think it is good to have the initial value for the all ReduceFunction and grouped ReduceFunction, but not for the GroupReduceFunction. In the GroupReduceFunction (the one with the iterator) it can be specified in the UDF. There is no need to ingest a value into the iterator.


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---

[zentol]

Github user StephanEwen commented on the pull request:

    https://github.com/apache/incubator-flink/pull/20#issuecomment-46467179
 
    Why do you add the "setInitialValue()" method to the reduce function? I think that is a bit clumsy. Is that for getting the value into the runtime?
   
    Why not add it as part of the user parameters? That keeps the user-facing APIs clean.


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---

[zentol]

Github user uce commented on the pull request:

    https://github.com/apache/incubator-flink/pull/20#issuecomment-46469440
 
    I also find it clumsy. Will change it as you suggested. I'm not sure why I didn't think of this myself.
   
    Sent from my iPhone
   
    > On 18 Jun 2014, at 19:32, Stephan Ewen <[hidden email]> wrote:
    >
    > Why do you add the "setInitialValue()" method to the reduce function? I think that is a bit clumsy. Is that for getting the value into the runtime?
    >
    > Why not add it as part of the user parameters? That keeps the user-facing APIs clean.
    >
    > —
    > Reply to this email directly or view it on GitHub.


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---