Dzoukr / Dapper.FSharp

Lightweight F# extension for StackOverflow Dapper with support for MSSQL, MySQL, PostgreSQL, and SQLite
MIT License
365 stars 35 forks source link

Support for group by, distinct, and aggregate functions #5

Closed ssiltanen closed 4 years ago

ssiltanen commented 4 years ago

Hi,

For this PR my original use case was to complement the awesome paging feature this library already had.

The problem I have had with it is that usually when I create a paginated API, I would like to return a total row count for the given search filter. So far this was not possible by using the selectQueryBuilder of this library and I had to multiply the filter creation logic for both plain dapper query and for the paginated query using this library.

One option would have been to just create a function to turn Where construct of this library into a normal string SQL and parameters that could be passed to Dapper. That way I would only need to implement the where building logic once and use it both Dapper and this library. Even though I think this feature would be good for better fallback support to plain dapper, I thought that basic aggregate functions support could be a useful addition for this library anyway. And once I started that path, making support for just count, seemed a bit silly. That's why I implemented the other basic aggregate functions and tried to implement them seamlessly to the existing select functions.

Since I had gotten this far, I thought that aggregate functions without a group by feature is a bit stump feature and since it is quite easy to implement I added support for it too. And the same goes for distinct support.

Still, support for having clause is missing, but making it work like where clause seemed a bit intimidating for now so I didn't implement it, but I think it would be a nice feature to go along with group by and aggregate functions eventually.

The one thing that bothers me with the aggregate function solution here is that when using joined queries it is not possible to use the special values (for example in COUNT()) anymore since, there isn't enough information to bind the aggregate function to some output record field. For the simplicity of usage I did not go the extra mile to try to cover that caveat. Instead I tried to document the limitation to README. Do you have suggestions on how that part could be made better?

ssiltanen commented 4 years ago

Also I was not completely sure if the tuple returning select functions should have support for the aggregate methods but for now I added support for them too. What do you think?

Dzoukr commented 4 years ago

Hello @ssiltanen, thanks of amazing work again! I really appreciate your hard work, however I think this is exactly the example of "fallback to plain Dapper" scenario. Aggregate functions are great, but syntax can get ugly real quick:

select {
    table "Persons"
    innerJoin "Dogs" "OwnerId" "Persons.Id"
    groupBy [ "Persons.Position" ]
} |> fun query -> 
    conn.SelectAsync<{| position:int; average:int option; rowCount:int |}>(
        query, [ avg "Persons.Position" "average"; count "Persons.Position" "rowCount" ])

Syntax like this (even you tried to make it as nicer as possible) is not the nicest + it requires lambda, which makes it even uglier.

I'll keep you PR here and won't merge yet and will think about:

Just to make it clear: I am really grateful for your hard work and don't want to waste your time - I'll use this PR as starting point of thinking about aggregate functions as described above. 🙏

ssiltanen commented 4 years ago

Hi,

Yes, I agree with the arguments that you made and that is fine, no worries.

In that case, it could be considered if this could be solved by creating a method to fall back to plain old dapper from the constructs of this library. This also would solve my original feature request of being able to use this library while not having to duplicate its logic when running the count query. For example something like (deconstruct is just a name i tossed right now):

let script, parameters =
    select { 
        table "MyTable"
        where (eq "Id" 1)
    } |> deconstruct<MyType>

let countQuery = "SELECT COUNT(*) FROM (script) _"
...

Although the caveat in the whole select deconstruct is that we would need the output binding type to be able to create the select clause.

For just the where clause, it would be a bit simpler still:

let where, parameters = eq "Id" 1 |> deconstruct

This approach would keep the library tidy and simple while allowing the fallback to dapper easily.

ssiltanen commented 4 years ago

Should I create a draft of that proposal? @Dzoukr

Dzoukr commented 4 years ago

I'll do that. It's already on my list. 😉

Dzoukr commented 4 years ago

Done ;)

ssiltanen commented 4 years ago

Great! Thank you, this feature helps me a lot :)

Dzoukr commented 3 years ago

Hey @ssiltanen, you wanna check the latest version 1.13 of Dapper.FSharp, you'll find some familiar code there. 😉 I hope you'll like it.

ssiltanen commented 3 years ago

Thank you, having count cleans up my code base even further!