Skip to content
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

Customize window frame support for dialect #14288

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Sevenannn
Copy link
Contributor

Which issue does this PR close?

NA

Rationale for this change

Some dialect doesn't support window frames in window function, for example, in dremio, window frame doesn't work with RANK, DENSE_RANK or ROW_NUMBER functions, and would cause error like will cause error like ROW/RANGE not allowed with RANK, DENSE_RANK or ROW_NUMBER functions.

What changes are included in this PR?

Include a method for Dialect determining whether the Dialect support window frame based on the window function name, start_bound, end_bound. When the window frame start_bound and end_bound indicates that no frame is specified in the original query, and the window function requires no window frame to function in the expected way, the window function will be unparsed as None in ast.

Are these changes tested?

Yes

Are there any user-facing changes?

No

* Customize window frame support for dialect

* fix: ignore frame only when frame implies no frame

* Add comments. move the window frame determine logic to dialect method

* Update datafusion/sql/src/unparser/dialect.rs

* Update test case

* fix

---------

Co-authored-by: Phillip LeBlanc <[email protected]>
@github-actions github-actions bot added the sql SQL Planner label Jan 25, 2025
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me -- thanks @Sevenannn and @phillipleblanc (I see your name on one of the commits)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sql SQL Planner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants