Opened 5 years ago

Closed 5 years ago

#24767 closed New feature (fixed)

Add GREATEST and LEAST Query Expressions

Reported by: Ian Foote Owned by: Ian Foote
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Postgres supports GREATEST and LEAST query expressions. These could easily be implemented in django by subclassing the Coalesce query expression and changing the function attribute to 'GREATEST' or 'LEAST' respectively.

I'm not sure which other databases support this, so perhaps this can only be added to contrib.postgres.

If this isn't appropriate for core, I'm happy to create a small PyPI package instead.

Change History (15)

comment:1 Changed 5 years ago by Josh Smeaton

Triage Stage: UnreviewedAccepted

This is appropriate for core.

Oracle supports GREATEST and LEAST, any null results in null.
Sqlite has MAX(a, b, ..) and MIN(a, b, ..), any null results in null.
Mysql has GREATEST and LEAST, any null results in null if version >= 5.0.13 (django only supports >= 5.5 so no need to support both behaviours).

I'm not certain how we should handle nulls. Should we allow backend specific behaviour (probably not, historically) or try to decide which way is "better" for Django.

As a rough idea we could require a "if_null=" kwarg that would produce GREATEST( [COALESCE(arg, if_null) for args..] ).That'd produce the same value on all backends, I think, at the cost of extraneous sql. There should be an opt-out to get backend specific behaviour though.

comment:2 Changed 5 years ago by Ian Foote

For what it's worth, in my (current) use case the postgres handling of null is ideal. I don't know how typical it is though.

My use case is annotating a model with a last_updated datetime. This is calculated from the created datetime of two related models:

queryset.annotate(
    last_update=Greatest(
        Max('comments__created'),
        Max('documents__created'),
    ),
)

comment:3 Changed 5 years ago by Josh Smeaton

The postgres behaviour is the more useful IMO but it's also the odd one out. But that's why I suggested wrapping everything in a coalesce while providing opt outs.

I don't think we'd be able to emulate other backend behaviour with postgres anyway without quite complex case expressions.

So as far as I'm concerned, if you can provide an expression that works for postgres and emulates postgres on other backends (with an opt out), it'd be accepted into core. Happy to help if needed.

comment:4 Changed 5 years ago by Ian Foote

Owner: changed from nobody to Ian Foote
Status: newassigned

comment:5 Changed 5 years ago by Ian Foote

I've started work on this on a branch. It works for postgres, but I'm not sure how to go about extending it to other databases. I assume I need to start overwriting the as_vendorname methods, but it's not clear to me what to put in them.

Also, should I be using as_postgres instead of defaulting to the postgres behaviour?

comment:6 Changed 5 years ago by Ian Foote

Ok, I've realised that overriding the function used for sqlite is simple, so now I just need to add the emulation of postgres' null handling.

comment:7 Changed 5 years ago by Josh Smeaton

If you open up a pull request now it'll make it easier for me (and others) to provide some feedback inline with the code, even though it's not ready yet.

Tip, check the coalesce() method of the ConcatPair. You'll need to figure out the "default" value (by asking the user for it..) if an argument is none though.

Also, sqlite requires at least 2 arguments. If there's only 1 argument then it will use the aggregate MAX rather than the function mimicking GREATEST. You could silently add a None expression to the list, require a minimum of 2 expressions, or error out. But it should be handled some how.

comment:8 Changed 5 years ago by Ian Foote

comment:9 Changed 5 years ago by Ian Foote

Does it matter if sqlite uses the wrong MAX for one expression? I'm having trouble creating a failing test for that.

comment:10 Changed 5 years ago by Ian Foote

It looks like mysql also doesn't like one expression. Maybe requiring two or more is the best solution for mysql and sqlite.

It also looks like the coalesce method doesn't help on mysql.

comment:11 Changed 5 years ago by Marc Tamlyn <marc.tamlyn@…>

Resolution: fixed
Status: assignedclosed

In 4ab53a55:

Fixed #24767 -- Added Greatest and Least expressions

Greatest and Least are row-level Function versions of Min and Max.

comment:12 Changed 5 years ago by Tim Graham <timograham@…>

In 6b41834:

Minor edits to Greatest/Least docs; refs #24767.

comment:13 Changed 5 years ago by Tim Graham

Resolution: fixed
Status: closednew

The new tests are not passing on older versions of MySQL which don't support microseconds (see Jenkins).

comment:14 Changed 5 years ago by Tim Graham <timograham@…>

In 167a3203:

Fixed tests for refs #24767 on databases that don't support microseconds.

comment:15 Changed 5 years ago by Ian Foote

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top