Fixed #13312 -- Added a way to customize the order_by() of null fields. #6981

boblefrag · 2016-07-27T13:18:40Z

https://code.djangoproject.com/ticket/13312

charettes · 2016-07-27T13:37:55Z

django/db/models/expressions.py

+        template = None
+        if self.nullslast:
+            template = 'IF(ISNULL(%(expression)s),1,0),%(expression)s %(ordering)s '
+            sql, params = self.as_sql(compiler, connection, template=template)


Is there a reason not to use the pattern used in other as_* methods?

No, my bad. Fixed

charettes · 2016-07-27T13:54:42Z

I suppose we could make OrderBy.asc()/desc() chainable in another commit by making them return self.

jarshwah · 2016-07-27T23:41:28Z

django/db/models/expressions.py

+        sql, params = self.as_sql(compiler, connection, template=template)
+        return sql, params
+
+    def as_postgresql(self, compiler, connection):


It'd be better to rename this as_sql and to remove the as_oracle method since it is the "default" implementation that all backends follow unless they opt out. As-is, any third party backends must implement their own version even if they support NULLS LAST syntax, such as mssql.

boblefrag · 2016-07-28T11:21:47Z

@charettes now asc and desc are chainable with nulls_last as asked.

charettes · 2016-07-28T11:25:59Z

django/db/models/expressions.py

@@ -879,6 +883,8 @@ def get_source_expressions(self):
        return [self.expression]

    def as_sql(self, compiler, connection, template=None, **extra_context):
+        if not template and self.nullslast:
+            template = '%(expression)s %(ordering)s NULLS LAST'


Use "%s NULLS LAST" % self.template to prevent code duplication.

charettes · 2016-07-28T11:30:30Z

Should we also add support for NULLS FIRST?

charettes · 2016-07-28T11:36:00Z

I feel like making nulls a parameter of asc()/desc() would make a better API than chaining by exposing both a nulls_first() and nulls_last() method. e.g.:

Event.objects.order_by(F('start_date').asc(nulls='first')) or
Event.objects.order_by(F('start_date').asc(nulls_first=True))

boblefrag · 2016-07-29T23:41:04Z

@charette now comply with the api you suggest.

charettes · 2016-08-01T15:56:04Z

@jarshwah, @mjtamlyn what you think of the (nulls='first') API? The only thing that bugs me is the usage of a string constant as it not used anywhere else in the ORM. We could also take a tri-valued nulls_first (or nulls_last kwarg) where False maps to NULLS LAST and None does nothing.

jarshwah · 2016-08-01T23:51:53Z

Don't like the string argument, sorry, and I feel that a tri-value would constantly require referencing the docs. I'd prefer nulls_first=True or nulls_last=True as an argument to asc/desc. Are there any better options?

charettes · 2016-08-02T01:54:59Z

Are there any better options?

I can't think of any. Maybe the this should be taken to the developpers mailing list?

boblefrag · 2016-08-02T09:06:13Z

I am ok with nulls_first=True syntax but before implementing it I need a green flag of yours so just let me know the endorsed syntax and I'll implement it.

mjtamlyn · 2016-08-02T09:53:25Z

I like the nulls_first=True option best as well. It's not brilliant, but best of a bad bunhc.

charettes · 2016-08-08T17:09:57Z

@boblefrag let's go with nulls_first=True and nulls_last=True.

You might want to delegate asc()/desc()'s **kwargs to OrderBy.__init__() and raise a ValueError if both nulls_first and nulls_last are specified.

adamchainz · 2016-08-15T15:25:08Z

FWIW nulls_first=True and nulls_last=True seems like a nice solution to me too (that said using a set of allowed strings like nulls='first' isn't exactly unpythonic per se, c.f. bytes.decode('ascii', errors='replace')).

boblefrag · 2016-08-16T13:40:39Z

@charettes nulls_first=True and nulls_last=True. What's next step?

charettes · 2016-08-16T13:58:22Z

The code is LGTM. Maybe break the nulls_first and nulls_last options assertions in two tests and add an additional test method for the ValueError raised when both options are used together.

boblefrag · 2016-08-17T08:24:05Z

@charettes done.

charettes · 2016-08-17T13:00:55Z

tests/ordering/tests.py

@@ -90,6 +90,90 @@ def test_order_by_override(self):
            attrgetter("headline")
        )

+    def test_order_by_nulls(self):
+        with self.assertRaises(ValueError):


Use assertRaisesMessage.

charettes · 2016-08-17T13:05:45Z

tests/ordering/tests.py

