keredson / peewee

a small, expressive orm -- supports postgresql, mysql and sqlite
http://docs.peewee-orm.com/
MIT License
13 stars 4 forks source link

counts aren't being returned #15

Closed keredson closed 7 years ago

keredson commented 7 years ago

example:

list(PageVisit.select(PageVisit.page, pw.fn.Count(PageVisit.page)).where(PageVisit.user==2). \
                  group_by(PageVisit.page).dicts())

now returns:

[{'page': 5L}, {'page': 2L}, {'page': 1L}, {'page': 20L}, {'page': 3L}, {'page': 19L}, {'page': 1L}, {'page': 1L}, {'page': 1L}, {'page': 4L}, {'page': 5L}, {'page': 7L}, {'page': 1L}, {'page': 17L}, {'page': 3L}, {'page': 1L}, {'page': 1L}, {'page': 1L}, {'page': 8L}, {'page': 7L}, {'page': 1L}, {'page': 1L}, {'page': 30L}, {'page': 5L}, {'page': 1L}, {'page': 1L}, {'page': 2L}, {'page': 4L}, {'page': 9L}, {'page': 1L}, {'page': 1L}, {'page': 29L}, {'page': 1L}, {'page': 18L}, {'page': 6L}, {'page': 1L}, {'page': 4L}, {'page': 1L}, {'page': 7L}, {'page': 1L}, {'page': 2L}, {'page': 4L}, {'page': 2L}, {'page': 3L}, {'page': 1L}, {'page': 10L}, {'page': 12L}, {'page': 4L}, {'page': 2L}, {'page': 1L}, {'page': 2L}, {'page': 1L}, {'page': 1L}, {'page': 1L}, {'page': 5L}, {'page': 11L}, {'page': 1L}, {'page': 1L}, {'page': 2L}, {'page': 2L}, {'page': 3L}, {'page': 3L}, {'page': 1L}, {'page': 4L}, {'page': 1L}, {'page': 53L}, {'page': 3L}, {'page': 3L}, {'page': 22L}, {'page': 1L}, {'page': 1L}, {'page': 42L}, {'page': 1L}, {'page': 2L}, {'page': 2L}, {'page': 38L}, {'page': 7L}, {'page': 3L}, {'page': 1L}, {'page': 106L}, {'page': 6L}, {'page': 3L}, {'page': 8L}, {'page': 6L}, {'page': 23L}, {'page': 1L}, {'page': 1L}, {'page': 1L}, {'page': 1L}, {'page': 1L}, {'page': 3L}, {'page': 22L}, {'page': 5L}, {'page': 3L}, {'page': 7L}, {'page': 1L}, {'page': 1L}, {'page': 3L}, {'page': 22L}, {'page': 2L}, {'page': 6L}, {'page': 1L}, {'page': 3L}, {'page': 2L}, {'page': 1L}, {'page': 2L}, {'page': 1L}, {'page': 1L}, {'page': 2L}, {'page': 1L}, {'page': 1L}, {'page': 6L}, {'page': 3L}, {'page': 2L}, {'page': 1L}, {'page': 1L}, {'page': 1L}, {'page': 1L}, {'page': 4L}, {'page': 5L}, {'page': 1L}, {'page': 2L}, {'page': 2L}]

note the lack of a count field. same if you look at the model, etc...

@brenguyen711

edit the count field is not missing, the 2L is the count, but w/ the wrong key. the page id is actually what is missing.

keredson commented 7 years ago

break occurred upstream between 2.8.3 and 2.8.4. 2.8.4 changelog mentions:

As a miscellaneous note, I did some major refactoring and cleanup in ExtQueryResultsWrapper and it's corollary in the speedups module. The code is much easier to read than before.

isolated test case:

import logging, os, sys
import peewee as pw

logger = logging.getLogger('peewee')
logger.setLevel(logging.DEBUG)
logger.addHandler(logging.StreamHandler())

os.system('dropdb test3')
os.system('createdb test3')
db = pw.PostgresqlDatabase('test3')

class Page(pw.Model):
  name = pw.CharField(default='Page')
  class Meta:
    database = db

class PageVisit(pw.Model):
  page = pw.ForeignKeyField(rel_model=Page)
  class Meta:
    database = db

db.create_tables([Page, PageVisit])

page = Page.create()
PageVisit.create(page=page)
PageVisit.create(page=page)

print list(PageVisit.select(PageVisit.page, pw.fn.Count(PageVisit.page)).group_by(PageVisit.page).dicts())
keredson commented 7 years ago

so the root is this rewrite of ExtQueryResultsWrapper.

specifically this line:

self.column_names.append(node._alias or arg._alias or arg.name)

in the previous version of this class, it used the name of the column returned by the database (count in our case) as the name. in this version, if the function doesn't have an alias, it uses the name of the first argument in the function.

in addition to breaking backward compat, this seems really wrong and highly likely to result in collisions. we often do queries like select fk_id, count(fk_id) from xxx group by fk_id that would break by this. but what should the behavior be? (i could see an argument for this too, if my function is LOWERCASE() or something, but it still seems surprising.)

i don't like the original behavior either. the resultant dict should not be database dependent. (postgres returns count, but mysql returns the full function def.) so i think i'm settling on it should just return the function name. (not the arg name) plus that's a low impact change.

open to other opinions tho.

keredson commented 7 years ago

see https://github.com/keredson/peewee/commit/326eaa9c6ed6e817ffc3196e28e428b3730fe7be