Opened 16 years ago
Closed 13 years ago
#12823 closed Bug (fixed)
Wrong SQL query when using .exclude()
| Reported by: | olepw | Owned by: | |
|---|---|---|---|
| Component: | Database layer (models, ORM) | Version: | 1.1 | 
| Severity: | Normal | Keywords: | |
| Cc: | philippe@…, hvdklauw@…, sebastian.goll@… | Triage Stage: | Accepted | 
| Has patch: | yes | Needs documentation: | no | 
| Needs tests: | no | Patch needs improvement: | no | 
| Easy pickings: | no | UI/UX: | no | 
Description
Given the following model:
from django.db import models
class Book(models.Model):
  title = models.TextField()
  chapter = models.ForeignKey('Chapter')
class Chapter(models.Model):
  title = models.TextField()
  paragraph = models.ForeignKey('Paragraph')
class Paragraph(models.Model):
  text = models.TextField()
  page = models.ManyToManyField('Page')
  
class Page(models.Model):
  text = models.TextField()
Create the following query:
q = Book.objects.exclude(chapter__paragraph__page__text='foo')
From this I would expect to get all books that does not contain the text 'foo'
print q.query
This produces:
SELECT `book_book`.`id`, `book_book`.`title`, `book_book`.`chapter_id` FROM `book_book` INNER JOIN `book_chapter` ON (`book_book`.`chapter_id` = `book_chapter`.`id`) WHERE NOT (`book_chapter`.`paragraph_id` IN (SELECT U1.`id` FROM `book_chapter` U1 INNER JOIN `book_paragraph` U2 ON (U1.`paragraph_id` = U2.`id`) INNER JOIN `book_paragraph_page` U3 ON (U2.`id` = U3.`paragraph_id`) INNER JOIN `book_page` U4 ON (U3.`page_id` = U4.`id`) WHERE U4.`text` = foo ) AND `book_chapter`.`paragraph_id` IS NOT NULL)
The error here is:
WHERE NOT (`book_chapter`.`paragraph_id` IN (SELECT U1.`id` FROM `book_chapter` U1
Where django tries to compare book_chapter.paragraph_id with book_chapter.id, two fields from the same table.
I would expect the query to be:
WHERE NOT (`book_book`.`chapter_id` IN (SELECT U1.`id` FROM `book_chapter` U1
Attachments (6)
Change History (28)
comment:1 by , 16 years ago
| Triage Stage: | Unreviewed → Accepted | 
|---|
comment:2 by , 15 years ago
comment:3 by , 15 years ago
| Has patch: | set | 
|---|
A quick note on the tests: I had to modify some ids to make the bug show because it basically boils down to comparing one model's PK against a PK value from another model. When you're testing with hand-created models whose PKs are all 1,2,3... it's easy to miss. 
comment:4 by , 15 years ago
| Patch needs improvement: | set | 
|---|
The patch should be against SVN trunk. Version 1.1 is in security-fixes-only mode.
comment:5 by , 15 years ago
| Patch needs improvement: | unset | 
|---|
Sorry I was in the process of porting the tests to trunk. My production app uses 1.1 so that's what I used to work. 
comment:6 by , 15 years ago
| Cc: | added | 
|---|
comment:7 by , 15 years ago
| milestone: | → 1.3 | 
|---|
I can confirm this bug.
Don't look at the models, the example is horrible and makes no sense in relation to real books ;-)
Setting this as 1.3 milestone.
comment:8 by , 15 years ago
Resuling SQL from the patch applied to Django 1.2.3 (with books as appname instead of book):
SELECT "books_book"."id", "books_book"."title", "books_book"."chapter_id" FROM "books_book" INNER JOIN "books_chapter" ON ("books_book"."chapter_id" = "books_chapter"."id") WHERE NOT (("books_book"."chapter_id" IN (SELECT U1."id" FROM "books_chapter" U1 INNER JOIN "books_paragraph" U2 ON (U1."paragraph_id" = U2."id") INNER JOIN "books_paragraph_page" U3 ON (U2."id" = U3."paragraph_id") INNER JOIN "books_page" U4 ON (U3."page_id" = U4."id") WHERE U4."text" = foo ) AND "books_chapter"."paragraph_id" IS NOT NULL))
I'm a bit unsure about the JOIN on the "books_chapter" in the main query which then isn't used at all.
comment:9 by , 15 years ago
| Cc: | added | 
|---|
comment:10 by , 15 years ago
I think I understand the problem: we need to unref the corresponding alias in the query so the extra unused join isn't generated. 
comment:11 by , 15 years ago
After giving it some thought I realized the join is actually used by the AND "books_chapter"."paragraph_id" IS NOT NULL)) clause. It looks like this is by design, but then maybe we could optimize the inner query to use the chapter table directly instead of joining it again ? 
comment:12 by , 15 years ago
| Type: | → Bug | 
|---|
comment:13 by , 15 years ago
| Severity: | → Normal | 
|---|
comment:14 by , 15 years ago
| Easy pickings: | unset | 
|---|---|
| Patch needs improvement: | set | 
12823_trunk.diff fails to apply cleanly on to trunk
by , 14 years ago
| Attachment: | 12823_1.2.diff added | 
|---|
comment:15 by , 14 years ago
| Patch needs improvement: | unset | 
|---|---|
| UI/UX: | unset | 
Disregard previous patches, but I can't remove them because I used a different account...
comment:17 by , 14 years ago
| Cc: | added | 
|---|
comment:18 by , 14 years ago
| Owner: | changed from to | 
|---|
comment:19 by , 13 years ago
| Status: | new → assigned | 
|---|
comment:20 by , 13 years ago
| Owner: | removed | 
|---|---|
| Status: | assigned → new | 
comment:21 by , 13 years ago
This is fixed in current master. The generated query is:
SELECT "queries_book"."id", "queries_book"."title", "queries_book"."chapter_id"
  FROM "queries_book"
 INNER JOIN "queries_chapter" ON ("queries_book"."chapter_id" = "queries_chapter"."id")
 WHERE NOT ("queries_chapter"."paragraph_id" IN
            (SELECT U3."paragraph_id"
               FROM "queries_paragraph_page" U3
              INNER JOIN "queries_page" U4 ON (U3."page_id" = U4."id")
 WHERE U4."text" = pg1 ))
I will also add a little patch to sql/query.py which will get rid of one extra IS NOT NULL check.
comment:22 by , 13 years ago
| Owner: | set to | 
|---|---|
| Resolution: | → fixed | 
| Status: | new → closed | 
by , 12 years ago
| Attachment: | 12823_1.5.diff added | 
|---|
updated patch for django 1.5.5 (bug not fixed in this release)
This may or may not be a dup of #11052