-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat: support distinct for window #16925
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
Conversation
|
FYI @crepererum |
|
Follow-up, we need to support more sliding distinct Accumulator: Currently, this PR only support sliding distinct Accumulator for count. But it should be easy because we already supported the whole steps from sql to physical plan with distinct in this PR. I can create follow-up PRs after this PR. |
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.
This turned out to be way more involved than I originally thought. I was hoping that this was just a forgotten boolean flag, but it seems that there's actually some proper wiring coding involved 😅
So thank you very much for implementing this so quickly ❤️
|
Thank you @crepererum for review! |
* feat: support distinct for window * fix * fix * fisx * fix unparse * fix test * fix test * easy way * add test * add comments
* feat: support distinct for window * fix * fix * fisx * fix unparse * fix test * fix test * easy way * add test * add comments
* feat: support distinct for window * fix * fix * fisx * fix unparse * fix test * fix test * easy way * add test * add comments
* feat: support distinct for window * fix * fix * fisx * fix unparse * fix test * fix test * easy way * add test * add comments
| window_frame, | ||
| aggregate, | ||
| ) | ||
| if distinct { |
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.
let aggregate = if distinct {..} else {..};
window_expr_from_aggregate_expr(
partition_by,
order_by,
window_frame,
aggregate,
)?
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.
Thank you @Omega359 for good suggestion and review, i will address it in any follow-up PR for me.
* feat: support distinct for window * fix * fix * fisx * fix unparse * fix test * fix test * easy way * add test * add comments
* feat: support distinct for window * fix * fix * fisx * fix unparse * fix test * fix test * easy way * add test * add comments
* feat: support distinct for window * fix * fix * fisx * fix unparse * fix test * fix test * easy way * add test * add comments
* feat: support distinct for window * fix * fix * fisx * fix unparse * fix test * fix test * easy way * add test * add comments
* feat: support distinct for window * fix * fix * fisx * fix unparse * fix test * fix test * easy way * add test * add comments
* feat: support distinct for window * fix * fix * fisx * fix unparse * fix test * fix test * easy way * add test * add comments
* feat: support distinct for window * fix * fix * fisx * fix unparse * fix test * fix test * easy way * add test * add comments
Which issue does this PR close?
Rationale for this change
support distinct for window
Details for window:
Follow-up, improve the performance for SlidingDistinctCountAccumulator, because we may can add each type specific for it also.
What changes are included in this PR?
support distinct for window
Details for window:
Follow-up, improve the performance for SlidingDistinctCountAccumulator, because we may can add each type specific for it also.
Are these changes tested?
Yes
Are there any user-facing changes?
New feature support now