[GitHub] flink issue #2938: [FLINK-4692] [tableApi] Add tumbling group-windows for ba...

2017-01-16 Thread twalthr
Github user twalthr commented on the issue: https://github.com/apache/flink/pull/2938 Thanks for the update @wuchong. I will test it again and merge it. --- 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

[GitHub] flink issue #2938: [FLINK-4692] [tableApi] Add tumbling group-windows for ba...

2017-01-11 Thread wuchong
Github user wuchong commented on the issue: https://github.com/apache/flink/pull/2938 Hi @twalthr , thanks for your reviewing. I have addressed your comments. --- 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

[GitHub] flink issue #2938: [FLINK-4692] [tableApi] Add tumbling group-windows for ba...

2017-01-11 Thread fhueske
Github user fhueske commented on the issue: https://github.com/apache/flink/pull/2938 It would also be good to have a set of well selected end-to-end IT cases. But Timo is right, the previous approach of starting a mini cluster for each test case was too expensive to maintain. The

[GitHub] flink issue #2938: [FLINK-4692] [tableApi] Add tumbling group-windows for ba...

2017-01-11 Thread twalthr
Github user twalthr commented on the issue: https://github.com/apache/flink/pull/2938 Thanks for fixing the bug. The collection execution mode is on purpose because we have too many IT cases at the moment. We should only test the subclasses of `FlinkRel` like `DataSetUnion`,

[GitHub] flink issue #2938: [FLINK-4692] [tableApi] Add tumbling group-windows for ba...

2017-01-10 Thread wuchong
Github user wuchong commented on the issue: https://github.com/apache/flink/pull/2938 Thanks @twalthr for your hint. It is a bug in my `DataSetTumbleTimeWindowAggReduceCombineFunction#combine(...)` method, that the rowtime attribute is dropped when combining. The collection

[GitHub] flink issue #2938: [FLINK-4692] [tableApi] Add tumbling group-windows for ba...

2017-01-10 Thread twalthr
Github user twalthr commented on the issue: https://github.com/apache/flink/pull/2938 @fhueske and I looked into this again. I seems that result depends on the batch ExecutionEnvironment. I used a regular environment while the test base uses a CollectionExecutionEnvironment. We don't

[GitHub] flink issue #2938: [FLINK-4692] [tableApi] Add tumbling group-windows for ba...

2017-01-10 Thread fhueske
Github user fhueske commented on the issue: https://github.com/apache/flink/pull/2938 Hi, I ran both methods with the provided data and both compute the same result on my machine. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub

[GitHub] flink issue #2938: [FLINK-4692] [tableApi] Add tumbling group-windows for ba...

2017-01-10 Thread twalthr
Github user twalthr commented on the issue: https://github.com/apache/flink/pull/2938 @wuchong I used the data of `org.apache.flink.table.api.scala.stream.table.AggregationsITCase`. ``` val data = List( (1L, 1, "Hi"), (2L, 2, "Hello"), (4L, 2,

[GitHub] flink issue #2938: [FLINK-4692] [tableApi] Add tumbling group-windows for ba...

2017-01-09 Thread wuchong
Github user wuchong commented on the issue: https://github.com/apache/flink/pull/2938 It's wired. I can't reproduce the wrong results in my local environment. What is the `data` in your test @twalthr ? I'm using the following data in the batch and stream test, but the result is

[GitHub] flink issue #2938: [FLINK-4692] [tableApi] Add tumbling group-windows for ba...

2017-01-09 Thread twalthr
Github user twalthr commented on the issue: https://github.com/apache/flink/pull/2938 Here is an example to reproduce the wrong results: ```scala @Test def testEventTimeTumblingWindowStream(): Unit = { val env =

[GitHub] flink issue #2938: [FLINK-4692] [tableApi] Add tumbling group-windows for ba...

2017-01-09 Thread twalthr
Github user twalthr commented on the issue: https://github.com/apache/flink/pull/2938 I will look into it again. Actually I used the same data, but maybe I did a mistake. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well.

[GitHub] flink issue #2938: [FLINK-4692] [tableApi] Add tumbling group-windows for ba...

2017-01-07 Thread wuchong
Github user wuchong commented on the issue: https://github.com/apache/flink/pull/2938 Hi @twalthr , I have tested and the batch and stream queries return the same result in my machine. The `org.apache.flink.api.scala.stream.table.AggregationsITCase#testEventTimeTumblingWindow` and

[GitHub] flink issue #2938: [FLINK-4692] [tableApi] Add tumbling group-windows for ba...

2017-01-06 Thread wuchong
Github user wuchong commented on the issue: https://github.com/apache/flink/pull/2938 Sure, I will look at it today @twalthr . --- 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

[GitHub] flink issue #2938: [FLINK-4692] [tableApi] Add tumbling group-windows for ba...

2017-01-06 Thread twalthr
Github user twalthr commented on the issue: https://github.com/apache/flink/pull/2938 Thanks for working on this @wuchong! The code looks very good. I haven"t reviewed the code entirely but tried to verify if batch and stream queries return the same result. Unfortunately, this is not

[GitHub] flink issue #2938: [FLINK-4692] [tableApi] Add tumbling group-windows for ba...

2017-01-06 Thread twalthr
Github user twalthr commented on the issue: https://github.com/apache/flink/pull/2938 I would like to shepherd this PR. --- 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