stephenafamo / bob

SQL query builder and ORM/Factory generator for Go with support for PostgreSQL, MySQL and SQLite
https://bob.stephenafamo.com
MIT License
760 stars 39 forks source link

Improve count query handling #226

Closed daddz closed 3 months ago

daddz commented 4 months ago

The Count() query does not properly strip any Preload...() (and ThenLoad...()) mods from the query.

This is especially cumbersome for queries where you need to filter the count based on a related table (to-one relation) with the auto-generated code. You have to build 2 different queries, one with SelectJoins for filtering and counting and another one with Preload...(psql.PreloadAs(...)) to again filter and get the actual results (All()).

The fact that Count() also modifies the original query makes re-using queries harder than necessary. (related discussion: https://github.com/stephenafamo/bob/discussions/177)

My initial solution was to simply modify the Count() method to also strip the PreloadColumns and Load fields of the base query which makes it at least possible to use it Count() together with Preload...() and ThenLoad...() but that still wasn't as nice.

Now I:

I guess there's an even nicer way to do it but I'm not that familiar with bob's code-base (yet) to come up with something better. :smile:

@stephenafamo: Let me know if this is a good approach and I'll add the asCountQuery variations for mysql and sqlite as well.

stephenafamo commented 4 months ago

This is a very good approach. I like it! This solves most of the issues around Count()

Although it would be less performant since it clones the whole query, I think it is worth it since it makes Count() work as expected.

One other thing that should be stripped is GroupBy().

daddz commented 3 months ago

@stephenafamo I added the GroupBy() stripping, the mysql and sqlite variants as well as a tip in the readme and changelog notes

daddz commented 3 months ago

I guess the asCountQuery could be reworked in the future to live only once outside the dialect/* but I couldn't find a satisfying solution so far. This might be a start at least.

stephenafamo commented 3 months ago

I guess the asCountQuery could be reworked in the future to live only once outside the dialect/*

I think it can be in internal

This way it can be reused, but it is not part of the public API.