feat(stream,over-window): implement `AggregateState` to allow aggregate function over window #9248

stdrc · 2023-04-18T08:06:23Z

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

This PR

Adds a common WindowBuffer structure to maintain buffered entries for a window frame.
Implements AggregateState which is a WindowState to support aggregate function over window.

Checklist For Contributors

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
~~[ ] I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).~~
I have demonstrated that backward compatibility is not broken by breaking changes and created issues to track deprecated features to be removed in the future. (Please refer to the issue)
All checks passed in ./risedev check (or alias, ./risedev c)

Checklist For Reviewers

I have requested macro/micro-benchmarks as this PR can affect performance substantially, and the results are shown.

Documentation

My PR DOES NOT contain user-facing changes.

Click here for Documentation

Types of user-facing changes

Please keep the types that apply to your changes, and remove the others.

Installation and deployment
Connector (sources & sinks)
SQL commands, functions, and operators
RisingWave cluster configuration changes
Other (please specify in the release note below)

Release note

src/stream/src/executor/over_window/state/aggregate.rs

Signed-off-by: Richard Chien <stdrc@outlook.com>

codecov · 2023-04-19T06:24:52Z

Codecov Report

Merging #9248 (155fd98) into main (3fe8bd4) will increase coverage by 0.05%.
The diff coverage is 96.38%.

@@            Coverage Diff             @@
##             main    #9248      +/-   ##
==========================================
+ Coverage   70.79%   70.85%   +0.05%     
==========================================
  Files        1216     1218       +2     
  Lines      202120   202616     +496     
==========================================
+ Hits       143089   143560     +471     
- Misses      59031    59056      +25

