-
Notifications
You must be signed in to change notification settings - Fork 752
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: Window func #5401
feat: Window func #5401
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
Thanks for the contribution! Please review the labels and make any necessary changes. |
@mergify update |
✅ Branch has been successfully updated |
@@ -0,0 +1,5 @@ | |||
mod window_frame; |
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.
Hmm, need a license header to make the lint happy :)
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, I'll fix it then. And this pr's far away from ready for reviews 😂.
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.
A great start! Welcome asking at discussions if you have any questions 💌
Sorry for the late update, because I'm too busy recently. |
#[derive( | ||
Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Hash, serde::Serialize, serde::Deserialize, | ||
)] | ||
pub struct WindowFrame { |
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.
Code in this file is mainly copied from another open source project -- apache/arrow-datafusion. And I'm not sure whether it's proper or if I should add some description on the top of it.
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.
I think it's ok. Do we have stateless tests for this pr? Like: https://github.com/datafuselabs/databend/blob/49252766894f3bb5f5d4767037142cb1b10d92bc/tests/suites/0_stateless/03_dml/03_0003_select_group_by.sql
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.
Please attach their license header in this file with a clear statement, for example:
https://github.com/apache/arrow-datafusion/blob/master/datafusion/core/src/prelude.rs#L1-L16
And add descriptions include:
- Original Project
- Link to commits
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.
I think it's ok. Do we have stateless tests for this pr? Like: https://github.com/datafuselabs/databend/blob/49252766894f3bb5f5d4767037142cb1b10d92bc/tests/suites/0_stateless/03_dml/03_0003_select_group_by.sql
Thanks for the remind and I'll add some stateless tests then.
Impressive pr, I'll take time to review it. |
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.
👍 Look good to me. BTW: need implement window function in new processor.
@mergify update |
✅ Branch has been successfully updated |
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.
👍
Yes, I'll create a follow-up issue and also make some optimizations about performance. |
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.
Left some comments, these can be improved in another pr.
arguments.push(ColumnWithField::new(Arc::clone(arg_column), arg_field)); | ||
} | ||
|
||
let function = if !AggregateFunctionFactory::instance().check(op) { |
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.
we can have a function field in this transformer.
}) | ||
.collect::<Vec<_>>(); | ||
|
||
let window_col = Series::concat(&window_col_per_tuple).unwrap(); |
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.
unwrap could be replaced by Result.
.collect::<Vec<ColumnRef>>() | ||
}) | ||
.map(|args| { | ||
let place = arena.alloc_layout(func.state_layout()); |
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.
we should call drop_states to destroy the inner allocation as PartialAggregator
did.
Ping @soyeric128 for documentation. |
Could you pls help confirm?
Thanks. |
We support all aggregate functions(any function that impl trait AggregateFunction) but have not yet implemented any non-aggr functions. |
Thanks for reply. |
I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/
Summary
impl it partially including
Changelog
aggregate window function
Related Issues
Closes #4653