Opened 3 years ago

Closed 3 years ago

Last modified 22 months ago

#33543 closed Cleanup/optimization (fixed)

Depracate passing False to OrderBy's nulls_first and nulls_last.

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

Description

Consider the following:

In [11]: [tv.published_at for tv in TemplateVersion.objects.order_by(F("published_at").desc(nulls_first=True))]
Out[11]: 
[None,
 datetime.datetime(2022, 2, 25, 13, 0, 12, 91916, tzinfo=<UTC>),
 datetime.datetime(2022, 2, 21, 10, 18, 0, 169248, tzinfo=<UTC>)]

In [12]: [tv.published_at for tv in TemplateVersion.objects.order_by(F("published_at").desc(nulls_first=False))]
Out[12]: 
[None,
 datetime.datetime(2022, 2, 25, 13, 0, 12, 91916, tzinfo=<UTC>),
 datetime.datetime(2022, 2, 21, 10, 18, 0, 169248, tzinfo=<UTC>)]

In [13]: [tv.published_at for tv in TemplateVersion.objects.order_by(F("published_at").desc(nulls_last=True))]
Out[13]: 
[datetime.datetime(2022, 2, 25, 13, 0, 12, 91916, tzinfo=<UTC>),
 datetime.datetime(2022, 2, 21, 10, 18, 0, 169248, tzinfo=<UTC>),
 None]

In [14]: [tv.published_at for tv in TemplateVersion.objects.order_by(F("published_at").desc(nulls_last=False))]
Out[14]: 
[None,
 datetime.datetime(2022, 2, 25, 13, 0, 12, 91916, tzinfo=<UTC>),
 datetime.datetime(2022, 2, 21, 10, 18, 0, 169248, tzinfo=<UTC>)]

Observe how nulls_first=False still puts the nulls first.

This happens because they both default False and when they are both False it lets the DB decide.

This is surprising behaviour, it also makes changing the null positioning based on a variable more awkward than it needs to be.

I think it would be better if they defaulted to None, let the DB decide when both are None and when one is not None do the ordering that implies.

Change History (7)

comment:1 by Adam Johnson, 3 years ago

I concur that it is confusing that nulls_first=False can still put the nulls first, and equally for the nulls_last=False.

I don't think we can change the semantics though as it would be a breaking change. The best we can probably do is make passing False for either a TypeError, by swapping their defaults for sentinel values.

Then, if you need a variable to switch between the behaviours you can use a construct like:

F(...).desc(**{("nulls_first" if nulls_first else "nulls_last"): True})

in reply to:  1 comment:2 by Mariusz Felisiak, 3 years ago

Summary: The behaviour of nulls_first and nulls_last is surprising and confusing.Depracate passing False to OrderBy's nulls_first and nulls_last.
Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

Agreed, this can be confusing.

Replying to Adam Johnson:

I concur that it is confusing that nulls_first=False can still put the nulls first, and equally for the nulls_last=False.

I don't think we can change the semantics though as it would be a breaking change. The best we can probably do is make passing False for either a TypeError, by swapping their defaults for sentinel values.

Then, if you need a variable to switch between the behaviours you can use a construct like:

F(...).desc(**{("nulls_first" if nulls_first else "nulls_last"): True})

... or we could change defaults to None and deprecate passing False, e.g.

  • django/db/models/expressions.py

    diff --git a/django/db/models/expressions.py b/django/db/models/expressions.py
    index a2da1f6e38..d4bb01362c 100644
    a b class OrderBy(Expression):  
    13571357    conditional = False
    13581358
    13591359    def __init__(
    1360         self, expression, descending=False, nulls_first=False, nulls_last=False
     1360        self, expression, descending=False, nulls_first=None, nulls_last=None
    13611361    ):
    13621362        if nulls_first and nulls_last:
    13631363            raise ValueError("nulls_first and nulls_last are mutually exclusive")
     1364        if nulls_first is False or nulls_last is False:
     1365            warnings.warn(...)
    13641366        self.nulls_first = nulls_first
    13651367        self.nulls_last = nulls_last
    13661368        self.descending = descending
    class OrderBy(Expression):  
    14271429
    14281430    def reverse_ordering(self):
    14291431        self.descending = not self.descending
    1430         if self.nulls_first or self.nulls_last:
    1431             self.nulls_first = not self.nulls_first
    1432             self.nulls_last = not self.nulls_last
     1432        if self.nulls_first is True:
     1433            self.nulls_first = None
     1434        if self.nulls_last is True:
     1435            self.nulls_last = None
    14331436        return self
    14341437
    14351438    def asc(self):
Last edited 3 years ago by Mariusz Felisiak (previous) (diff)

comment:3 by AllenJonathan, 3 years ago

Owner: changed from nobody to AllenJonathan
Status: newassigned

comment:4 by Mariusz Felisiak, 3 years ago

Has patch: set
Patch needs improvement: set

comment:5 by Mariusz Felisiak, 3 years ago

Patch needs improvement: unset

comment:6 by GitHub <noreply@…>, 3 years ago

Resolution: fixed
Status: assignedclosed

In 68da6b3:

Fixed #33543 -- Deprecated passing nulls_first/nulls_last=False to OrderBy and Expression.asc()/desc().

Thanks Allen Jonathan David for the initial patch.

comment:7 by Mariusz Felisiak <felisiak.mariusz@…>, 22 months ago

In 94ad46e9:

Refs #33543 -- Made Expression.asc()/desc() and OrderBy raise ValueError when nulls_first/nulls_last=False is passed.

Per deprecation timeline.

Note: See TracTickets for help on using tickets.
Back to Top