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, per @alamb, like
1. adding filter clause to leave room for future room without API
disruption,
2. adding nth_value and tile and leaving them unimplemented and leave
room for good first PR
3. merge it into master as it
3 because although I agree with having a feature branch, the maintenance of
continuous rebasing would be troublesome, and I can estimate the whole series
of window function implementations take > 1 month. if I can keep the merged
code _isolated_ and _structurally complete_ 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 complete_ 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:
[email protected]
