Code

Opened 5 years ago

Closed 3 years ago

Last modified 3 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@… 5 years ago.
django.db.models.query.py
11980_defer_plus_annotate_fix.diff (4.3 KB) - added by ruosteinen 4 years ago.
11890_regression_tests.diff (5.5 KB) - added by ruosteinen 4 years ago.
Regression test diff against trunk.
16409-defer-only-annotate-r16510.diff (2.7 KB) - added by mrmachine 3 years ago.
Bogus test.
11890-extra-select-related-r16522.diff (3.3 KB) - added by mrmachine 3 years ago.
Removed explicit PKs from test. Still bogus.

Download all attachments as: .zip

Change History (19)

Changed 5 years ago by jaklaassen@…

django.db.models.query.py

comment:1 Changed 5 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 5 years ago by Alex

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

comment:3 Changed 4 years ago by jacob

  • milestone set to 1.2
  • Triage Stage changed from Unreviewed to Accepted

comment:4 Changed 4 years ago by ruosteinen

  • Owner changed from nobody to ruosteinen
  • Version changed from 1.1 to SVN

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 4 years ago by ruosteinen

comment:5 Changed 4 years ago by ruosteinen

  • Status changed from new to assigned

comment:6 Changed 4 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 4 years ago by ruosteinen

Regression test diff against trunk.

comment:7 Changed 4 years ago by russellm

  • milestone changed from 1.2 to 1.3

Not critical for 1.2

comment:8 Changed 3 years ago by ramiro

#15523 reported the same problem with slightly different symptoms.

comment:9 Changed 3 years ago by milosu

  • Cc milos.urbanek@… added

comment:10 Changed 3 years ago by baumer1122

  • Severity set to Normal
  • Type set to Bug

comment:11 Changed 3 years ago by ramiro

  • Easy pickings unset
  • UI/UX unset

See also #16409.

comment:12 Changed 3 years ago by mrmachine

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 3 years ago by mrmachine

Bogus test.

Changed 3 years ago by mrmachine

Removed explicit PKs from test. Still bogus.

comment:13 Changed 3 years ago by ramiro

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

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

comment:14 Changed 3 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.