-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Issue #8086: Parallel Window Refactoring #8269
Conversation
Basic code motion to refactor WindowExecutor into smarter, purpose-built subclasses.
`WindowCumeDistExecutor` does not need `NextRank`
Convert rank and dense_rank to compute their state at the start of each block This will allow them to be called on multiple threads and out of order.
Break out the window executor code into its own files.
Create top-level local state objects for window execution. This will enable parallel evaluation.
Executor cleanup.
Remove the Compute method as it is vestigial.
Break out windowed aggregation evaluation state from the aggregator. More code cleanup and naming clarifications.
* Update coverage * Fix stateless DENSE_RANK setup (another victory for SVS=2)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Looks great! Unifying everything behind one interface makes everything a lot more clean.
Could we perhaps work on improving the code coverage while we're at it? A lot of these uncovered lines in window_executor.cpp
are likely inherited from the previous implementation, but it might be worthwhile to have a look at covering them. Particularly those in the WindowDenseRankExecutor
Add some coverage tests.
Thanks! LGTM |
This PR is mostly code motion for parallel window function computation, but there is concurrent work being done on the
EXCLUDE
functionality by Romaine that relies on it.The non-trivial changes are:
DENSE_RANK
required care.