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

Support dynamic order by clause #2061

Open
go-aegian opened this issue Feb 11, 2023 · 7 comments
Open

Support dynamic order by clause #2061

go-aegian opened this issue Feb 11, 2023 · 7 comments

Comments

@go-aegian
Copy link

go-aegian commented Feb 11, 2023

the order by clause in the :many definition query should be set dynamically

-- name: ListOrders :many
  select * from order where status = $1 order by @orderBy::text;

Generated

const listOrders = "select * from order where status = $1 order by @orderBy::text"

type ListOrdersParams struct {
      Status string
      OrderBy string
}
func (q *Queries) ListOrders(ctx context.Context, arg ListOrdersParams) ([]*ListOrdersRow, error) {
        orderBy:=arg.OrderBy
        if orderBy=="" {
           orderBy= "0"
        }
        listOrdersWithOrderBy = strings.Replace(listOrders, "@orderBy::text", orderBy, -1)

	rows, err := q.db.Query(ctx, listOrdersWithOrderBy, arg.Status)

...

What database engines need to be changed?

PostgreSQL, MySQL

What programming language backends need to be changed?

Go, Python, Kotlin

@go-aegian go-aegian added enhancement New feature or request triage New issues that hasn't been reviewed labels Feb 11, 2023
@kyleconroy kyleconroy added 📚 mysql 📚 postgresql 🔧 golang and removed triage New issues that hasn't been reviewed labels Jun 7, 2023
@jwc-clinnection
Copy link
Contributor

I have implemented this in #2343

@seanlaff
Copy link
Contributor

seanlaff commented Jan 10, 2024

I understand the desire to prevent sqlc turning into a templating language, as that defeats the "it's just sql" pitch. Perhaps there's a middle ground using existing sqlc patterns to cover the most common ORDER BY use case.

I'm imagining a new macro sqlc.orderBy() that takes inspiration from sqlc.slice(). Sqlc could use the AST to figure out what columns are available to sort and present those as typesafe args in the generated code.

SELECT id, name FROM authors ORDER BY sqlc.orderBy(myOrderBy);

-- use it as many times as you want
SELECT name
FROM (
  SELECT id, name FROM authors ORDER BY sqlc.orderBy(myInnerOrderBy) LIMIT 10
)
ORDER BY sqlc.orderBy(myOuterOrderBy)

Generating code like

type ListAuthorsOrderByFoo string

const (
    ListAuthorsSortIdAsc     ListAuthorsOrderByFoo = "id_asc"
    ListAuthorsSortIdDesc    ListAuthorsOrderByFoo = "id_desc"
    ListAuthorsSortNameAsc   ListAuthorsOrderByFoo = "name_asc"
    ListAuthorsSortNameDesc  ListAuthorsOrderByFoo = "name_desc"
)

const listAuthors = `-- name: ListAuthors :many
SELECT id, name FROM authors
ORDER BY /*ORDERBY:foo*/` // <- similiar to how sqlc handles slices for mysql

func (q *Queries) ListAuthors(ctx context.Context, foo []ListAuthorsFooOrderBy) ([]Author, error) {
  // ... map orderBy args to the query
}

A solution like that might move the needle enough to solve a lot of the order-by blockers. It doesn't support dynamic expressions, but at least it handles the most common use case of wanting to control the sort order of the columns present in your projection.

neekolas added a commit to xmtp/xmtp-node-go that referenced this issue Apr 25, 2024
## tl;dr

Bun has been bothering me for a while, so I've been working on a migration that will get rid of our ORM altogether and just use boring SQL queries for everything.

[`sqlc`](https://sqlc.dev/) is a very slick tool to generate code based on plain SQL queries using placeholders for arguments. It's not perfect...I had to do some gymnastics to make a few of the query types work. 

But the fact that there is no runtime other than the standard SQL driver and some generated code outweighs its limitations IMO. There's no fancy ORM library to worry about mangling your queries, and the learning curve is basically just "how well do you know SQL".

## What's wrong with Bun?
- No support for serializable transactions
- The SQL driver is not as well maintained as PGX
- High learning curve to build complex queries, even if you know SQL well
- Relations system is not very powerful and ends up doing N+1 queries a lot of the time.
- Configuring the database with struct tags is errorprone, and there aren't great checks to make sure the struct tags actually match the schema.

## Things that suck right now with sqlc

- I can't find a good way to have dynamic ORDER BY expressions. So I literally have separate queries for ASC and DESC versions. It's not the end of the world, but it's very frustrating. There's an [issue to fix it](sqlc-dev/sqlc#2061), and some hacky workarounds using CASE statements, but it's not great.
- Making the filters play nice with `json_populate_recordset` is a bit of a pain. Switching to the `pgx` driver helped, since I think there was a bug in Bun's pgdriver.

## Migration plan
We use Bun in a lot of places and for a lot of things today.

- It powers the `authz` database and all the migrations there
- It powers the migrations for the `message` database (but not the queries)
- It powers the `mls` database and all the queries in the `mlsstore`.

The priority right now is to remove it from the `mlsstore`. We will still use it for migrations (`sqlc` can read Bun migrations just fine).

This involves replacing the bun `pgdriver` with `pgx` (done in this PR) and replacing all the Bun ORM queries with `sqlc` queries. I have most of the queries written, but I'll split up the actual migration over several PRs. This can be done incrementally, but once the process is complete we can delete the Bun models.

We aren't using any of the fancy `sqlc` cloud features and have no plans to. 

## What knucklehead brought Bun into our codebase?

Ummmm. 😬. That was me.
neekolas added a commit to xmtp/xmtp-node-go that referenced this issue Apr 25, 2024
Bun has been bothering me for a while, so I've been working on a migration that will get rid of our ORM altogether and just use boring SQL queries for everything.

[`sqlc`](https://sqlc.dev/) is a very slick tool to generate code based on plain SQL queries using placeholders for arguments. It's not perfect...I had to do some gymnastics to make a few of the query types work.

But the fact that there is no runtime other than the standard SQL driver and some generated code outweighs its limitations IMO. There's no fancy ORM library to worry about mangling your queries, and the learning curve is basically just "how well do you know SQL".

- No support for serializable transactions
- The SQL driver is not as well maintained as PGX
- High learning curve to build complex queries, even if you know SQL well
- Relations system is not very powerful and ends up doing N+1 queries a lot of the time.
- Configuring the database with struct tags is errorprone, and there aren't great checks to make sure the struct tags actually match the schema.

- I can't find a good way to have dynamic ORDER BY expressions. So I literally have separate queries for ASC and DESC versions. It's not the end of the world, but it's very frustrating. There's an [issue to fix it](sqlc-dev/sqlc#2061), and some hacky workarounds using CASE statements, but it's not great.
- Making the filters play nice with `json_populate_recordset` is a bit of a pain. Switching to the `pgx` driver helped, since I think there was a bug in Bun's pgdriver.

We use Bun in a lot of places and for a lot of things today.

- It powers the `authz` database and all the migrations there
- It powers the migrations for the `message` database (but not the queries)
- It powers the `mls` database and all the queries in the `mlsstore`.

The priority right now is to remove it from the `mlsstore`. We will still use it for migrations (`sqlc` can read Bun migrations just fine).

This involves replacing the bun `pgdriver` with `pgx` (done in this PR) and replacing all the Bun ORM queries with `sqlc` queries. I have most of the queries written, but I'll split up the actual migration over several PRs. This can be done incrementally, but once the process is complete we can delete the Bun models.

We aren't using any of the fancy `sqlc` cloud features and have no plans to.

Ummmm. 😬. That was me.
neekolas added a commit to xmtp/xmtp-node-go that referenced this issue Apr 25, 2024
…382)

* Initial commit

* Set up sqlc

* Initial commit

* Set up sqlc

* Migrate first store methods to sqlc

* Migrate first store methods to sqlc

* Migrate first store methods to sqlc

* Add GetAddressLog SQL

* Add GetAddressLog SQL

* Merge branch '04-24-migrate_first_store_methods_to_sqlc' of github.com:xmtp/xmtp-node-go into 04-24-migrate_first_store_methods_to_sqlc

* Add GetAddressLog SQL

* Initial commit

* Set up sqlc

* Initial commit

* Set up sqlc

* Add InsertLog Query (#383)

* Add insertlog query

* validation service

* insert log

* revocation for removed members

* lint

* remove unnecessary log

* change test to use query from sqlc

* remove comments

* fix tests

* Migrate first store methods to sqlc

* Migrate MLS DB To SQLC (#380)

Bun has been bothering me for a while, so I've been working on a migration that will get rid of our ORM altogether and just use boring SQL queries for everything.

[`sqlc`](https://sqlc.dev/) is a very slick tool to generate code based on plain SQL queries using placeholders for arguments. It's not perfect...I had to do some gymnastics to make a few of the query types work.

But the fact that there is no runtime other than the standard SQL driver and some generated code outweighs its limitations IMO. There's no fancy ORM library to worry about mangling your queries, and the learning curve is basically just "how well do you know SQL".

- No support for serializable transactions
- The SQL driver is not as well maintained as PGX
- High learning curve to build complex queries, even if you know SQL well
- Relations system is not very powerful and ends up doing N+1 queries a lot of the time.
- Configuring the database with struct tags is errorprone, and there aren't great checks to make sure the struct tags actually match the schema.

- I can't find a good way to have dynamic ORDER BY expressions. So I literally have separate queries for ASC and DESC versions. It's not the end of the world, but it's very frustrating. There's an [issue to fix it](sqlc-dev/sqlc#2061), and some hacky workarounds using CASE statements, but it's not great.
- Making the filters play nice with `json_populate_recordset` is a bit of a pain. Switching to the `pgx` driver helped, since I think there was a bug in Bun's pgdriver.

We use Bun in a lot of places and for a lot of things today.

- It powers the `authz` database and all the migrations there
- It powers the migrations for the `message` database (but not the queries)
- It powers the `mls` database and all the queries in the `mlsstore`.

The priority right now is to remove it from the `mlsstore`. We will still use it for migrations (`sqlc` can read Bun migrations just fine).

This involves replacing the bun `pgdriver` with `pgx` (done in this PR) and replacing all the Bun ORM queries with `sqlc` queries. I have most of the queries written, but I'll split up the actual migration over several PRs. This can be done incrementally, but once the process is complete we can delete the Bun models.

We aren't using any of the fancy `sqlc` cloud features and have no plans to.

Ummmm. 😬. That was me.

* Migrate first store methods to sqlc

* Add InsertLog Query (#383)

* Add insertlog query

* validation service

* insert log

* revocation for removed members

* lint

* remove unnecessary log

* change test to use query from sqlc

* remove comments

* fix tests

* Add GetAddressLog SQL

* Merge branch '04-24-migrate_first_store_methods_to_sqlc' of github.com:xmtp/xmtp-node-go into 04-24-migrate_first_store_methods_to_sqlc

* Add InsertLog Query (#383)

* Add insertlog query

* validation service

* insert log

* revocation for removed members

* lint

* remove unnecessary log

* change test to use query from sqlc

* remove comments

* fix tests

* Add InsertLog Query (#383)

* Add insertlog query

* validation service

* insert log

* revocation for removed members

* lint

* remove unnecessary log

* change test to use query from sqlc

* remove comments

* fix tests

---------

Co-authored-by: Andrew Plaza <[email protected]>
@danielbraun89
Copy link

Any update on this?

@marcomayer
Copy link

I stumbled into this too, as it's quite common to have some kind of sorting that needs to be flexible. Think of showing data to users that they can sort by a number of different columns.

Is there a workaround for this besides having multiple queries defined with just the order by being different? Maybe with COALESCE or so? 🤔

@shagohead
Copy link

shagohead commented Jul 10, 2024

At now for having configurable ORDER BY clause you can do it like this:

ORDER BY
CASE WHEN @order_by::varchar = 'id_asc' THEN authors.id END ASC,
CASE WHEN @order_by = 'id_desc' THEN authors.id END DESC,
CASE WHEN @order_by = 'birth_year_asc' THEN authors.birth_year END ASC,
CASE WHEN @order_by = 'birth_year_desc' THEN authors.birth_year END DESC

But it will be a neat to have some enumerated type instead of just a string.

@kianooshaz
Copy link

Any update on this?

1 similar comment
@EfogDev
Copy link

EfogDev commented Oct 4, 2024

Any update on this?

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

No branches or pull requests

9 participants