Opened 6 years ago

Closed 6 years ago

#10127 closed (fixed)

select_related does not play well with annotate

Reported by: sylvain.pasche@… Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

This is with the latest trunk version. If you use select_related() with a query that does an aggegation over a ManyToMany relationship, the resulting fields get mixed up.

This is best demonstrated by a modification in the aggregates.py test:

Index: modeltests/aggregation/models.py
===================================================================
--- modeltests/aggregation/models.py    (revision 9791)
+++ modeltests/aggregation/models.py    (working copy)
@@ -162,7 +162,7 @@

 # Forward
 # Average age of the Authors of each book with a rating less than 4.5
->>> books = Book.objects.all().filter(rating__lt=4.5).annotate(Avg('authors__age'))
+>>> books = Book.objects.all().filter(rating__lt=4.5).select_related().annotate(Avg('authors__age'))
 >>> sorted([(b.name, b.authors__age__avg) for b in books])
 [(u'Artificial Intelligence: A Modern Approach', 51.5), (u'Practical Django Projects', 29.0), (u'Python Web Development with Django', 30.3...), (u'Sams Teach Yourself Django in 24 Hours', 45.0)]

With this modification, the test fails this way:

Failed example:
    sorted([(b.name, b.authors__age__avg) for b in books])
Expected:
    [(u'Artificial Intelligence: A Modern Approach', 51.5), (u'Practical Django Projects', 29.0), (u'Python Web Development with Django', 30.3...), (u'Sams Teach Yourself Django in 24 Hours', 45.0)]
Got:
    [(u'Artificial Intelligence: A Modern Approach', 7L), (u'Practical Django Projects', 3L), (u'Python Web Development with Django', 7L), (u'Sams Teach Yourself Django in 24 Hours', 1L)]

Attachments (1)

aggregation_regress_models.diff (644 bytes) - added by Sylvain Pasche <sylvain.pasche@…> 6 years ago.
aggregation_regress patch

Download all attachments as: .zip

Change History (8)

comment:1 Changed 6 years ago by Alex

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

What version of Django are you using? A similar ticket was filed about a week ago, and was subsequently fixed, if you haven't done svn up in a few days please try that and report back.

comment:2 Changed 6 years ago by Sylvain Pasche <sylvain.pasche@…>

I'm using trunk of today ([9791], Windows, Python 2.5), so looks like ticket:10064 didn't fix it. Maybe ticket:10086 is the same, but the testcase was not clear.

comment:3 Changed 6 years ago by ramiro

FWIW I can reproduce this with r9791:

  • On win32 with Python 2.5.2 + its bundled SQLite stuff (3.3.4 / sqlite3 module is 3.3.2) and with MySQL-python 1.2.2
  • On Debian (stable/Etch) with both:
    • Python 2.4.4 + MySQL-python 1.2.1p2, SQLite and Postgres (see below for more info)
    • Python 2.5 + MySQL-python 1.2.2 and and its sqlite3 module

On Debian, SQLite version is always 3.3.8, pysqlite/sqlite3 is 2.3.2.

MySQL version is always 5.0.32, MyISAM storage engine.

Under Debian, using python-psycopg2 2.0.5.1 against Postgres 8.1 it also fails but with an exception:

----------------------------------------------------------------------
File "~/t10127_test/tests/modeltests/aggregation/models.py", line ?, in modeltests.aggregation.models.__te
st__.API_TESTS
Failed example:
    sorted([(b.name, b.authors__age__avg) for b in books])
Exception raised:
    Traceback (most recent call last):
      File "~/t10127_test/django/test/_doctest.py", line 1267, in __run
        compileflags, 1) in test.globs
      File "<doctest modeltests.aggregation.models.__test__.API_TESTS[22]>", line 1, in ?
        sorted([(b.name, b.authors__age__avg) for b in books])
      File "~/t10127_test/django/db/models/query.py", line 186, in _result_iter
        self._fill_cache()
      File "~/t10127_test/django/db/models/query.py", line 667, in _fill_cache
        self._result_cache.append(self._iter.next())
      File "~/t10127_test/django/db/models/query.py", line 281, in iterator
        for row in self.query.results_iter():
      File "~/t10127_test/django/db/models/sql/query.py", line 240, in results_iter
        for rows in self.execute_sql(MULTI):
      File "~/t10127_test/django/db/models/sql/query.py", line 1960, in execute_sql
        cursor.execute(sql, params)
    ProgrammingError: column "aggregation_publisher.id" must appear in the GROUP BY clause or be used in an aggregate function

----------------------------------------------------------------------

comment:4 Changed 6 years ago by Koen Biermans <koen.biermans@…>

I also reported some errors in #10113. The group by problem on Postgres was fixed, but the wrong results on sqlite are caused by an sqlite bug (mine was 3.3.4). If you upgrade your sqlite, you shouldn't get wrong results no more.

I guess this should be documented somewhere or made to fail loud.

comment:5 Changed 6 years ago by russellm

  • Resolution set to fixed
  • Status changed from new to closed

(In [9805]) Fixed #10127 -- Corrected handling of select_related() in annotate() calls. Thanks to Sylvain Pasche <sylvain.pasche@…> for the report and test case.

Changed 6 years ago by Sylvain Pasche <sylvain.pasche@…>

aggregation_regress patch

comment:6 Changed 6 years ago by Sylvain Pasche <sylvain.pasche@…>

  • Resolution fixed deleted
  • Status changed from closed to reopened

Thanks for the fix. There is still an issue when using both select_related and annotate. The ForeignKey fields are not populated correctly.

With the attached patch, I'm getting the following error (which does not happen when I remove the select_related()):

File "[...]aggregation_regress\models.py", line ?, in regressiontests.aggregation_regress.models.__test__.API_TESTS
Failed example:
    sorted([b.publisher.name for b in books])
Expected:
    [u'Apress', u'Prentice Hall', u'Prentice Hall', u'Sams']
Got:
    [1L, 2L, 3L, 3L]

comment:7 Changed 6 years ago by russellm

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [9808]) Fixed #10127 -- Corrected (no, really, this time!) the way the select_related() cache is populated when annotations are also contained in the query. Thanks to Sylvain Pasche <sylvain.pasche@…> for the report and test case.

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