Skip to content

Named window support - #6419

Merged
mustafasrepo merged 12 commits into
apache:mainfrom
synnada-ai:feature/named-window
May 25, 2023
Merged

Named window support#6419
mustafasrepo merged 12 commits into
apache:mainfrom
synnada-ai:feature/named-window

Conversation

@berkaysynnada

@berkaysynnadaberkaysynnada commented May 23, 2023

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes#6125.

Rationale for this change

With 0.34 release of sql-parser, statements can differentiate the named windows. We can now support such queries:

SELECT SUM(amount) OVER running_window, AVG(amount) OVER running_window
FROM sales_us
WINDOW running_window AS (ORDER BY ts)
ORDER BY ts
LIMIT 5;

What changes are included in this PR?

After the query parse, WindowType of the function can be in the form of WindowSpec or NamedWindow. In select_to_plan() function, if there exist some NamedWindow's, there are reshaped as WindowSpec's by using the definition of the windows. If there are any undefined windows, an error is generated.

sqlparser dependency is also updated to 0.34 with this PR.

Are these changes tested?

Yes, slt tests are added for both working and erroneous conditions.

Are there any user-facing changes?

@github-actionsgithub-actionsBot added core Core DataFusion crate logical-expr Logical plan and expressions sql SQL Planner sqllogictest SQL Logic Tests (.slt) labels May 23, 2023
mustafasrepo
mustafasrepo reviewed May 23, 2023
View reviewed changes
Comment threaddatafusion/sql/src/expr/mod.rs
mustafasrepo
mustafasrepo reviewed May 23, 2023
View reviewed changes
Comment threaddatafusion/sql/src/expr/order_by.rs
@berkaysynnada

berkaysynnada commented May 23, 2023•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

This PR needs sql-parser 0.34 release also. @alamb I have taken the necessary changes from #6416 regarding the sqlparser update. I wonder what you suggest, should I remove those parts and wait for your PR to be merged, or you prefer to continue with this PR?

@alamb

alamb commented May 23, 2023

Copy link
Copy Markdown
Contributor

This PR needs sql-parser 0.34 release also. @alamb I have taken the necessary changes from #6416 regarding the sqlparser update. I wonder what you suggest, should I remove those parts and wait for your PR to be merged, or you prefer to continue with this PR?

I recommend we wait for #6416 to merge, but we can review this PR earlier.

@alamb

alamb commented May 23, 2023

Copy link
Copy Markdown
Contributor

#6416 is merged

@mustafasrepo
Merge branch 'main' into feature/named-window
14c5b51
# Please enter a commit message to explain why this merge is necessary,
# especially if it merges an updated upstream into a topic branch.
#
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.
@mustafasrepo
Merge branch 'main' into feature/named-window
972a31b
# Conflicts:
#	datafusion/sql/src/expr/function.rs
#	datafusion/sql/src/expr/mod.rs
#	datafusion/sql/src/select.rs
#	datafusion/sql/src/statement.rs
@mustafasrepo
Minor changes
87608dd
@github-actionsgithub-actionsBot removed the logical-expr Logical plan and expressions label May 23, 2023
comphead
comphead reviewed May 23, 2023
View reviewed changes

@compheadcomphead left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great, I'm linking this new syntax to #6338 where is window functions get documented

@alamb

alamb commented May 23, 2023

Copy link
Copy Markdown
Contributor

I plan to review this tomorrow

alamb
alamb approved these changes May 24, 2023
View reviewed changes

@alambalamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @berkaysynnada and @mustafasrepo -- I think this looks great!

Comment threaddatafusion/core/tests/sqllogictests/test_files/window.slt
Comment threaddatafusion/core/tests/sqllogictests/test_files/window.slt
Comment threaddatafusion/sql/src/select.rs Outdated
Comment threaddatafusion/sql/src/select.rs
Comment threaddatafusion/sql/src/select.rs Outdated
@mustafasrepo
mustafasrepo merged commit a4233ad into apache:mainMay 25, 2023
@berkaysynnada
berkaysynnada deleted the feature/named-window branch May 25, 2023 10:22
alamb
alamb reviewed May 25, 2023
View reviewed changes

@alambalamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks again @berkaysynnada

Comment threaddatafusion/sql/src/select.rs
@alambalamb mentioned this pull request May 29, 2023
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alambalambalamb approved these changes
@compheadcompheadcomphead left review comments
+1 more reviewer
@mustafasrepomustafasrepomustafasrepo approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

coreCore DataFusion cratesqlSQL PlannersqllogictestSQL Logic Tests (.slt)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add support for named window frames

5 participants

@berkaysynnada@alamb@comphead@mustafasrepo@ozankabak