+        Article.objects.create(
+            headline="Article 5",
+            author=author_1,
+            pub_date=datetime(2005, 7, 28)


Add a trailing comma.

charettes · 2016-08-17T13:12:21Z

You can reuse the Article objects created in setup instead of creating two new ones per tests by adding an author to one of them. You could also convert the test class to use setupTestData instead of setUp. Please use the title of this PR as commit message.

rebase

boblefrag · 2016-08-22T08:27:56Z

@charettes is there anything left to be done for making this PR mergeable ?

charettes · 2016-08-22T10:36:15Z

Marked the ticket as ready for final review.

timgraham · 2016-09-02T14:54:55Z

Documentation is missing. See our patch review checklist for details. Please uncheck "Needs documentation" on the ticket after updating.

danielsamuels · 2016-09-10T07:33:46Z

django/db/models/expressions.py

+    def __init__(self, expression, descending=False, nulls_first=False,
+                 nulls_last=False):
+        if nulls_first and nulls_last:
+            raise ValueError("nulls_first and nulls_last are mutualy exclusive")


Minor typo: should be 'mutually'.

boblefrag · 2016-09-14T12:06:43Z

fixed typo and added doc

timgraham

There's a test failure on Oracle also:

======================================================================
FAIL: test_order_by_nulls_first (ordering.tests.OrderingTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/tim/code/django/tests/ordering/tests.py", line 138, in test_order_by_nulls_first
    attrgetter("headline")
  File "/home/tim/code/django/django/test/testcases.py", line 964, in assertQuerysetEqual
    return self.assertEqual(list(items), values, msg=msg)
AssertionError: Lists differ: ['Article 1', 'Article 4', 'Article 3', 'Article 2'] != ['Article 1', 'Article 2', 'Article 3', 'Article 4']

First differing element 1:
'Article 4'
'Article 2'

- ['Article 1', 'Article 4', 'Article 3', 'Article 2']
?                        ^                         ^

+ ['Article 1', 'Article 2', 'Article 3', 'Article 4']

timgraham · 2016-09-16T23:56:12Z

docs/ref/models/expressions.txt

@@ -551,10 +551,14 @@ calling the appropriate methods on the wrapped expression.
    .. method:: asc()

        Returns the expression ready to be sorted in ascending order.
+        `nulls_last` and `nulls_first` define how null values


These should be added as kwargs to the method signatures.
Use double backticks rather than single.
Add a mention in the 1.11 release notes.
Add .. versionchanged:: 1.11 annotations as described in https://docs.djangoproject.com/en/dev/internals/contributing/writing-documentation/#documenting-new-features.

timgraham · 2016-09-16T23:57:23Z

tests/ordering/tests.py

@@ -90,6 +96,58 @@ def test_order_by_override(self):
            attrgetter("headline")
        )

+    def test_order_by_nulls_first_and_last(self):
+        with self.assertRaisesMessage(ValueError,


Use:

msg = '...' with self.assertRaisesMessage(ValueError, msg):

timgraham · 2016-09-16T23:57:44Z

tests/ordering/tests.py

+        Article.objects.filter(headline="Article 3").update(author=self.author_1)
+        Article.objects.filter(headline="Article 4").update(author=self.author_2)
+
+        # check that asc and desc are chainable with nulls_last


chop "check that" and just state the expected behavior.

timgraham · 2016-09-16T23:58:32Z

tests/ordering/tests.py

+
+        # check that asc and desc are chainable with nulls_last
+
+        self.assertQuerysetEqual(


Use:

self.assertSequenceEqual( Article.objects.order_by(F("author").desc(nulls_last=True)), [self.a4, ...] )

timgraham · 2016-09-16T23:58:56Z

tests/ordering/tests.py

-    def setUp(self):
-        self.a1 = Article.objects.create(
+
+    @classmethod


Please make this change and the test changes unrelated to the bug fix in a separate commit for clarity.

this have been made to comply with #6981 (comment)

The change is fine, I'm just asking that it be a be separate commit similar to 31098e3 so that the refactoring isn't mixed in with the new feature.

Does it make sense?

… fields

boblefrag · 2016-10-18T14:37:56Z

@timgraham here is the 2 commits. Let me know if it's ok for you.

timgraham · 2016-10-18T15:15:07Z

tests/ordering/tests.py

@@ -213,14 +265,10 @@ def test_order_by_pk(self):
        Ensure that 'pk' works as an ordering option in Meta.
        Refs #8291.
        """
-        Author.objects.create(pk=1)


My think was that the setUpTestData changes would be the first commit and changes like this would be included there so that the bug fix commit doesn't have to make changes in unrelated tests.

felixxm · 2016-11-13T14:55:58Z

tests/ordering/tests.py

+            ]
+        )
+
+    def test_order_by_nulls_first(self):


Please add authors for article3 and article4 like in test_order_by_nulls_last:

--- a/tests/ordering/tests.py +++ b/tests/ordering/tests.py @@ -125,6 +125,8 @@ class OrderingTests(TestCase): ) def test_order_by_nulls_first(self): + Article.objects.filter(headline="Article 3").update(author=self.author_1) + Article.objects.filter(headline="Article 4").update(author=self.author_2) self.assertSequenceEqual( Article.objects.order_by(F("author").asc(nulls_first=True)), [ self.a1, @@ -138,8 +140,8 @@ class OrderingTests(TestCase): Article.objects.order_by(F("author").desc(nulls_first=True)), [ self.a1, self.a2, - self.a3, - self.a4 + self.a4, + self.a3 ] )

Now we have 4 articles without authors, hence we do not check that asc and desc works with nulls_first flag.

timgraham · 2016-11-15T20:28:51Z

Updated in #7563.

charettes reviewed Jul 27, 2016
View reviewed changes

boblefrag force-pushed the ticket_13312 branch from 48bd2d6 to fa750c8 Compare July 27, 2016 13:45

charettes changed the title ~~Fixed #13312 -- Add a way to customize the order_by() of null fields~~ Fixed #13312 -- Added a way to customize the order_by() of null fields. Jul 27, 2016

boblefrag force-pushed the ticket_13312 branch 2 times, most recently from bdee62b to 8d84390 Compare July 27, 2016 15:53

jarshwah reviewed Jul 27, 2016
View reviewed changes

charettes reviewed Jul 28, 2016
View reviewed changes

boblefrag force-pushed the ticket_13312 branch 2 times, most recently from 118136b to d7c4e3f Compare July 29, 2016 18:19

boblefrag force-pushed the ticket_13312 branch from d7c4e3f to 9f85a72 Compare August 16, 2016 07:21

boblefrag force-pushed the ticket_13312 branch from 9f85a72 to c6ffc79 Compare August 16, 2016 14:17

charettes reviewed Aug 17, 2016
View reviewed changes

boblefrag force-pushed the ticket_13312 branch from c6ffc79 to f47ba4d Compare August 18, 2016 08:56

Merge pull request #1 from django/master

b9d47fd

rebase

danielsamuels reviewed Sep 10, 2016
View reviewed changes

boblefrag force-pushed the ticket_13312 branch from f47ba4d to ad5219f Compare September 14, 2016 12:05

timgraham requested changes Sep 17, 2016

View reviewed changes

boblefrag force-pushed the ticket_13312 branch 3 times, most recently from 1a57da5 to 7f2db39 Compare September 23, 2016 16:18

Merge remote-tracking branch 'upstream/master'

2db509c

boblefrag force-pushed the ticket_13312 branch from 7f2db39 to 520c517 Compare September 23, 2016 16:24

boblefrag and others added 2 commits October 18, 2016 11:13

Fixed django#13312 -- Added a way to customize the order_by() of null…

8744774

… fields

Used setUpTestData for the ordering testcase.

48fb868

boblefrag force-pushed the ticket_13312 branch from 520c517 to 48fb868 Compare October 18, 2016 09:16

timgraham reviewed Oct 18, 2016

View reviewed changes

felixxm requested changes Nov 13, 2016

View reviewed changes

This was referenced Nov 14, 2016

Refs #13312 -- Used setUpTestData for the ordering testcase. #7555

Merged

Fixed #13312 -- Added a way to customize the order_by() of null fields #7563

Merged

timgraham closed this Nov 15, 2016

charettes mentioned this pull request Oct 31, 2019

Refs #13312 -- Removed unnecessary IF wrapping in MySQL nulls_last handling. #12001

Merged


		# check that asc and desc are chainable with nulls_last

		self.assertQuerysetEqual(

Fixed #13312 -- Added a way to customize the order_by() of null fields. #6981

Fixed #13312 -- Added a way to customize the order_by() of null fields. #6981

Conversation

boblefrag commented Jul 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charettes commented Jul 27, 2016

Choose a reason for hiding this comment

boblefrag commented Jul 28, 2016

Choose a reason for hiding this comment

charettes commented Jul 28, 2016

charettes commented Jul 28, 2016

boblefrag commented Jul 29, 2016

charettes commented Aug 1, 2016

jarshwah commented Aug 1, 2016

charettes commented Aug 2, 2016

boblefrag commented Aug 2, 2016

mjtamlyn commented Aug 2, 2016

charettes commented Aug 8, 2016

adamchainz commented Aug 15, 2016 • edited

boblefrag commented Aug 16, 2016

charettes commented Aug 16, 2016

boblefrag commented Aug 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charettes commented Aug 17, 2016

boblefrag commented Aug 22, 2016 • edited

charettes commented Aug 22, 2016

timgraham commented Sep 2, 2016

Choose a reason for hiding this comment

boblefrag commented Sep 14, 2016

timgraham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boblefrag commented Oct 18, 2016

Choose a reason for hiding this comment

felixxm Nov 13, 2016 • edited

Choose a reason for hiding this comment

timgraham commented Nov 15, 2016

adamchainz commented Aug 15, 2016 •

edited

boblefrag commented Aug 22, 2016 •

edited

felixxm Nov 13, 2016 •

edited