Details

    • Type: New Feature
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.3.0
    • Component/s: DataStream API, Streaming
    • Labels:
      None

      Issue Links

        Activity

        Hide
        githubbot ASF GitHub Bot added a comment -

        GitHub user VenturaDelMonte opened a pull request:

        https://github.com/apache/flink/pull/2756

        FLINK-4997 Extending Window Function Metadata

        This PR aims to introduce what discussed in [FLIP-2](https://cwiki.apache.org/confluence/display/FLINK/FLIP-2+Extending+Window+Function+Metadata).
        WindowedStream apply methods have been overloaded in order to support ProcessWindowFunction (and its rich counterpart).
        Streaming runtime internals have been modified in order to support the new function, however fully backward compatibility to WindowFunction (and its rich counterpart) is guaranteed by silently wrapping it with a ProcessWindowFunction.
        This PR implementation strictly follow what decided in the FLIP, nothing has been changed for AllWindowedStream.
        Windows documentation has been overhauled in order to illustrate ProcessWindowFunction API.

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

        $ git pull https://github.com/VenturaDelMonte/flink flip-2

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

        https://github.com/apache/flink/pull/2756.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 #2756


        commit 5a33fe2b9e5c23e8529964a489465f51410432df
        Author: Ventura Del Monte <venturadelmonte@gmail.com>
        Date: 2016-09-21T14:04:00Z

        Merge remote-tracking branch 'apache/master'

        commit 39a8bb9433edf5bb9adb850c667b71ef8d25c6b6
        Author: Ventura Del Monte <venturadelmonte@gmail.com>
        Date: 2016-09-22T08:31:58Z

        Merge remote-tracking branch 'upstream/master'

        commit 2507a71e2a40b05b3e5c7507a1c32d6678e07810
        Author: Ventura Del Monte <venturadelmonte@gmail.com>
        Date: 2016-09-22T09:40:40Z

        Merge remote-tracking branch 'upstream/master'

        commit 57d0bca5681e5ea0ba63f3b95fe4f949af3734de
        Author: Ventura Del Monte <venturadelmonte@gmail.com>
        Date: 2016-10-04T15:03:55Z

        Merge remote-tracking branch 'upstream/master'

        commit 9f55a1e3e56b48d9dc5a4d1b3109b41e1c89ce5d
        Author: Ventura Del Monte <venturadelmonte@gmail.com>
        Date: 2016-11-02T09:14:52Z

        Merge remote-tracking branch 'upstream/master'

        commit 9a71d092ad06c3592355ad11dfb7bd4b982ded9f
        Author: Ventura Del Monte <venturadelmonte@gmail.com>
        Date: 2016-11-03T20:44:56Z

        FLINK-4997 [streaming] Extending window function metadata introducing ProcessWindowFunction

        commit 878983074d364ea0d340bd3497343f286d28e3db
        Author: Ventura Del Monte <venturadelmonte@gmail.com>
        Date: 2016-11-04T14:29:52Z

        FLINK-4997 [docs] improved windows documentation explaining the new ProcessWindowFunction API


        Show
        githubbot ASF GitHub Bot added a comment - GitHub user VenturaDelMonte opened a pull request: https://github.com/apache/flink/pull/2756 FLINK-4997 Extending Window Function Metadata This PR aims to introduce what discussed in [FLIP-2] ( https://cwiki.apache.org/confluence/display/FLINK/FLIP-2+Extending+Window+Function+Metadata ). WindowedStream apply methods have been overloaded in order to support ProcessWindowFunction (and its rich counterpart). Streaming runtime internals have been modified in order to support the new function, however fully backward compatibility to WindowFunction (and its rich counterpart) is guaranteed by silently wrapping it with a ProcessWindowFunction. This PR implementation strictly follow what decided in the FLIP, nothing has been changed for AllWindowedStream. Windows documentation has been overhauled in order to illustrate ProcessWindowFunction API. You can merge this pull request into a Git repository by running: $ git pull https://github.com/VenturaDelMonte/flink flip-2 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/2756.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 #2756 commit 5a33fe2b9e5c23e8529964a489465f51410432df Author: Ventura Del Monte <venturadelmonte@gmail.com> Date: 2016-09-21T14:04:00Z Merge remote-tracking branch 'apache/master' commit 39a8bb9433edf5bb9adb850c667b71ef8d25c6b6 Author: Ventura Del Monte <venturadelmonte@gmail.com> Date: 2016-09-22T08:31:58Z Merge remote-tracking branch 'upstream/master' commit 2507a71e2a40b05b3e5c7507a1c32d6678e07810 Author: Ventura Del Monte <venturadelmonte@gmail.com> Date: 2016-09-22T09:40:40Z Merge remote-tracking branch 'upstream/master' commit 57d0bca5681e5ea0ba63f3b95fe4f949af3734de Author: Ventura Del Monte <venturadelmonte@gmail.com> Date: 2016-10-04T15:03:55Z Merge remote-tracking branch 'upstream/master' commit 9f55a1e3e56b48d9dc5a4d1b3109b41e1c89ce5d Author: Ventura Del Monte <venturadelmonte@gmail.com> Date: 2016-11-02T09:14:52Z Merge remote-tracking branch 'upstream/master' commit 9a71d092ad06c3592355ad11dfb7bd4b982ded9f Author: Ventura Del Monte <venturadelmonte@gmail.com> Date: 2016-11-03T20:44:56Z FLINK-4997 [streaming] Extending window function metadata introducing ProcessWindowFunction commit 878983074d364ea0d340bd3497343f286d28e3db Author: Ventura Del Monte <venturadelmonte@gmail.com> Date: 2016-11-04T14:29:52Z FLINK-4997 [docs] improved windows documentation explaining the new ProcessWindowFunction API
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user manuzhang commented on a diff in the pull request:

        https://github.com/apache/flink/pull/2756#discussion_r86973108

        — Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/runtime/operators/windowing/functions/InternalWindowFunction.java —
        @@ -46,5 +46,5 @@
        *

        • @throws Exception The function may throw exceptions to fail the program and trigger recovery.
          */
        • public abstract void apply(KEY key, W window, IN input, Collector<OUT> out) throws Exception;
          + public abstract void process(KEY key, W window, IN input, Collector<OUT> out) throws Exception;
            • End diff –

        I think this PR is good as a first step but I really want to see the benefits of introducing the new interface, especially for users. I don't see how the window metadata is extended with the current implementation. That's my $0.02. @aljoscha is more of an expert here.

        Show
        githubbot ASF GitHub Bot added a comment - Github user manuzhang commented on a diff in the pull request: https://github.com/apache/flink/pull/2756#discussion_r86973108 — Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/runtime/operators/windowing/functions/InternalWindowFunction.java — @@ -46,5 +46,5 @@ * @throws Exception The function may throw exceptions to fail the program and trigger recovery. */ public abstract void apply(KEY key, W window, IN input, Collector<OUT> out) throws Exception; + public abstract void process(KEY key, W window, IN input, Collector<OUT> out) throws Exception; End diff – I think this PR is good as a first step but I really want to see the benefits of introducing the new interface, especially for users. I don't see how the window metadata is extended with the current implementation. That's my $0.02. @aljoscha is more of an expert here.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user manuzhang commented on the issue:

        https://github.com/apache/flink/pull/2756

        R: @aljoscha could you take a look ?

        Show
        githubbot ASF GitHub Bot added a comment - Github user manuzhang commented on the issue: https://github.com/apache/flink/pull/2756 R: @aljoscha could you take a look ?
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user aljoscha commented on a diff in the pull request:

        https://github.com/apache/flink/pull/2756#discussion_r87801992

        — Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/datastream/WindowedStream.java —
        @@ -800,4 +907,39 @@ public StreamExecutionEnvironment getExecutionEnvironment() {
        public TypeInformation<T> getInputType()

        { return input.getType(); }

        +
        + private static <T, R, K, W extends Window> ProcessWindowFunction<T, R, K, W> wrapWindowFunction(final WindowFunction<T, R, K, W> cleanedFunction) {
        — End diff –

        There is already `WrappingFunction`, that can be used for these kinds of purposes.

        Show
        githubbot ASF GitHub Bot added a comment - Github user aljoscha commented on a diff in the pull request: https://github.com/apache/flink/pull/2756#discussion_r87801992 — Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/datastream/WindowedStream.java — @@ -800,4 +907,39 @@ public StreamExecutionEnvironment getExecutionEnvironment() { public TypeInformation<T> getInputType() { return input.getType(); } + + private static <T, R, K, W extends Window> ProcessWindowFunction<T, R, K, W> wrapWindowFunction(final WindowFunction<T, R, K, W> cleanedFunction) { — End diff – There is already `WrappingFunction`, that can be used for these kinds of purposes.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user aljoscha commented on a diff in the pull request:

        https://github.com/apache/flink/pull/2756#discussion_r87802539

        — Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/datastream/WindowedStream.java —
        @@ -459,7 +545,28 @@ public WindowedStream(KeyedStream<T, K> input,

        • @param resultType Type information for the result type of the window function
        • @return The data stream that is the result of applying the window function to the window.
          */
        • public <R> SingleOutputStreamOperator<R> apply(R initialValue, FoldFunction<T, R> foldFunction, WindowFunction<R, R, K, W> function, TypeInformation<R> resultType) {
          + public <R> SingleOutputStreamOperator<R> apply(R initialValue, FoldFunction<T, R> foldFunction,
            • End diff –

        I think we can already implement https://issues.apache.org/jira/browse/FLINK-3869 (see my last comment there) for the `ProcessWindowFunction`. I.e. don't have `apply(ReduceFunction, WindowFunction)` but `reduce(ReduceFunction, ProcessWindowFunction)` (same for fold).

        Also, `apply(R, FoldFunction, WindowFunction)` has a bug, in that it is too restrictive, see the Jira issue I linked.

        Show
        githubbot ASF GitHub Bot added a comment - Github user aljoscha commented on a diff in the pull request: https://github.com/apache/flink/pull/2756#discussion_r87802539 — Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/datastream/WindowedStream.java — @@ -459,7 +545,28 @@ public WindowedStream(KeyedStream<T, K> input, @param resultType Type information for the result type of the window function @return The data stream that is the result of applying the window function to the window. */ public <R> SingleOutputStreamOperator<R> apply(R initialValue, FoldFunction<T, R> foldFunction, WindowFunction<R, R, K, W> function, TypeInformation<R> resultType) { + public <R> SingleOutputStreamOperator<R> apply(R initialValue, FoldFunction<T, R> foldFunction, End diff – I think we can already implement https://issues.apache.org/jira/browse/FLINK-3869 (see my last comment there) for the `ProcessWindowFunction`. I.e. don't have `apply(ReduceFunction, WindowFunction)` but `reduce(ReduceFunction, ProcessWindowFunction)` (same for fold). Also, `apply(R, FoldFunction, WindowFunction)` has a bug, in that it is too restrictive, see the Jira issue I linked.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user VenturaDelMonte commented on a diff in the pull request:

        https://github.com/apache/flink/pull/2756#discussion_r87974146

        — Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/datastream/WindowedStream.java —
        @@ -800,4 +907,39 @@ public StreamExecutionEnvironment getExecutionEnvironment() {
        public TypeInformation<T> getInputType()

        { return input.getType(); }

        +
        + private static <T, R, K, W extends Window> ProcessWindowFunction<T, R, K, W> wrapWindowFunction(final WindowFunction<T, R, K, W> cleanedFunction) {
        — End diff –

        Yes, I know about it, but having a WrappingFunction<WindowFunction> would not be possible here because we need to create a ProcessWindowFunction. Anyway as I would get rid of the triple nesting InternalWindowFunction(ProcessWindowFunction(WindowFunction))), this is no longer an issue. By the way, I added this triple nesting because I thought you wanted to completely decouple the WindowFunction interface from runtime internals.

        Show
        githubbot ASF GitHub Bot added a comment - Github user VenturaDelMonte commented on a diff in the pull request: https://github.com/apache/flink/pull/2756#discussion_r87974146 — Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/datastream/WindowedStream.java — @@ -800,4 +907,39 @@ public StreamExecutionEnvironment getExecutionEnvironment() { public TypeInformation<T> getInputType() { return input.getType(); } + + private static <T, R, K, W extends Window> ProcessWindowFunction<T, R, K, W> wrapWindowFunction(final WindowFunction<T, R, K, W> cleanedFunction) { — End diff – Yes, I know about it, but having a WrappingFunction<WindowFunction> would not be possible here because we need to create a ProcessWindowFunction. Anyway as I would get rid of the triple nesting InternalWindowFunction(ProcessWindowFunction(WindowFunction))), this is no longer an issue. By the way, I added this triple nesting because I thought you wanted to completely decouple the WindowFunction interface from runtime internals.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user VenturaDelMonte commented on a diff in the pull request:

        https://github.com/apache/flink/pull/2756#discussion_r87975777

        — Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/datastream/WindowedStream.java —
        @@ -459,7 +545,28 @@ public WindowedStream(KeyedStream<T, K> input,

        • @param resultType Type information for the result type of the window function
        • @return The data stream that is the result of applying the window function to the window.
          */
        • public <R> SingleOutputStreamOperator<R> apply(R initialValue, FoldFunction<T, R> foldFunction, WindowFunction<R, R, K, W> function, TypeInformation<R> resultType) {
          + public <R> SingleOutputStreamOperator<R> apply(R initialValue, FoldFunction<T, R> foldFunction,
            • End diff –

        Thx for the review, first of all. I see your point here. I will introduce these ProcessWindowFunction-specific fold/reduce methods, w/o altering the old WindowFunction API of course.

        Show
        githubbot ASF GitHub Bot added a comment - Github user VenturaDelMonte commented on a diff in the pull request: https://github.com/apache/flink/pull/2756#discussion_r87975777 — Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/datastream/WindowedStream.java — @@ -459,7 +545,28 @@ public WindowedStream(KeyedStream<T, K> input, @param resultType Type information for the result type of the window function @return The data stream that is the result of applying the window function to the window. */ public <R> SingleOutputStreamOperator<R> apply(R initialValue, FoldFunction<T, R> foldFunction, WindowFunction<R, R, K, W> function, TypeInformation<R> resultType) { + public <R> SingleOutputStreamOperator<R> apply(R initialValue, FoldFunction<T, R> foldFunction, End diff – Thx for the review, first of all. I see your point here. I will introduce these ProcessWindowFunction-specific fold/reduce methods, w/o altering the old WindowFunction API of course.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user aljoscha commented on a diff in the pull request:

        https://github.com/apache/flink/pull/2756#discussion_r88001538

        — Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/datastream/WindowedStream.java —
        @@ -800,4 +907,39 @@ public StreamExecutionEnvironment getExecutionEnvironment() {
        public TypeInformation<T> getInputType()

        { return input.getType(); }

        +
        + private static <T, R, K, W extends Window> ProcessWindowFunction<T, R, K, W> wrapWindowFunction(final WindowFunction<T, R, K, W> cleanedFunction) {
        — End diff –

        I see, I'm afraid I didn't communicate that very well.

        For `WindowOperator` the user facing `WindowFunction` is already abstracted from the actual `WindowOperator` by the `InternalWindowFunction`. I just realised that these "AlignedProcessingTimeWindowOperators" don't have that abstraction and directly take a `WindowFunction`, there we would have to introduce some level of nesting, either by wrapping a `WindowFunction` in a `ProcessWindowFunction` or by also using `InternalWindowFunction` there. Thing is, though, that these operators will be removed because their state cannot easily made repartitionable (see the work that we did on key-groups and repartitionable state).

        Show
        githubbot ASF GitHub Bot added a comment - Github user aljoscha commented on a diff in the pull request: https://github.com/apache/flink/pull/2756#discussion_r88001538 — Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/datastream/WindowedStream.java — @@ -800,4 +907,39 @@ public StreamExecutionEnvironment getExecutionEnvironment() { public TypeInformation<T> getInputType() { return input.getType(); } + + private static <T, R, K, W extends Window> ProcessWindowFunction<T, R, K, W> wrapWindowFunction(final WindowFunction<T, R, K, W> cleanedFunction) { — End diff – I see, I'm afraid I didn't communicate that very well. For `WindowOperator` the user facing `WindowFunction` is already abstracted from the actual `WindowOperator` by the `InternalWindowFunction`. I just realised that these "AlignedProcessingTimeWindowOperators" don't have that abstraction and directly take a `WindowFunction`, there we would have to introduce some level of nesting, either by wrapping a `WindowFunction` in a `ProcessWindowFunction` or by also using `InternalWindowFunction` there. Thing is, though, that these operators will be removed because their state cannot easily made repartitionable (see the work that we did on key-groups and repartitionable state).
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user VenturaDelMonte commented on a diff in the pull request:

        https://github.com/apache/flink/pull/2756#discussion_r88021488

        — Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/datastream/WindowedStream.java —
        @@ -800,4 +907,39 @@ public StreamExecutionEnvironment getExecutionEnvironment() {
        public TypeInformation<T> getInputType()

        { return input.getType(); }

        +
        + private static <T, R, K, W extends Window> ProcessWindowFunction<T, R, K, W> wrapWindowFunction(final WindowFunction<T, R, K, W> cleanedFunction) {
        — End diff –

        Aljoscha you read my mind, I was just introducing InternalWindowFunction in these other WindowOperators in order to have a flatter implementation flatter. Regarding their removal, I checked that new work and I agree with you.

        Show
        githubbot ASF GitHub Bot added a comment - Github user VenturaDelMonte commented on a diff in the pull request: https://github.com/apache/flink/pull/2756#discussion_r88021488 — Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/datastream/WindowedStream.java — @@ -800,4 +907,39 @@ public StreamExecutionEnvironment getExecutionEnvironment() { public TypeInformation<T> getInputType() { return input.getType(); } + + private static <T, R, K, W extends Window> ProcessWindowFunction<T, R, K, W> wrapWindowFunction(final WindowFunction<T, R, K, W> cleanedFunction) { — End diff – Aljoscha you read my mind, I was just introducing InternalWindowFunction in these other WindowOperators in order to have a flatter implementation flatter. Regarding their removal, I checked that new work and I agree with you.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user aljoscha commented on a diff in the pull request:

        https://github.com/apache/flink/pull/2756#discussion_r88021648

        — Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/datastream/WindowedStream.java —
        @@ -800,4 +907,39 @@ public StreamExecutionEnvironment getExecutionEnvironment() {
        public TypeInformation<T> getInputType()

        { return input.getType(); }

        +
        + private static <T, R, K, W extends Window> ProcessWindowFunction<T, R, K, W> wrapWindowFunction(final WindowFunction<T, R, K, W> cleanedFunction) {
        — End diff –

        😃

        Show
        githubbot ASF GitHub Bot added a comment - Github user aljoscha commented on a diff in the pull request: https://github.com/apache/flink/pull/2756#discussion_r88021648 — Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/datastream/WindowedStream.java — @@ -800,4 +907,39 @@ public StreamExecutionEnvironment getExecutionEnvironment() { public TypeInformation<T> getInputType() { return input.getType(); } + + private static <T, R, K, W extends Window> ProcessWindowFunction<T, R, K, W> wrapWindowFunction(final WindowFunction<T, R, K, W> cleanedFunction) { — End diff – 😃
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user aljoscha commented on the issue:

        https://github.com/apache/flink/pull/2756

        The changes look very good! 👍

        I'll rebase on top of the recent changes that introduced new `fold()`/`reduce()` and will also reshuffle/squash the commits a bit to make the history clearer. I hope that's alright.

        I'll also change the name of the `process()` method in `InternalWindowFunction` back to `apply()` so that we have less changes and the core of the changes is more obvious. You could change that name in a future PR if you'd like but it's purely internal. I hope this is also alright.

        A word on commit titles: we normally use imperative mood, i.e. "Extend WindowFunction Metadata" instead of "Extending ..." or "Extends ...". Initially, this might seem strange but you can think of a commit as being the command to do something. This is a good read on commit messages: http://chris.beams.io/posts/git-commit/ I'm not writing so much to discourage you, I just like good commit messages. And I hope you'll keep up the good work in the code. 😃

        Btw, do you also want to do the same changes for `AllWindowedStream`?

        Show
        githubbot ASF GitHub Bot added a comment - Github user aljoscha commented on the issue: https://github.com/apache/flink/pull/2756 The changes look very good! 👍 I'll rebase on top of the recent changes that introduced new `fold()`/`reduce()` and will also reshuffle/squash the commits a bit to make the history clearer. I hope that's alright. I'll also change the name of the `process()` method in `InternalWindowFunction` back to `apply()` so that we have less changes and the core of the changes is more obvious. You could change that name in a future PR if you'd like but it's purely internal. I hope this is also alright. A word on commit titles: we normally use imperative mood, i.e. "Extend WindowFunction Metadata" instead of "Extending ..." or "Extends ...". Initially, this might seem strange but you can think of a commit as being the command to do something. This is a good read on commit messages: http://chris.beams.io/posts/git-commit/ I'm not writing so much to discourage you, I just like good commit messages. And I hope you'll keep up the good work in the code. 😃 Btw, do you also want to do the same changes for `AllWindowedStream`?
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user VenturaDelMonte commented on the issue:

        https://github.com/apache/flink/pull/2756

        Hi,
        Thank you for both your review and your suggestions! It is good to learn something new everyday!
        Regarding InternalWindowFunction, that's alright, although we may need to change it in the future in order to support more metadata.
        About AllWindowedStream, of course I would like to, should I open a new issue and make a new PR?

        Show
        githubbot ASF GitHub Bot added a comment - Github user VenturaDelMonte commented on the issue: https://github.com/apache/flink/pull/2756 Hi, Thank you for both your review and your suggestions! It is good to learn something new everyday! Regarding InternalWindowFunction, that's alright, although we may need to change it in the future in order to support more metadata. About AllWindowedStream, of course I would like to, should I open a new issue and make a new PR?
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user manuzhang commented on the issue:

        https://github.com/apache/flink/pull/2756

        @VenturaDelMonte sorry, it was my suggestion to change `InternalWindowFunction#apply` to `process`. Anyway, great work 👍

        Show
        githubbot ASF GitHub Bot added a comment - Github user manuzhang commented on the issue: https://github.com/apache/flink/pull/2756 @VenturaDelMonte sorry, it was my suggestion to change `InternalWindowFunction#apply` to `process`. Anyway, great work 👍
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user aljoscha commented on the issue:

        https://github.com/apache/flink/pull/2756

        @VenturaDelMonte Yes to `AllWindowedStream` (Issue, PR).

        And yes, we need to extend the interface of `InternalWindowFunction` in the future and we can also change the name to process in a separate commit. I just wanted to reduce the amount of unrelated changes.

        Show
        githubbot ASF GitHub Bot added a comment - Github user aljoscha commented on the issue: https://github.com/apache/flink/pull/2756 @VenturaDelMonte Yes to `AllWindowedStream` (Issue, PR). And yes, we need to extend the interface of `InternalWindowFunction` in the future and we can also change the name to process in a separate commit. I just wanted to reduce the amount of unrelated changes.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user VenturaDelMonte commented on the issue:

        https://github.com/apache/flink/pull/2756

        @manuzhang thank you for your feedback and no problem!

        @aljoscha 👍

        Show
        githubbot ASF GitHub Bot added a comment - Github user VenturaDelMonte commented on the issue: https://github.com/apache/flink/pull/2756 @manuzhang thank you for your feedback and no problem! @aljoscha 👍
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user manuzhang commented on the issue:

        https://github.com/apache/flink/pull/2756

        @VenturaDelMonte Any updates here ?

        Show
        githubbot ASF GitHub Bot added a comment - Github user manuzhang commented on the issue: https://github.com/apache/flink/pull/2756 @VenturaDelMonte Any updates here ?
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user aljoscha commented on the issue:

        https://github.com/apache/flink/pull/2756

        @manuzhang I finished reviewing and the code is good to go but I'm currently adding more tests before merging this because I realised that we are lacking there a bit.

        See https://issues.apache.org/jira/browse/FLINK-5237

        Show
        githubbot ASF GitHub Bot added a comment - Github user aljoscha commented on the issue: https://github.com/apache/flink/pull/2756 @manuzhang I finished reviewing and the code is good to go but I'm currently adding more tests before merging this because I realised that we are lacking there a bit. See https://issues.apache.org/jira/browse/FLINK-5237
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user VenturaDelMonte commented on the issue:

        https://github.com/apache/flink/pull/2756

        @manuzhang the other PR is ready since few days, you can find it here: #2946 .

        @aljoscha do you plan to add more tests also for AllWindowedStream?

        Show
        githubbot ASF GitHub Bot added a comment - Github user VenturaDelMonte commented on the issue: https://github.com/apache/flink/pull/2756 @manuzhang the other PR is ready since few days, you can find it here: #2946 . @aljoscha do you plan to add more tests also for AllWindowedStream?
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user aljoscha commented on the issue:

        https://github.com/apache/flink/pull/2756

        @VenturaDelMonte, that could require yet more tests, yes. We'll see once I'm done with those for the regular windows.

        Show
        githubbot ASF GitHub Bot added a comment - Github user aljoscha commented on the issue: https://github.com/apache/flink/pull/2756 @VenturaDelMonte, that could require yet more tests, yes. We'll see once I'm done with those for the regular windows.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user VenturaDelMonte commented on the issue:

        https://github.com/apache/flink/pull/2756

        @aljoscha ok, then let me know, please, if I can help somehow.

        Show
        githubbot ASF GitHub Bot added a comment - Github user VenturaDelMonte commented on the issue: https://github.com/apache/flink/pull/2756 @aljoscha ok, then let me know, please, if I can help somehow.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user manuzhang commented on the issue:

        https://github.com/apache/flink/pull/2756

        @aljoscha @VenturaDelMonte This seems to be taking a long time. What is the progress now ?

        Show
        githubbot ASF GitHub Bot added a comment - Github user manuzhang commented on the issue: https://github.com/apache/flink/pull/2756 @aljoscha @VenturaDelMonte This seems to be taking a long time. What is the progress now ?
        Hide
        githubbot ASF GitHub Bot added a comment -

        GitHub user aljoscha opened a pull request:

        https://github.com/apache/flink/pull/3285

        FLINK-4997 [streaming] Introduce ProcessWindowFunction

        This is an updated/enhanced version of #2756.

        I did roughly these changes:

        • Add support for the `AggregatingFunction`/`ProcessWindowFunction` combination
        • Mark the new methods/interfaces as `@PublicEvolving`
        • Change how the Scala wrapper functions work because we recently moved all of them to use `WrappingFunction`. This is not complete yet and I created issues for fixing that: https://issues.apache.org/jira/browse/FLINK-5740 and https://issues.apache.org/jira/browse/FLINK-5741
        • Add tests in `WindowTranslationTest.java` and `WindowTranslationTest.scala`.

        @VenturaDelMonte and @manuzhang, would you like to have a look at this. I took quite a while because there was some upheaval in the internals of that part of the code and the tests. I'm sorry for that.

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

        $ git pull https://github.com/aljoscha/flink process-window-function

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

        https://github.com/apache/flink/pull/3285.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 #3285


        commit 6f476f92f22b661f26d322615af6a52eca01c69c
        Author: Aljoscha Krettek <aljoscha.krettek@gmail.com>
        Date: 2017-02-07T09:54:54Z

        [hotfix] Fix trailing whitespace in WindowedStream.java

        commit ab74f4142ddc4aab6e76563b32464d526f719ae7
        Author: Ventura Del Monte <venturadelmonte@gmail.com>
        Date: 2016-11-23T17:00:23Z

        FLINK-4997 [streaming] Introduce ProcessWindowFunction

        commit b6759f446bad5170fabc803103c764f204412301
        Author: Ventura Del Monte <venturadelmonte@gmail.com>
        Date: 2016-11-09T09:49:47Z

        FLINK-4997 [streaming] Add ProcessWindowFunction to Scala API

        commit a9791e0e7526bfd419e72ac749ce068f18df857a
        Author: Aljoscha Krettek <aljoscha.krettek@gmail.com>
        Date: 2017-02-07T13:38:25Z

        FLINK-4997 Add ProcessWindowFunction support for .aggregate()

        commit 01048feccd5aced42b1979a3bf7b355443c08f5b
        Author: Aljoscha Krettek <aljoscha.krettek@gmail.com>
        Date: 2016-11-24T07:14:48Z

        FLINK-5237 Consolidate and harmonize Window Translation Tests


        Show
        githubbot ASF GitHub Bot added a comment - GitHub user aljoscha opened a pull request: https://github.com/apache/flink/pull/3285 FLINK-4997 [streaming] Introduce ProcessWindowFunction This is an updated/enhanced version of #2756. I did roughly these changes: Add support for the `AggregatingFunction`/`ProcessWindowFunction` combination Mark the new methods/interfaces as `@PublicEvolving` Change how the Scala wrapper functions work because we recently moved all of them to use `WrappingFunction`. This is not complete yet and I created issues for fixing that: https://issues.apache.org/jira/browse/FLINK-5740 and https://issues.apache.org/jira/browse/FLINK-5741 Add tests in `WindowTranslationTest.java` and `WindowTranslationTest.scala`. @VenturaDelMonte and @manuzhang, would you like to have a look at this. I took quite a while because there was some upheaval in the internals of that part of the code and the tests. I'm sorry for that. You can merge this pull request into a Git repository by running: $ git pull https://github.com/aljoscha/flink process-window-function Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/3285.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 #3285 commit 6f476f92f22b661f26d322615af6a52eca01c69c Author: Aljoscha Krettek <aljoscha.krettek@gmail.com> Date: 2017-02-07T09:54:54Z [hotfix] Fix trailing whitespace in WindowedStream.java commit ab74f4142ddc4aab6e76563b32464d526f719ae7 Author: Ventura Del Monte <venturadelmonte@gmail.com> Date: 2016-11-23T17:00:23Z FLINK-4997 [streaming] Introduce ProcessWindowFunction commit b6759f446bad5170fabc803103c764f204412301 Author: Ventura Del Monte <venturadelmonte@gmail.com> Date: 2016-11-09T09:49:47Z FLINK-4997 [streaming] Add ProcessWindowFunction to Scala API commit a9791e0e7526bfd419e72ac749ce068f18df857a Author: Aljoscha Krettek <aljoscha.krettek@gmail.com> Date: 2017-02-07T13:38:25Z FLINK-4997 Add ProcessWindowFunction support for .aggregate() commit 01048feccd5aced42b1979a3bf7b355443c08f5b Author: Aljoscha Krettek <aljoscha.krettek@gmail.com> Date: 2016-11-24T07:14:48Z FLINK-5237 Consolidate and harmonize Window Translation Tests
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user VenturaDelMonte commented on the issue:

        https://github.com/apache/flink/pull/3285

        I checked the code and it looks fine according to me. I see you are dealing with RichFunction inheritance in wrapper classes. It is something that bothered me too when I was working on the previous PR. Actually there could be a similar minor issue in RichProcessWindowFunction due to duplicate code, however I cannot see any easy workaround for it.
        Regarding the overall feature, are you planning to do anything similar for #2946 or should I reflect these changes also there?

        Show
        githubbot ASF GitHub Bot added a comment - Github user VenturaDelMonte commented on the issue: https://github.com/apache/flink/pull/3285 I checked the code and it looks fine according to me. I see you are dealing with RichFunction inheritance in wrapper classes. It is something that bothered me too when I was working on the previous PR. Actually there could be a similar minor issue in RichProcessWindowFunction due to duplicate code, however I cannot see any easy workaround for it. Regarding the overall feature, are you planning to do anything similar for #2946 or should I reflect these changes also there?
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user manuzhang commented on the issue:

        https://github.com/apache/flink/pull/3285

        LGTM. Really excited to see we move further in this direction, and faster if possible.

        Show
        githubbot ASF GitHub Bot added a comment - Github user manuzhang commented on the issue: https://github.com/apache/flink/pull/3285 LGTM. Really excited to see we move further in this direction, and faster if possible.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user aljoscha commented on the issue:

        https://github.com/apache/flink/pull/3285

        @VenturaDelMonte I opened this: https://issues.apache.org/jira/browse/FLINK-5740. I want to make `WrappingFunction` an interface because, as you noted, `RichProcessWindowFunction` does currently not work.

        I would merge this then. If you want you can pick up the work on the all-window case again and carry it to an end in the same way. What do you think?

        Show
        githubbot ASF GitHub Bot added a comment - Github user aljoscha commented on the issue: https://github.com/apache/flink/pull/3285 @VenturaDelMonte I opened this: https://issues.apache.org/jira/browse/FLINK-5740 . I want to make `WrappingFunction` an interface because, as you noted, `RichProcessWindowFunction` does currently not work. I would merge this then. If you want you can pick up the work on the all-window case again and carry it to an end in the same way. What do you think?
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user VenturaDelMonte commented on the issue:

        https://github.com/apache/flink/pull/3285

        Sure, I can take care of the all-window case!

        Show
        githubbot ASF GitHub Bot added a comment - Github user VenturaDelMonte commented on the issue: https://github.com/apache/flink/pull/3285 Sure, I can take care of the all-window case!
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user aljoscha closed the pull request at:

        https://github.com/apache/flink/pull/3285

        Show
        githubbot ASF GitHub Bot added a comment - Github user aljoscha closed the pull request at: https://github.com/apache/flink/pull/3285
        Hide
        aljoscha Aljoscha Krettek added a comment -

        Implemented in:
        1dcb2dcd8969941988a4fc7e5488e9272dfd507e
        86dff0e6d584027994dd1320845169cc8b1a83d5
        4f047e13518ad2eb493903179e38eb174a37994c
        fe2a3016f98e45d0c94a3fa1ed8c17b89a516859

        Show
        aljoscha Aljoscha Krettek added a comment - Implemented in: 1dcb2dcd8969941988a4fc7e5488e9272dfd507e 86dff0e6d584027994dd1320845169cc8b1a83d5 4f047e13518ad2eb493903179e38eb174a37994c fe2a3016f98e45d0c94a3fa1ed8c17b89a516859
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user aljoscha commented on the issue:

        https://github.com/apache/flink/pull/2756

        I finally merged it! Thanks for staying with this for so long and working on this, @VenturaDelMonte 😃

        Hopefully, we can now make faster progress.

        Could you please close this PR?

        Show
        githubbot ASF GitHub Bot added a comment - Github user aljoscha commented on the issue: https://github.com/apache/flink/pull/2756 I finally merged it! Thanks for staying with this for so long and working on this, @VenturaDelMonte 😃 Hopefully, we can now make faster progress. Could you please close this PR?
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user VenturaDelMonte closed the pull request at:

        https://github.com/apache/flink/pull/2756

        Show
        githubbot ASF GitHub Bot added a comment - Github user VenturaDelMonte closed the pull request at: https://github.com/apache/flink/pull/2756

          People

          • Assignee:
            ventura Ventura Del Monte
            Reporter:
            ventura Ventura Del Monte
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development