Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move window analysis to the window method #7672

Merged
merged 4 commits into from
Sep 28, 2023
Merged

Move window analysis to the window method #7672

merged 4 commits into from
Sep 28, 2023

Conversation

mustafasrepo
Copy link
Contributor

@mustafasrepo mustafasrepo commented Sep 27, 2023

Which issue does this PR close?

Closes #.

Rationale for this change

This PR is mostly a restructure PR. All of the changes comes from code movement or comment updates.

Currently window construction analysis for the given input is done using a helper function inside enforce_sorting. However, doing this in enforce_sorting clutters code a bit there. Also this functionality may be useful in other places also.

This PR moves this analysis to the methods of the WindowAggExec and BoundedWindowAggExec.

What changes are included in this PR?

Are these changes tested?

Existing tests should work

Are there any user-facing changes?

@github-actions github-actions bot added the core Core DataFusion crate label Sep 27, 2023
Copy link
Contributor

@metesynnada metesynnada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I check the code, LGTM!.

Copy link
Contributor

@ozankabak ozankabak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took two passes on this change and it looks good to me as well.

@alamb
Copy link
Contributor

alamb commented Sep 28, 2023

I skimmed the PR and it also looks good to me -- thank you @ozankabak and @mustafasrepo and @metesynnada

@alamb alamb merged commit 26a3602 into apache:main Sep 28, 2023
Ted-Jiang pushed a commit to Ted-Jiang/arrow-datafusion that referenced this pull request Oct 7, 2023
* Move window analysis to the method

* Preserve physical partition keys, during recreation of the window

* Final review

* Avoid cloning if not necessary

---------

Co-authored-by: Mehmet Ozan Kabak <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core DataFusion crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants