Opened 7 years ago

Closed 5 years ago

Last modified 5 years ago

#11890 closed Bug (fixed)

Defer/only + annotate is broken

Reported by: jaklaassen@… Owned by: ruosteinen
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: milos.urbanek@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

If you use defer or only + annotate, the ORM returns an empty list. To replicate this:

class Manufacturer(models.Model):
  name = models.CharField(max_length=64)
  country = models.CharField(max_length=32)

class Car(models.Model):
  manufacturer = models.ForeignKey(Manufacturer, related_name='models')

>>>Manufacturer.objects.only('name').annotate(Count('models'))
>>>[]

This is due to a typo in django.db.models.sql.query at line 714.

The existing code:

if table in only_load and col not in only_load[table]:

should read

if table in only_load and column not in only_load[table]:

Attachments (5)

query.py (1.7 KB) - added by jaklaassen@… 7 years ago.
django.db.models.query.py
11980_defer_plus_annotate_fix.diff (4.3 KB) - added by ruosteinen 7 years ago.
11890_regression_tests.diff (5.5 KB) - added by ruosteinen 7 years ago.
Regression test diff against trunk.
16409-defer-only-annotate-r16510.diff (2.7 KB) - added by Tai Lee 5 years ago.
Bogus test.
11890-extra-select-related-r16522.diff (3.3 KB) - added by Tai Lee 5 years ago.
Removed explicit PKs from test. Still bogus.

Download all attachments as: .zip

Change History (19)

Changed 7 years ago by jaklaassen@…

Attachment: query.py added

django.db.models.query.py

comment:1 Changed 7 years ago by jaklaassen@…

Has patch: set
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

After a little more work, I had two more problems directly related to this. The iterator method of QuerySet and the results_iter method of Query seem to be calculating the index of the start of the aggregation_fields incorrectly when also using only/defer. I am a little less certain of the solution I have working for this.

I have attached the patch for Query because it is a little more involed.

QuerySet iterator() defines aggregate_start on line 204:

aggregate_start = index_start + len(self.model._meta.fields)

I would propose to define it before the existing row 257 as:

aggregate_start = index_start + len(load_fields or self.model._meta.fields)

comment:2 Changed 7 years ago by Alex Gaynor

Please upload a diff against the root of the source tree, rather than a snippet of code.

comment:3 Changed 7 years ago by Jacob

milestone: 1.2
Triage Stage: UnreviewedAccepted

comment:4 Changed 7 years ago by ruosteinen

Owner: changed from nobody to ruosteinen
Version: 1.1SVN

Select fields were getting garbled for only/defer fields as already discussed and index offset didn't take into accord the deferred fields.

Added a regression test as well. (Diffed on revision 12437.)

Changed 7 years ago by ruosteinen

comment:5 Changed 7 years ago by ruosteinen

Status: newassigned

comment:6 Changed 7 years ago by ruosteinen

Patch needs improvement: set

Noticed an extra error when using select_related as well. For models:

from django.db import models
class Person(models.Model):
    first_name = models.CharField(max_length=200)
    last_name = models.CharField(max_length=50)
    best_friend = models.ForeignKey('self', blank=True, null=True, editable=False)
        
    nick_name = models.CharField(max_length=50, blank=True, null=True)

class Candy(models.Model):
    owner = models.ForeignKey(Person, related_name="candies")
    qty = models.PositiveIntegerField(default=0)


from django.db.models import Sum;

Person.objects.all().delete()
Candy.objects.all().delete()
p = Person.objects.create(first_name='Mr', last_name='Egotistic')
p.best_friend = p
p.id = 1            # For determinism
p.save()
creations = [Candy(pk=n, owner_id=p.pk, qty=42).save() for n in xrange(10)]

assert Person.objects.annotate(Sum('candies__qty')).select_related('best_friend').defer('first_name','best_friend__nick_name').get(pk=p.pk).candies__qty__sum == 378

Indexing goes wrong in django.db.sql.compiler even when applying this patch. I'll work on it but in the meantime I'll add a diff for regression.

Changed 7 years ago by ruosteinen

Attachment: 11890_regression_tests.diff added

Regression test diff against trunk.

comment:7 Changed 7 years ago by Russell Keith-Magee

milestone: 1.21.3

Not critical for 1.2

comment:8 Changed 6 years ago by Ramiro Morales

#15523 reported the same problem with slightly different symptoms.

comment:9 Changed 6 years ago by milosu

Cc: milos.urbanek@… added

comment:10 Changed 5 years ago by Peter Baumgartner

Severity: Normal
Type: Bug

comment:11 Changed 5 years ago by Ramiro Morales

Easy pickings: unset
UI/UX: unset

See also #16409.

comment:12 Changed 5 years ago by Tai Lee

I've tried, but can't reproduce a problem with the select/extra tests in r16510. Reading the test case, I'm not sure it was valid in the first place. It creates 10 Candy objects with qty of 42, then tests that the Sum is 378 - that never would have been correct.

The issues in the original report have been fixed in [16522]. Although the symptoms changed (raising an IndexError instead of returning an empty queryset), the fix was the same. I believe the symptoms probably changed due to underlying changes in Django since this ticket was reported.

Changed 5 years ago by Tai Lee

Bogus test.

Changed 5 years ago by Tai Lee

Removed explicit PKs from test. Still bogus.

comment:13 Changed 5 years ago by Ramiro Morales

Resolution: fixed
Status: assignedclosed

I think we can safely close this as fixed in [16522]. Thanks ruosteinen and mrmachine.

comment:14 Changed 5 years ago by Jacob

milestone: 1.3

Milestone 1.3 deleted

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