-
Notifications
You must be signed in to change notification settings - Fork 1.6k
fix: window unparsing #17367
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
base: main
Are you sure you want to change the base?
fix: window unparsing #17367
Conversation
assert_snapshot!( | ||
sql, | ||
@r#"SELECT "test"."k", "test"."v", rank() OVER (PARTITION BY "test"."k" ORDER BY "test"."v" ASC NULLS FIRST ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING) FROM "test" QUALIFY (rank() OVER (PARTITION BY "test"."k" ORDER BY "test"."v" ASC NULLS FIRST ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING) = 1)"# |
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 @chenkovsky, this now generates valid Datafusion SQL.
Unfortunately, the QUALIFY
keyword is not available in PostgreSQL, as well as other systems (https://modern-sql.com/caniuse/qualify), meaning the output does not work there.
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 @chenkovsky, LGTM. I leave below just a minor note.
impl Dialect for PostgreSqlDialect { | ||
fn supports_qualify(&self) -> bool { | ||
false | ||
} |
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.
Like PostgreSQL, MySQL and SQLite also do not support the Qualify
keyword, so they should also have supports_qualify
returning false
.
Which issue does this PR close?
Rationale for this change
in LogicalPlan::Filter unparsing,
if there's a window expr, it should be converted to quailify.
What changes are included in this PR?
If window expr is found, convert filter to quailify.
Are these changes tested?
UT
Are there any user-facing changes?
No