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

First draft for supporting window functions and other aggregate function expressions #4322

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

weiznich
Copy link
Member

@weiznich weiznich commented Oct 25, 2024

See
here and
here for the relevant postgresql documentation

This PR implements mainly the relevant DSL structure for this types. As of today this should support most of the relevant API surface.

This PR is not finished yet, I mainly push this now to get some early feedback on the exposed user API. See the two dummy tests for how to use the new functionality.

I'm mainly interested in the following questions:

  • Is the exposed structure reasonable?
  • Is the naming good?
  • What to do about frame clauses, it looks ugly right now
  • What to do about within_clauses?

What's still missing:

  • Support for other backends
  • Tests, a lot of tests
  • Compile tests to verify that we do not generate invalid SQL
  • Documentation

cc @oeed

…ion expressions

See
[here](https://www.postgresql.org/docs/current/sql-expressions.html#SYNTAX-WINDOW-FUNCTIONS)
and
[here](https://www.postgresql.org/docs/current/sql-expressions.html#SYNTAX-AGGREGATES)
for the relevant postgresql documentation

This PR implements mainly the relevant DSL structure for this types. As
of today this should support most of the relevant API surface.

This PR is not finished yet, I mainly push this now to get some early
feedback on the exposed user API. See the two dummy tests for how to use
the new functionality.

What's still missing:

- [ ] Support for other backends
- [ ] Tests, a lot of tests
- [ ] Compile tests to verify that we do not generate invalid SQL
- [ ] Documentation
@weiznich weiznich requested a review from a team October 25, 2024 16:35
@oeed
Copy link
Contributor

oeed commented Oct 28, 2024

Looks nice to me! Only observations from my end:

  • I see you added a distinct() function, e.g. dsl::count(users::id).distinct(), is that functionally the same as count_distinct()?
  • The naming of filter_aggregate() is a little more verbose (and potentially a little less discoverable) than filter(), I assume that was primarily done to prevent confusion with query filtering?

I haven't needed windowing functions much in the past, so don't have much to add on those.

@weiznich
Copy link
Member Author

I see you added a distinct() function, e.g. dsl::count(users::id).distinct(), is that functionally the same as count_distinct()?

Yes that's the same for count(), it just generalizes that to any aggregate function (as allowed by postgres). I plan to deprecate count_distinct() as soon as that is ready.

The naming of filter_aggregate() is a little more verbose (and potentially a little less discoverable) than filter(), I assume that was primarily done to prevent confusion with query filtering?

Rustc had issues resolving the right filter() method, so I just went with a different name. I think we should be able to improve the discoverability by adding a #[doc(alias = "filter")] there so that it shows up whn you search for filter in the API docs?

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

Successfully merging this pull request may close these issues.

2 participants