First draft for supporting window functions and other aggregate function expressions

diesel-rs / diesel

A safe, extensible ORM and Query Builder for Rust

https://diesel.rs

Apache License 2.0

12.79k stars 1.08k forks source link

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

Open weiznich opened 4 weeks ago

weiznich commented 4 weeks ago

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

oeed commented 3 weeks ago

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 commented 3 weeks ago

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?