flink-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From fhueske <...@git.apache.org>
Subject [GitHub] flink issue #3459: [FLINK-5654] Add processing time OVER RANGE BETWEEN x PRE...
Date Wed, 08 Mar 2017 22:46:07 GMT
Github user fhueske commented on the issue:

    https://github.com/apache/flink/pull/3459
  
    Hi @huawei-flink, sorry for the long delay. I focused on adding support for retraction
to the new UDAGG interface (PR #3470), support for time mode functions (PR #3370, #3425),
and the most simple OVER window case (PR #3397) first. These additions should serve as a good
basis for the more advanced OVER window cases. 
    
    I had a look this PR and noticed a few things:
    - The Table API is implemented in Scala. We have a few Java classes (code copied &
fixed from Calcite, Java examples, and tests) but do not intend to add more. The core parts
like operators, translation, and runtime code have to be implemented in Scala. Mixing languages
makes the maintenance more difficult.
    - Please squash your commits. Usually, PRs are opened with a single commit and new commits
are added when feedback is addressed. Before merging a committer squashes these commits. However,
it is too much effort to squash more than 50 commits including merge commits which can cause
trouble. If multiple contributors worked on a PR, figure out how you can separate the work
into one commit per author.
    - We have recently reworked our aggregation interface which will also serve as the interface
for user-defined aggregations which should also be usable in OVER windows. Please use these
aggregation functions. When designing the interface we had their use in OVER windows in mind.
If you find that the interface is lacking a method, please start a discussion. However, we
cannot have several incompatible aggregation interfaces in the Table API / SQL. Please rebase
to the current master and use the new aggregation functions.
    - A couple of days ago, we added `PROCTIME()` and `ROWTIME()` methods which should be
used to identify the time mode.
    - The first OVER window aggregation should serve as a blueprint for future OVER window
implementations. We should try to keep the implementations as close as possible to share common
code and make the overall maintenance of the code easier.
    
    Minor comments:
    - Why did you check modifications to the `.gitignore` files?
    - Please do not remove tests without replacing them with equivalent tests.
    - Please do not reformat classes (see FunctionCatalog). These changes cause additional
effort when reviewing a PR.
    - Do not change ScalaDoc comments to JavaDoc comments in Scala code. 
    ScalaDoc:
    ```
    /**
      * Comment
      */
    ```
    JavaDoc:
    ```
    /**
     * Comment
     */
    ```
    
    Thanks, Fabian



---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

Mime
View raw message