Github user fhueske commented on the issue:
https://github.com/apache/flink/pull/3675
Thanks, will merge 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 and
Github user haohui commented on the issue:
https://github.com/apache/flink/pull/3675
Yes. I agree with you that `FLOOR` / `CEIL` are quite fragile and it is
much clearer to use the `TUMBLING` / `HOP` / `SESSION` constructs.
---
If your project is set up for it, you can reply to this
Github user fhueske commented on the issue:
https://github.com/apache/flink/pull/3675
Thanks for the review @haohui!
Just to make sure: You're OK with removing support for windows defined as
`GROUP BY CEIL(rowtime() TO HOUR)`?
Thanks, Fabian
---
If your project is
Github user haohui commented on the issue:
https://github.com/apache/flink/pull/3675
The PR looks pretty good. Thanks @fhueske for improving the documentation.
+1 (non-binding)
---
If your project is set up for it, you can reply to this email and have your
reply appear on
Github user fhueske commented on the issue:
https://github.com/apache/flink/pull/3675
Hi @haohui, I updated the PR and refactored the rule into an abstract class
that is extended by a rule for datastream and another one for dataset.
**Please note**: I also removed support for
Github user fhueske commented on the issue:
https://github.com/apache/flink/pull/3675
Thanks for the review @haohui!
You're right about the code duplication. I had started with addressing
DataSet and DataStream in one class but ended up with many `if(stream)`
conditions.
Github user haohui commented on the issue:
https://github.com/apache/flink/pull/3675
Thanks for the PR! It looks good to me overall. One problem is that
`dataSet.LogicalWindowAggregateRule` has duplicated quite a bit of code in the
`dataStream.LogicalWindowAggregateRule`.
