[GitHub] [arrow-datafusion] Jimexist edited a comment on pull request #334: Add window expression part 1 - logical and physical planning, structure, to/from proto, and explain, for empty over clause only

[GitBox <gi...@apache.org>]

Jimexist edited a comment on pull request #334:
URL: https://github.com/apache/arrow-datafusion/pull/334#issuecomment-845184684


   > I haven't had the time to go through yet;
   > 
   > Since this is a large decomposable change, a suggestion here is to create a branch and merge this on that branch, so that we do not have incomplete features in master and allow PRs to that branch without an issue. Then merge the branch into master once its first iteration is ready (e.g. logical and 1 physical plan).
   > 
   > (This is something that I wished to have when implementing large features; it gives the time to review in parts without the risk of having incomplete code in master)
   > 
   > With that said, I am also fine risk it and merg to master; @Jimexist do you have any preference how would you prefer to work on this here? :)
   
   I wish to:
   1. add unit tests
   2. fix some small comments like
      1.  adding filter clause to leave room for future room without API disruption, 
      2. adding nth_value and tile
   3. merge it into master as it
   
   3 because although I agree with having a feature branch, the maintenance of continuous rebasing is trouble some. if I can keep the merged code isolated and structurally correct then there's little worry about breaking changes in future: I believe this PR:
   1. is _isolated_ because  the _explain_ works, while actually executing the window SQL returns zero rows, due to unimplemented exec plan, which I intend to do next
   2. is structurally correct if reviewers can focus on the phase and timing of the window clause in the query planner. future changes of e.g. adding one or more sort phases requires only modifications within the `.window` `fn` and thus can be non-intrusive


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org