Flag	Coverage Δ
rust	`70.85% <96.38%> (+0.05%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/expr/src/function/window/kind.rs	`100.00% <ø> (ø)`
src/expr/src/function/window/call.rs	`70.00% <69.23%> (-30.00%)`	⬇️
...stream/src/executor/over_window/state/aggregate.rs	`96.51% <96.51%> (ø)`
...rc/stream/src/executor/over_window/state/buffer.rs	`98.83% <98.83%> (ø)`
src/stream/src/executor/over_window/mod.rs	`95.91% <100.00%> (+0.55%)`	⬆️
src/stream/src/executor/over_window/partition.rs	`95.45% <100.00%> (ø)`
src/stream/src/executor/over_window/state/lag.rs	`100.00% <100.00%> (ø)`
src/stream/src/executor/over_window/state/lead.rs	`100.00% <100.00%> (ø)`
src/stream/src/executor/over_window/state/mod.rs	`74.35% <100.00%> (+2.13%)`	⬆️

... and 6 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

soundOfDestiny · 2023-04-19T08:31:29Z

src/stream/src/executor/over_window/state/buffer.rs

+                    assert!(start_off <= end_off);
+                }
+                if end_off <= 0 {
+                    0


should it be LEFT_IDX here?

Seems to have bug here, let me add some unit tests later.

Fixed a bug related to index maintaining, PTAL

TennyZhuang · 2023-04-20T08:36:02Z

src/stream/src/executor/over_window/mod.rs

@@ -363,7 +362,9 @@ impl<S: StateStore> OverWindowExecutor<S> {
                let (ret_values, evict_hints): (Vec<_>, Vec<_>) = partition
                    .states
                    .iter_mut()
-                    .map(|state| state.slide())
+                    .map(|state| state.output())
+                    .try_collect::<Vec<_>>()?


What is the point of returning an error early here when .map(|o| (o.return_value, o.evict_hint)) has no side effects?

Because batch aggregator will possibly throw an error

Agree, but the collect is unnecessary if we have rust-itertools/itertools#402

We can resolve it later :)

TennyZhuang · 2023-04-20T08:42:43Z

src/stream/src/executor/over_window/state/lag.rs

    buffer: VecDeque<BufferEntry>,
    curr_idx: usize,
 }

 impl LagState {
    pub fn new(frame: &Frame) -> Self {
-        let offset = must_match!(frame, Frame::Offset(offset) if *offset < 0 => -offset as usize);
+        let offset = must_match!(frame, Frame::Rows(FrameBound::Preceding(offset), FrameBound::CurrentRow) => *offset);


What's the difference between our must_match and assert_matches?

Looks the same😄, will change to assert_matches.

TennyZhuang

LGTM

soundOfDestiny

👍

Signed-off-by: Richard Chien <stdrc@outlook.com>

This comment was marked as outdated.

Sign in to view

stdrc closed this Apr 18, 2023

stdrc force-pushed the rc/aggregate-window-state branch from 7dad266 to be50b16 Compare April 18, 2023 08:09

stdrc reopened this Apr 18, 2023

Base automatically changed from rc/unify-batch-stream-agg-call to main April 18, 2023 08:22

github-actions bot added the type/feature label Apr 18, 2023

stdrc force-pushed the rc/aggregate-window-state branch from 54e388d to 203f729 Compare April 18, 2023 08:22

stdrc mentioned this pull request Apr 18, 2023

Tracking: Over Window (a.k.a. Window Function or Over Aggregation) #9124

Open

37 tasks

stdrc changed the title ~~feat(stream,over-window): implement AggregateState to allow aggregate over window~~ feat(stream,over-window): implement AggregateState to allow aggregate function over window Apr 18, 2023

TennyZhuang reviewed Apr 18, 2023

View reviewed changes

src/stream/src/executor/over_window/state/aggregate.rs Outdated Show resolved Hide resolved

stdrc added 5 commits April 19, 2023 13:24

refine lag and lead

ae3737e

Signed-off-by: Richard Chien <stdrc@outlook.com>

add WindowBuffer structure

5b15677

Signed-off-by: Richard Chien <stdrc@outlook.com>

almost finish AggregateState

24e841b

Signed-off-by: Richard Chien <stdrc@outlook.com>

refine WindowBuffer

161fc05

Signed-off-by: Richard Chien <stdrc@outlook.com>

implement aggregation

9818593

Signed-off-by: Richard Chien <stdrc@outlook.com>

stdrc force-pushed the rc/aggregate-window-state branch from 773aa63 to 9818593 Compare April 19, 2023 05:25

stdrc added 4 commits April 19, 2023 13:50

add unit test for aggregate over window

0e47fdd

Signed-off-by: Richard Chien <stdrc@outlook.com>

fix WindowBufer::slide bug

f394c53

Signed-off-by: Richard Chien <stdrc@outlook.com>

add TODO to add unit tests

ee7b659

Signed-off-by: Richard Chien <stdrc@outlook.com>

fix rd check

8f40747

Signed-off-by: Richard Chien <stdrc@outlook.com>

stdrc marked this pull request as ready for review April 19, 2023 05:55

stdrc requested review from BugenZhao, st1page and soundOfDestiny April 19, 2023 05:55

stdrc added 2 commits April 19, 2023 13:56

add todo

ab1ceb0

Signed-off-by: Richard Chien <stdrc@outlook.com>

add todo

5da2017

Signed-off-by: Richard Chien <stdrc@outlook.com>

soundOfDestiny reviewed Apr 19, 2023

View reviewed changes

TennyZhuang reviewed Apr 20, 2023

View reviewed changes

TennyZhuang approved these changes Apr 20, 2023

View reviewed changes

soundOfDestiny approved these changes Apr 20, 2023

View reviewed changes

st1page approved these changes Apr 20, 2023

View reviewed changes

stdrc added 4 commits April 21, 2023 14:33

frame valid check

fff4922

Signed-off-by: Richard Chien <stdrc@outlook.com>

rename

1e462d8

Signed-off-by: Richard Chien <stdrc@outlook.com>

change curr_window_left to smallest_retained_key

215f1bc

Signed-off-by: Richard Chien <stdrc@outlook.com>

fix WindowBuffer

a4ea7d9

Signed-off-by: Richard Chien <stdrc@outlook.com>

stdrc requested a review from soundOfDestiny April 21, 2023 08:21

comment out assertions to avoid clippy warning

155fd98

Signed-off-by: Richard Chien <stdrc@outlook.com>

stdrc enabled auto-merge April 21, 2023 09:32

stdrc added this pull request to the merge queue Apr 21, 2023

Merged via the queue into main with commit 64aa26d Apr 21, 2023
30 checks passed

stdrc deleted the rc/aggregate-window-state branch April 21, 2023 10:16

This was referenced May 6, 2023

Support aggregate function as OVER window function #4978

Closed

Support lead, lag to access the previous or next row #8962

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(stream,over-window): implement `AggregateState` to allow aggregate function over window #9248

feat(stream,over-window): implement `AggregateState` to allow aggregate function over window #9248

stdrc commented Apr 18, 2023 •

edited

This comment was marked as outdated.

codecov bot commented Apr 19, 2023 •

edited

soundOfDestiny Apr 19, 2023

stdrc Apr 19, 2023

stdrc Apr 21, 2023

TennyZhuang Apr 20, 2023

stdrc Apr 20, 2023

TennyZhuang Apr 20, 2023

TennyZhuang Apr 20, 2023

stdrc Apr 20, 2023 •

edited

TennyZhuang left a comment

soundOfDestiny left a comment

feat(stream,over-window): implement AggregateState to allow aggregate function over window #9248

feat(stream,over-window): implement AggregateState to allow aggregate function over window #9248

Conversation

stdrc commented Apr 18, 2023 • edited

What's changed and what's your intention?

Checklist For Contributors

Checklist For Reviewers

Documentation

Types of user-facing changes

Release note

This comment was marked as outdated.

codecov bot commented Apr 19, 2023 • edited

Codecov Report

soundOfDestiny Apr 19, 2023

Choose a reason for hiding this comment

stdrc Apr 19, 2023

Choose a reason for hiding this comment

stdrc Apr 21, 2023

Choose a reason for hiding this comment

TennyZhuang Apr 20, 2023

Choose a reason for hiding this comment

stdrc Apr 20, 2023

Choose a reason for hiding this comment

TennyZhuang Apr 20, 2023

Choose a reason for hiding this comment

TennyZhuang Apr 20, 2023

Choose a reason for hiding this comment

stdrc Apr 20, 2023 • edited

Choose a reason for hiding this comment

TennyZhuang left a comment

Choose a reason for hiding this comment

soundOfDestiny left a comment

Choose a reason for hiding this comment

feat(stream,over-window): implement `AggregateState` to allow aggregate function over window #9248

feat(stream,over-window): implement `AggregateState` to allow aggregate function over window #9248

stdrc commented Apr 18, 2023 •

edited

codecov bot commented Apr 19, 2023 •

edited

stdrc Apr 20, 2023 •

edited