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.

Sign up for GitHub

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

Jump to bottom

Support FILTER in over clause #1007

Merged
merged 5 commits into from
Oct 23, 2023
Merged

Support FILTER in over clause #1007

merged 5 commits into from
Oct 23, 2023

Conversation

lovasoa
Copy link
Contributor

@lovasoa lovasoa commented Oct 18, 2023 •
edited
Loading

support FILTER clause in window functions

fixes #1006

@lovasoa lovasoa force-pushed the filter-in-over-clause branch from 4ada270 to c8cf584 Compare October 18, 2023 16:05
@coveralls
Copy link

coveralls commented Oct 18, 2023 •
edited
Loading

Pull Request Test Coverage Report for Build 6564024257

  • 66 of 67 (98.51%) changed or added relevant lines in 13 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 87.392%
Changes Missing Coverage Covered Lines Changed/Added Lines %
src/dialect/sqlite.rs 1 2 50.0%
Totals Coverage Status
Change from base Build 6435373786: 0.04%
Covered Lines: 16844
Relevant Lines: 19274
💛 - Coveralls

@lovasoa lovasoa force-pushed the filter-in-over-clause branch from c8cf584 to 64c232e Compare October 18, 2023 16:49
@lovasoa
Copy link
Contributor Author

lovasoa commented Oct 20, 2023

@alamb , I'd love to get your feedback on this 🙂

As before, I need this for SQLPage

@alamb alamb changed the title filter in over clause Support FILTER in over clause Oct 20, 2023
alamb
alamb approved these changes Oct 20, 2023
View reviewed changes
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.

Thanks @lovasoa -- I have a few suggestions and comments, but I also think we can merge this PR as is

src/ast/mod.rs Outdated Show resolved Hide resolved
src/ast/mod.rs Outdated Show resolved Hide resolved
src/ast/mod.rs Outdated Show resolved Hide resolved
tests/sqlparser_sqlite.rs Outdated Show resolved Hide resolved
src/ast/mod.rs Show resolved Hide resolved
src/dialect/sqlite.rs Show resolved Hide resolved
@lovasoa
Copy link
Contributor Author

lovasoa commented Oct 20, 2023 •
edited
Loading

@alamb : thanks for the review, I addressed your comments. We can see about removing AggregateExpressionWithFilter and supports_filter_during_aggregation later, but I'd be careful because it will prevent parsing some statements that parsed before.

lovasoa
lovasoa commented Oct 21, 2023
View reviewed changes
src/ast/mod.rs Outdated Show resolved Hide resolved
alamb
alamb approved these changes Oct 23, 2023
View reviewed changes
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.

Thanks @lovasoa -- I still thing something is confused here as I don't think SQL supports things like select foo FILTER(where x > 5) FROM t which I think this PR does allow.

However I think this is a reasonable improvement

It would be great to sort out the naming of Dialect::supports_filter_during_aggregation as that now also controls filter during any function

@alamb alamb merged commit ce62fe6 into apache:main Oct 23, 2023
10 checks passed
@lovasoa lovasoa deleted the filter-in-over-clause branch October 23, 2023 23:28
@alamb alamb mentioned this pull request Oct 30, 2023
Merged
serprex pushed a commit to serprex/sqlparser-rs that referenced this pull request Nov 6, 2023
@lovasoa @alamb @serprex
Support FILTER in over clause (apache#1007)
7db5d9f 
Co-authored-by: Andrew Lamb <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alamb alamb alamb approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

support FILTER clause in window functions with an OVER clause
3 participants
@lovasoa @coveralls @alamb