Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#13274 closed (invalid)

Grouping in sql adds everything in extra select.

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

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

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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 Changed 5 years ago by MockSoul

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

comment:3 Changed 5 years ago by MockSoul

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 Changed 5 years ago by MockSoul

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

comment:5 Changed 5 years ago by MockSoul

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 Changed 5 years ago by kmtracey

  • Resolution set to invalid
  • Status changed from new to closed

"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 Changed 5 years ago by MockSoul

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