Opened 15 years ago

Closed 13 years ago

Last modified 12 years ago

#11890 closed Bug (fixed)

Defer/only + annotate is broken

Reported by: jaklaassen@… Owned by: ruosteinen
Component: Database layer (models, ORM) Version: dev
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@… 15 years ago.
django.db.models.query.py
11980_defer_plus_annotate_fix.diff (4.3 KB ) - added by ruosteinen 14 years ago.
11890_regression_tests.diff (5.5 KB ) - added by ruosteinen 14 years ago.
Regression test diff against trunk.
16409-defer-only-annotate-r16510.diff (2.7 KB ) - added by Tai Lee 13 years ago.
Bogus test.
11890-extra-select-related-r16522.diff (3.3 KB ) - added by Tai Lee 13 years ago.
Removed explicit PKs from test. Still bogus.

Download all attachments as: .zip

Change History (19)

by jaklaassen@…, 15 years ago

Attachment: query.py added

django.db.models.query.py

comment:1 by jaklaassen@…, 15 years ago

Has patch: set

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 by Alex Gaynor, 15 years ago

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

comment:3 by Jacob, 14 years ago

milestone: 1.2
Triage Stage: UnreviewedAccepted

comment:4 by ruosteinen, 14 years ago

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.)

by ruosteinen, 14 years ago

comment:5 by ruosteinen, 14 years ago

Status: newassigned

comment:6 by ruosteinen, 14 years ago

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.

by ruosteinen, 14 years ago

Attachment: 11890_regression_tests.diff added

Regression test diff against trunk.

comment:7 by Russell Keith-Magee, 14 years ago

milestone: 1.21.3

Not critical for 1.2

comment:8 by Ramiro Morales, 13 years ago

#15523 reported the same problem with slightly different symptoms.

comment:9 by milosu, 13 years ago

Cc: milos.urbanek@… added

comment:10 by Peter Baumgartner, 13 years ago

Severity: Normal
Type: Bug

comment:11 by Ramiro Morales, 13 years ago

Easy pickings: unset
UI/UX: unset

See also #16409.

comment:12 by Tai Lee, 13 years ago

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.

by Tai Lee, 13 years ago

Bogus test.

by Tai Lee, 13 years ago

Removed explicit PKs from test. Still bogus.

comment:13 by Ramiro Morales, 13 years ago

Resolution: fixed
Status: assignedclosed

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

comment:14 by Jacob, 12 years ago

milestone: 1.3

Milestone 1.3 deleted

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