Opened 14 years ago

Closed 14 years ago

Last modified 14 years ago

#13274 closed (invalid)

Grouping in sql adds everything in extra select.

Reported by: Vadim Fint Owned by: nobody
Component: Database layer (models, ORM) Version: dev
Severity: Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Why so?

E.g.

# (we are in app asdapp)
class Asd(Model):
   pass

class Other(Model):
  asd = models.ForeignKey(Asd)

qs = Asd.objects.extra(select={'super_extra': 'SELECT count(*) from asdapp.other WHERE asdapp_other.asd_id = asdapp_asd.id'})
# normal extra, isnt it?
# but if we add some groupping, all fails
qs.query.group_by = [('asdapp', 'id')]
print qs # error -- invalid query
# query will have stange groupping: something like "(...) GROUP BY asdapp.id, SELECT COUNT(*) from asdapp.other WHERE asdapp_other.asd_id = asdapp_asd.id"

This is because code in compiler.get_groupping() just adds everything (!) in extra to groupping:

#django/django/db/models/sql/compiler.py
475                 extra_selects = []
476                 for extra_select, extra_params in self.query.extra_select.itervalues():
477                     extra_selects.append(extra_select)
478                     params.extend(extra_params)
479                 for col in group_by + self.query.related_select_cols + extra_selects:
480                     if isinstance(col, (list, tuple)):
481                         result.append('%s.%s' % (qn(col[0]), qn(col[1])))
482                     elif hasattr(col, 'as_sql'):
483                         result.append(col.as_sql(qn))
484                     else:
485                         result.append('(%s)' % str(col))

Why the hell django explicity tries to add extra cols to groupping? IMHO, this only should be done by developer itself (if he needs that behaviour).

Extra is usually a subquery. There is no much reason to make extra(select={'col_alias': 'real_col'}) (this way grouping will work). Maybe somebody wanted an iterkeys() against query.extra_select in line 476? :)

Change History (7)

comment:1 by Alex Gaynor, 14 years ago

AFAIK the SQL standard (and most SQL DB implementations) require that any fields in the SELECT clause also be in the GROUP BY clause (the notable exception being MySQL).

comment:2 by Vadim Fint, 14 years ago

Yes, but column alias must appear, not column itself!

comment:3 by Vadim Fint, 14 years ago

E.g.

# Ok, no grouping at all
select id, (select count(*) from bookmarks_taskbookmark tb where tb.task_id = tasks_task.id) as books from tasks_task

# Obviously NOT ok
select id, (select count(*) from bookmarks_taskbookmark tb where tb.task_id = tasks_task.id) as books from tasks_task GROUP BY (select count(*)...)

# Ok in MySQL, but not ok in other (proper) dbms (e.g. in PostgreSQL). id col must be in group by clause too
select id, (select count(*) from bookmarks_taskbookmark tb where tb.task_id = tasks_task.id) as books from tasks_task GROUP BY books

# Proper variant
select id, (select count(*) from bookmarks_taskbookmark tb where tb.task_id = tasks_task.id) as books from tasks_task GROUP BY id, books

comment:4 by Vadim Fint, 14 years ago

Component: UncategorizedDatabase layer (models, ORM)

comment:5 by Vadim Fint, 14 years ago

You can safely put col alias into group clause, because, even in PostgreSQL, both ways are ok:

select id as id_alias from tasks_task group by id;
select id as id_alias from tasks_task group by id_alias;

comment:6 by Karen Tracey, 14 years ago

Resolution: invalid
Status: newclosed

"Why" questions and design discussion don't belong in the ticket tracker, the django-users group would be a better starting place. Alternatively if you search the tracker for alias and "group by" you would pretty quickly find:

http://code.djangoproject.com/ticket/11916#comment:13

which gives the reason why the Django code is written as it currently is. If you have a specific suggestion for improvement that works on all supported databases, feel free to open a ticket for it. If you want to know why the code is the way it is, some investigation of the tracker/code history or a question on django-users is preferable to opening a ticket.

comment:7 by Vadim Fint, 14 years ago

ok, sorry for that. I saw that ticket, but I thought it does not fix problem. Seems to work fine, but it produces such weird messed up sql... huh..

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