Window frame GROUPS mode support #4155
Conversation
…g search algorithm
github-actions
bot
added
core
|
@alamb, this already went through our internal review process, so I can say LGTM. Looking forward to getting community feedback. |
|
Thank you @ozankabak -- I will put this on my review queue for tomorrow |
There was a problem hiding this comment.
@zembunia this is a very nice PR and a pleasure to read. It is well tested, well commented, and well structured. 🏆 Thank you.
I left a few style comments, but nothing that needs to be completed prior to merging from my perspective.
Here is the relevant description of GROUPs for anyone else reviewing this PR
In GROUPS mode, the offset again must yield a non-null, non-negative integer, and the option means that the frame starts or ends the specified number of peer groups before or after the current row's peer group, where a peer group is a set of rows that are equivalent in the ORDER BY ordering. (There must be an ORDER BY clause in the window definition to use GROUPS mode.)
I'll plan to merge this PR tomorrow unless there are any additional comments
| @@ -1511,12 +1511,6 @@ pub fn create_window_expr_with_name( | |||
| }) | |||
| .collect::<Result<Vec<_>>>()?; | |||
| if let Some(ref window_frame) = window_frame { | |||
| if window_frame.units == WindowFrameUnits::Groups { | |||
| let err = df.collect().await.unwrap_err(); | ||
| assert_contains!( | ||
| err.to_string(), | ||
| "Execution error: GROUPS mode requires an ORDER BY clause".to_owned() |
| @@ -113,10 +114,10 @@ impl WindowExpr for BuiltInWindowExpr { | |||
| .iter() | |||
| .map(|v| v.slice(partition_range.start, length)) | |||
| .collect::<Vec<_>>(); | |||
| let mut window_frame_ctx = WindowFrameContext::new(&window_frame); | |||
There was a problem hiding this comment.
This is a very nice encapsulation of the window frame calculation. Thank you
| #[derive(Debug)] | ||
| pub enum WindowFrameContext<'a> { | ||
| // ROWS-frames are inherently stateless: | ||
| Rows(&'a Arc<WindowFrame>), |
There was a problem hiding this comment.
As a matter of style, I find &Arc<WindowFrame> strange. I would expect either an owned Arc (aka increment the ref count):
| Rows(&'a Arc<WindowFrame>), | |
| Rows(Arc<WindowFrame>), |
Or else just a reference:
| Rows(&'a Arc<WindowFrame>), | |
| Rows(&'a WindowFrame), |
I don't understand the need to have a reference to the Arc
| &mut self, | ||
| window_frame: &Arc<WindowFrame>, | ||
| range_columns: &[ArrayRef], | ||
| _sort_options: &[SortOptions], |
There was a problem hiding this comment.
if the sort options are not used, I wonder why pass it to calculate_range
| // This structure encapsulates all the state information we require as we | ||
| // scan groups of data while processing window frames. | ||
| #[derive(Debug, Default)] | ||
| pub struct WindowFrameStateGroups { |
There was a problem hiding this comment.
This structure is fine -- I just wanted to point out a pattern for writing state machines in Rust is to encapsulate each state in an enum variant, and then you can use the type system to ensure the state transitions are valid
So like
pub enum WindowFrameStateGroupsState {
Start { },
InGroup { ... }
End {}
}Or something similar
| /// This function finds the next group and its start index for a given group and start index. | ||
| /// It utilizes an exponentially growing step size to find the group boundary. | ||
| // TODO: For small group sizes, proceeding one-by-one to find the group change can be more efficient. | ||
| // Statistics about previous group sizes can be used to choose one-by-one vs. exponentially growing, |
There was a problem hiding this comment.
I agree there are likely many ways to improve the performance of this algorithm -- however, I think the first thing to do is getting a working implementation that is well tested and structured (as this PR is) and as you say we can drive additional optimizations from there.
| let arrays: Vec<ArrayRef> = vec![ | ||
| Arc::new(Float64Array::from_slice([5.0, 7.0, 8.0, 9., 10.])), | ||
| Arc::new(Float64Array::from_slice([2.0, 3.0, 3.0, 4.0, 5.0])), | ||
| Arc::new(Float64Array::from_slice([5.0, 7.0, 8.0, 10., 11.0])), | ||
| Arc::new(Float64Array::from_slice([15.0, 13.0, 8.0, 5., 0.0])), | ||
| ]; |
There was a problem hiding this comment.
Stylistically, you can probably avoid a non trivial amount of repetition from these tests by refactoring the array functions into an individual function.
| } | ||
|
|
||
| #[test] | ||
| fn test_window_frame_groups_preceding_huge_delta() { |
There was a problem hiding this comment.
Why is a delta of 10 "huge"? Is it because the delta is larger than the size of the arrays?
|
thanks for putting the work into this. turns out the psql comparison integration test is of great use. |
Yes I agree -- it is wonderful! |
…unnecessary argument
|
@alamb The low hanging fruits are addressed. Two of your non-blocker feedbacks (state machine implementation, ref counting) will be examined and worked on next week. This can be merged, if you approve. |
|
Thanks @zembunia
will do. Also to be clear, those are suggestions / nice to haves. Definitely not required unless you think it would improve the code and you have the time |
|
Benchmark runs are scheduled for baseline = 225d62c and contender = 129654c. 129654c is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
This PR provides the support of the
GROUPSmode in the window frames, which was a missing item in #3570 enhancement.The
GROUPSmode is implemented regarding the specification in PostgreSQL window function calls.Rationale for this change
This change is part of an enhancement #361 that is on the roadmap.
What changes are included in this PR?
The common single method calculating the window range (calculate_range) is removed from the window_expr. New structs that can hold any state information for each window frame mode are introduced.
The
ROWSmode does not require a state as it is simple row index calculation, thought the state struct is empty apart from the simplecalculate_rangemethod specific toROWSmode.For the
RANGEmode, a stateful calculation can be utilized in the future. For now, the state struct is empty and the specificcalculate_rangeimplementation is moved to the state struct.For the
GROUPSmode, a stateful implementation, that keeps track of the moving window range of groups for each consecutive row, is provided.The
frame exclusionis still not supported.Observations
The implementation for the
RANGEmode can also utilize a stateful implementation, instead of calculating the window range for each row from scratch.Future work
RANGEmode implementationfind_next_group_and_start_index). This method can be improved to choose an approach depending on statistics about previous group sizes. It can either search the next group by advancing one-by-one (for small group sizes) or utilizing the exponentially growing step size, or even setting a base step size when exponentially growing. We can also create a benchmark implementation to get insights about the crossover point.Are these changes tested?
New unit tests relevant to the added functionality are added in
window_frame_state.rs. The tests inwindows.rsis extended to cover theGROUPSmode, and a test file is added to the integration test SQLs.Are there any user-facing changes?
No