Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add_representative_country_and_main_mandate is suboptimal #19

Open
lauxley opened this issue Feb 19, 2016 · 5 comments
Open

add_representative_country_and_main_mandate is suboptimal #19

lauxley opened this issue Feb 19, 2016 · 5 comments

Comments

@lauxley
Copy link
Member

lauxley commented Feb 19, 2016

So i was looking for where representative.main_mandate came from and it led me to representatives.views.RepresentativeViewMixin.add_representative_country_and_main_mandate

and i think the same could be done with:

country = representative.mandates.filter(constituency__isnull=False, constituency__country__isnull=False)[0].constituency.country

main_mandate = representative.mandates.filter(end_date__gt=today, group__kind='group')[0]

And since it's not so time consuming any more cached properties on the model would be cool ?

@jpic
Copy link
Member

jpic commented Feb 20, 2016

Yeah we can do this, if that causes any additional query (i'm thinking about the list page this could cause N+N*2 queries ^^) tests should tell us ;)

@lauxley
Copy link
Member Author

lauxley commented Feb 23, 2016

It will be 2 queries fetching each one row instead of one big query and a loop over it, it may be slower in case there is a very limited amount of mandates but i think it will be a gain in most cases.
It will also make it possible to make those attribute lazy if we want to, and will avoid a monkey patch.
I'll try to benchmark it and make a PR after i'm done with the design stuff.

@jpic
Copy link
Member

jpic commented Feb 23, 2016

Performance optimization code could be prettier here I agree with that. At least it's pretty explicit what happens here ;)

That said, I would be suprised if replacing one query by several does a performance gain, particularely in the case of the list page, ie. with 96 objects displayed in the list, that would cause 193 queries, sounds like database butchering to me :D

@lauxley
Copy link
Member Author

lauxley commented Feb 23, 2016

Yeah i didn't took into consideration that the mandates are prefetched anyway so those queries would just add to the total. But 96 queries doesn't look too good either to be honest, and if i had to chose i'd rather make 192 instant queries than 96 big ones ?
Maybe this is a good case for denormalization ?

@jpic
Copy link
Member

jpic commented Feb 23, 2016

Exactly what I was thinking, ​either duplicate the logic, and just do the
right queryset in Detail view and and List view, and reuse that in API
views, either do denorm even though this implies perhaps some difficulties
doing bulk inserts and the like.

I don't know if duplicating the logic would be such a bad thing here, if
it's the simplest solution that works perhaps we should go for this: just
make one qs for detail, one for list ^^

@bram any opinion on this ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants