refactor(over window): generalize WindowBuffer and window function AggregateState
#14647
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Jan 18, 2024
This was referenced Jan 18, 2024
|
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
9 tasks
stdrc
commented
Jan 18, 2024
Comment on lines
+252
to
+276
| buffer_ref.curr_idx < buffer_ref.buffer.len() && { | ||
| let start_off = self.frame_bounds.start.to_offset(); | ||
| if let Some(start_off) = start_off { | ||
| if start_off >= 0 { | ||
| true // pure following frame, always preceding-saturated | ||
| } else { | ||
| // FIXME(rc): Clippy rule `clippy::nonminimal_bool` is misreporting that | ||
| // the following can be simplified. | ||
| #[allow(clippy::nonminimal_bool)] | ||
| { | ||
| assert!(buffer_ref.curr_idx >= buffer_ref.left_idx); | ||
| } | ||
| buffer_ref.curr_idx - buffer_ref.left_idx >= start_off.unsigned_abs() | ||
| } | ||
| } else { | ||
| false // unbounded frame start, never preceding-saturated | ||
| } | ||
| } |
There was a problem hiding this comment.
Algorithm not changed, only moved
stdrc
commented
Jan 18, 2024
Comment on lines
+273
to
+298
| buffer_ref.curr_idx < buffer_ref.buffer.len() && { | ||
| let end_off = self.frame_bounds.end.to_offset(); | ||
| if let Some(end_off) = end_off { | ||
| if end_off <= 0 { | ||
| true // pure preceding frame, always following-saturated | ||
| } else { | ||
| // FIXME(rc): Ditto. | ||
| #[allow(clippy::nonminimal_bool)] | ||
| { | ||
| assert!(buffer_ref.right_excl_idx > 0); | ||
| assert!(buffer_ref.right_excl_idx > buffer_ref.curr_idx); | ||
| assert!(buffer_ref.right_excl_idx <= buffer_ref.buffer.len()); | ||
| } | ||
| buffer_ref.right_excl_idx - 1 - buffer_ref.curr_idx >= end_off as usize | ||
| } | ||
| } else { | ||
| false // unbounded frame end, never following-saturated | ||
| } | ||
| } |
There was a problem hiding this comment.
Algorithm not changed, only moved
stdrc
commented
Jan 18, 2024
Comment on lines
+295
to
+332
| if buffer_ref.buffer.is_empty() { | ||
| *buffer_ref.left_idx = 0; | ||
| *buffer_ref.right_excl_idx = 0; | ||
| } | ||
|
|
||
| let start_off = self.frame_bounds.start.to_offset(); | ||
| let end_off = self.frame_bounds.end.to_offset(); | ||
| if let Some(start_off) = start_off { | ||
| let logical_left_idx = *buffer_ref.curr_idx as isize + start_off; | ||
| if logical_left_idx >= 0 { | ||
| *buffer_ref.left_idx = | ||
| std::cmp::min(logical_left_idx as usize, buffer_ref.buffer.len()); | ||
| } else { | ||
| *buffer_ref.left_idx = 0; | ||
| } | ||
| } else { | ||
| // unbounded start | ||
| *buffer_ref.left_idx = 0; | ||
| } | ||
| if let Some(end_off) = end_off { | ||
| let logical_right_excl_idx = *buffer_ref.curr_idx as isize + end_off + 1; | ||
| if logical_right_excl_idx >= 0 { | ||
| *buffer_ref.right_excl_idx = | ||
| std::cmp::min(logical_right_excl_idx as usize, buffer_ref.buffer.len()); | ||
| } else { | ||
| *buffer_ref.right_excl_idx = 0; | ||
| } | ||
| } else { | ||
| // unbounded end | ||
| *buffer_ref.right_excl_idx = buffer_ref.buffer.len(); | ||
| } |
There was a problem hiding this comment.
Algorithm not changed, only moved
stdrc
force-pushed
the
rc/refactor-find-affected-ranges
branch
from
3181b68 to
e2e4eb1
Compare
stdrc
force-pushed
the
rc/generalize-window-buffer
branch
from
27c8004 to
980d29d
Compare
stdrc
force-pushed
the
rc/refactor-find-affected-ranges
branch
from
e2e4eb1 to
5bd2c0e
Compare
stdrc
force-pushed
the
rc/generalize-window-buffer
branch
from
980d29d to
dfe1a5b
Compare
stdrc
force-pushed
the
rc/refactor-find-affected-ranges
branch
from
5bd2c0e to
56ad72f
Compare
stdrc
force-pushed
the
rc/generalize-window-buffer
branch
from
c25c28f to
8f5ed91
Compare
stdrc
force-pushed
the
rc/refactor-find-affected-ranges
branch
from
56ad72f to
4b99ab1
Compare
stdrc
force-pushed
the
rc/generalize-window-buffer
branch
from
8f5ed91 to
d3eba36
Compare
stdrc
force-pushed
the
rc/refactor-find-affected-ranges
branch
from
4b99ab1 to
6a55390
Compare
stdrc
force-pushed
the
rc/generalize-window-buffer
branch
from
d3eba36 to
9933c72
Compare
stdrc
force-pushed
the
rc/refactor-find-affected-ranges
branch
from
6a55390 to
d7cf817
Compare
stdrc
force-pushed
the
rc/generalize-window-buffer
branch
2 times, most recently
from
d23eeca to
bd2ed75
Compare
stdrc
force-pushed
the
rc/refactor-find-affected-ranges
branch
from
15fd42d to
9cd74ef
Compare
stdrc
force-pushed
the
rc/generalize-window-buffer
branch
from
bd2ed75 to
16f5d06
Compare
stdrc
force-pushed
the
rc/refactor-find-affected-ranges
branch
from
9cd74ef to
58054c8
Compare
stdrc
force-pushed
the
rc/generalize-window-buffer
branch
from
16f5d06 to
58d9f5d
Compare
stdrc
force-pushed
the
rc/refactor-find-affected-ranges
branch
from
82b2728 to
b5f5a39
Compare
stdrc
force-pushed
the
rc/generalize-window-buffer
branch
from
58d9f5d to
8d89879
Compare
stdrc
force-pushed
the
rc/refactor-find-affected-ranges
branch
from
b5f5a39 to
9ae199f
Compare
stdrc
force-pushed
the
rc/generalize-window-buffer
branch
from
8d89879 to
21e2a86
Compare
stdrc
force-pushed
the
rc/refactor-find-affected-ranges
branch
from
a148e1c to
b2653b5
Compare
stdrc
force-pushed
the
rc/generalize-window-buffer
branch
from
21e2a86 to
12760a7
Compare
stdrc
force-pushed
the
rc/refactor-find-affected-ranges
branch
2 times, most recently
from
c002f15 to
9e8731e
Compare
stdrc
force-pushed
the
rc/generalize-window-buffer
branch
from
12760a7 to
c44e1fe
Compare
stdrc
force-pushed
the
rc/generalize-window-buffer
branch
from
c44e1fe to
622719f
Compare
fuyufjh
approved these changes
Feb 1, 2024
Signed-off-by: Richard Chien <[email protected]>
Signed-off-by: Richard Chien <[email protected]>
Signed-off-by: Richard Chien <[email protected]>
Signed-off-by: Richard Chien <[email protected]>
Signed-off-by: Richard Chien <[email protected]>
Signed-off-by: Richard Chien <[email protected]>
stdrc
force-pushed
the
rc/generalize-window-buffer
branch
from
622719f to
86795a0
Compare
fuyufjh
reviewed
Feb 2, 2024
| type Value: Clone; | ||
|
|
||
| /// Whether the preceding half of the current window is saturated. | ||
| /// By "saturated" we mean that every row that is possible to be in the preceding half of the |
There was a problem hiding this comment.
Feel confused from my understanding of the English word "saturated" 😕
How about "buffered", "loaded" or "cached"?
There was a problem hiding this comment.
Maybe we can change the word later
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
This PR generalizes
WindowBufferandAggregateStateso that window aggregation for calls withRANGEframes can be easily supported by just adding a newWindowImplimplementation.Checklist
./risedev check(or alias,./risedev c)Documentation
Release note
If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.