Code

Opened 5 years ago

Last modified 17 months ago

#11104 new Bug

HAVING filter screws with extra() SQL parameter ordering

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

Description

>>> qs = Model.objects.extra(where=['foo>%s'], params=[2]).annotate(count=Count('bar')).filter(bar=99)
>>> qs.query.as_sql()
('SELECT `site_publication`.`id`, ..., FROM ... WHERE foo > %s GROUP BY ... HAVING bar > %s',
 (99, 2))

As you can see, the parameters (99 and 2) are in the wrong order. "foo", the first filter, should be evaluated against 2, and bar, the second one, against 99.

If "bar" is not an aggregate, it works, because apparently the filter would be placed in the WHERE clause *before* the "extra()" where.

Using rev. 10743.

Attachments (1)

django.db.models.sql.query.diff (804 bytes) - added by paluh 5 years ago.
changing grouping generation from values to keys of extra selects

Download all attachments as: .zip

Change History (12)

comment:1 Changed 5 years ago by russellm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

A related use case, reported in #11117:

Book.objects.extra(
    select={'date': "DATE_FORMAT(timestamp, '%s')"}, 
    select_params=('%%H',), 
    where=["DATE_FORMAT(timestamp, '%%H:%%i') > '%s'"],
    params=['22:00']).values('date').annotate(cnt=Count('id'))

the resulting sql query is:

SELECT DATE_FORMAT(timestamp, '%H') AS `date`, COUNT(id) AS `cnt` 
FROM `books`
WHERE DATE_FORMAT(timestamp, '%H:%i') > '%H' 
GROUP BY DATE_FORMAT(timestamp, '22:00') 
ORDER BY timestamp ASC

The placeholder values '%H' and '22:00' are interchanged, which is obviously a result of using the GROUP BY here on a user-defined variable with placeholder (--> 'date')

comment:2 Changed 5 years ago by mpaolini

I think all filters on annotated queryset should end up in HAVING clause, not in WHERE

imagine you want to do something like:

SELECT * FROM blog group by location, rating having count(id) > 10 OR rating >1 

right now (r11199) you can ony add HAVING clauses using if you filter using an aggregate. Does this make sense to you?

comment:3 Changed 5 years ago by Alex

#11916 was closed as a dupe of this, it has a patch.

Changed 5 years ago by paluh

changing grouping generation from values to keys of extra selects

comment:4 Changed 5 years ago by paluh

I think that I've found bug related to this ticket. When generating list for grouping values (not keys) are taken from extra selects.

--- django/db/models/sql/query.py	(revision 11729)
+++ django/db/models/sql/query.py	(working copy)
@@ -885,9 +885,9 @@
             group_by = self.group_by or []
 
             extra_selects = []
-            for extra_select, extra_params in self.extra_select.itervalues():
-                extra_selects.append(extra_select)
-                params.extend(extra_params)
+            for extra_select_key in self.extra_select.iterkeys():
+                extra_selects.append(extra_select_key)
+
             for col in group_by + self.related_select_cols + extra_selects:
                 if isinstance(col, (list, tuple)):
                     result.append('%s.%s' % (qn(col[0]), qn(col[1])))

comment:5 Changed 5 years ago by paluh

sorry for my previous post and attachment - it's related to #11916 which I've reopened.

comment:6 Changed 4 years ago by anonymous

  • Cc bas@… added

comment:7 Changed 4 years ago by shauncutts

  • Cc shaunc@… added

comment:8 Changed 3 years ago by julien

  • Severity set to Normal
  • Type set to Bug

comment:9 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:10 Changed 2 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:11 Changed 17 months ago by akaariai

  • Component changed from ORM aggregation to Database layer (models, ORM)

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from (none) to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


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

 
Note: See TracTickets for help on using tickets.