#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)
follow-up: 2 comment:1 by , 3 years ago
comment:2 by , 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: | Unreviewed → Accepted |
Type: | Uncategorized → Cleanup/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 thenulls_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 aTypeError
, 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): 1357 1357 conditional = False 1358 1358 1359 1359 def __init__( 1360 self, expression, descending=False, nulls_first= False, nulls_last=False1360 self, expression, descending=False, nulls_first=None, nulls_last=None 1361 1361 ): 1362 1362 if nulls_first and nulls_last: 1363 1363 raise ValueError("nulls_first and nulls_last are mutually exclusive") 1364 if nulls_first is False or nulls_last is False: 1365 warnings.warn(...) 1364 1366 self.nulls_first = nulls_first 1365 1367 self.nulls_last = nulls_last 1366 1368 self.descending = descending … … class OrderBy(Expression): 1427 1429 1428 1430 def reverse_ordering(self): 1429 1431 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 1433 1436 return self 1434 1437 1435 1438 def asc(self):
comment:3 by , 3 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
I concur that it is confusing that
nulls_first=False
can still put the nulls first, and equally for thenulls_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 aTypeError
, by swapping their defaults for sentinel values.Then, if you need a variable to switch between the behaviours you can use a construct like: