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

Numeric implements SqlOrd #3206

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

Conversation

joel-u410
Copy link

For postgres at least (untested on other databases), Numeric should implement SqlOrd to allow use of aggregation functions like max().

@weiznich
Copy link
Member

Thanks for opening this PR. Generally speaking it is fine to add such an impl, but we cannot accept a SqlOrder impl that is conditionally applied if a specific feature is passed as this would enabled for the sqlite/mysql backend as well as soon as a user want to use more than one backend at once. This means I would like to see tests for all backends. Those should be part of the PR.

@joel-u410
Copy link
Author

Makes sense. It probably works for other backends too (I'd expect that Numeric types in any database would support ordering operations) -- the only reason I'd made it conditional was that Postgres was the only backend I was using/testing. I'll look into adding tests for other supported backends and making it unconditional.

@scvalex
Copy link

scvalex commented Nov 20, 2022

I just ran into this as well.

@joel-u410 Any chance you could add the requested tests so that this gets merged? Otherwise, do you mind if I fork your branch, add the tests, and open a new PR?

@scvalex
Copy link

scvalex commented Nov 20, 2022

In case anyone else runs into this before it gets merged, the simple workaround is to define our own min and max functions:

use diesel::sql_types::Numeric;

sql_function! { fn min(x: Numeric) -> Numeric; }
sql_function! { fn max(x: Numeric) -> Numeric; }

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.

3 participants