diesel icon indicating copy to clipboard operation
diesel copied to clipboard

Numeric implements SqlOrd

Open joel-u410 opened this issue 2 years ago • 4 comments

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

joel-u410 avatar Jun 13 '22 23:06 joel-u410

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.

weiznich avatar Jun 14 '22 14:06 weiznich

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.

joel-u410 avatar Jun 14 '22 14:06 joel-u410

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 avatar Nov 20 '22 14:11 scvalex

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; }

scvalex avatar Nov 20 '22 14:11 scvalex