-
Notifications
You must be signed in to change notification settings - Fork 600
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
feat(stream): implement HopWindowExecutor #1718
Conversation
Signed-off-by: TennyZhuang <[email protected]>
Signed-off-by: TennyZhuang <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #1718 +/- ##
==========================================
+ Coverage 71.12% 71.33% +0.21%
==========================================
Files 603 605 +2
Lines 78066 78553 +487
==========================================
+ Hits 55524 56037 +513
+ Misses 22542 22516 -26
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
Signed-off-by: TennyZhuang <[email protected]>
Signed-off-by: TennyZhuang <[email protected]>
Ready for review! |
Signed-off-by: TennyZhuang <[email protected]>
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.
LGTM
('-', 5, 3, t(10, 33)), | ||
('+', 6, 2, t(10, 42)), | ||
('-', 7, 1, t(10, 51)), | ||
('+', 8, 3, t(11, 02)), |
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.
That's coooool! Hope others can also write their tests within this style
StreamExecutorError::InvalidArgument(format!( | ||
"window_size {} cannot be divided by window_slide {}", | ||
window_size, self.window_slide | ||
)) |
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.
Better to do this check in frontend
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.
But frontend don't need the result..
let window_start_offset = | ||
self.window_slide.checked_mul_int(i).ok_or_else(|| { | ||
StreamExecutorError::InvalidArgument(format!( | ||
"window_slide {} cannot be multiplied by {}", |
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.
Just curious... How can this happen?
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.
Not sure :(
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.
The IntervalUnit
can only represent months in i32,if the month is too big or the units are too big, it will happen (
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.
Same to days
Signed-off-by: TennyZhuang <[email protected]>
What's changed and what's your intention?
PLEASE DO NOT LEAVE THIS EMPTY !!!
Implement hop window in backend.
Checklist
Refer to a related PR or issue link (optional)