-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Avoid Aliased Window Expr Enter Unreachable Code #14109
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
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
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 @berkaysynnada and @ozankabak
| extract_partition_keys(window_func) | ||
| } else { | ||
| // window functions expressions are only Expr::WindowFunction | ||
| unreachable!() |
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.
maybe we can turn this into returning an internal error rather than a panic for safety
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.
Sounds good. I think there are few of those in the codebase and it would make a good first issue to remove these unreachable panics and replace them with internal errors. Let's create an issue for this
(cherry picked from commit fda500a)
(cherry picked from commit fda500a)
(cherry picked from commit fda500a)
(cherry picked from commit fda500a)
Which issue does this PR close?
Closes #14108.
Rationale for this change
Please see the issue.
What changes are included in this PR?
Aliased window expr is handled as if it is a normal window expr
Are these changes tested?
Yes, with an .slt test, which was failing before
Are there any user-facing changes?