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

Fixed #28933 -- Improved the efficiency of ModelAdmin.date_hierarchy queries. #9469

Merged
merged 1 commit into from Feb 15, 2018

Conversation

Projects
None yet
5 participants
@hakib
Copy link
Contributor

hakib commented Dec 16, 2017

Ticket

The predicate generated by date_hierarchy makes it very difficult for databases to optimize the query.

The following date hierarchy:

/admin/app/model?created__year=2017&created__month=12&created__day=16

Will generate the following where clause (PostgreSql):

WHERE created between '2017-01-01' and '2017-31-12' and EXTRACT('month', created) = 12 and EXTRACT('day', created) = 16

The query above will not be able to utilize range based indexes on the date hierarchy column - on big tables this has a significant performance impact.

The current implementation of date hierarchy is relying on the "default" filtering mechinizem used by Django Admin. I propose implementing custom filtering for Django Admin that will better utilize it's hierarchical nature and make it more database "friendly".

Instead of the query above the date hierarchy would generate the following predicates for different levels of the heirarchy:

/admin/app/model?created__year=2017&created__month=12&created__day=16 
WHERE created >= '2017-12-16' and created < '2017-12-17'
/admin/app/model?created__year=2017&created__month=12
WHERE created >= '2017-12-01' and created < '2018-01-01'
/admin/app/model?created__year=2017
WHERE created >= '2017-01-01' and created < '2018-01-01'

Please let me know if this is acceptable.

@hakib hakib force-pushed the hakib:ticket_28933 branch from 8de9e10 to f51a467 Dec 16, 2017

@charettes
Copy link
Member

charettes left a comment

Thanks for taking the time to turn hakib/django-admin-lightweight-date-hierarchy into this PR, this is looking great!

One small concern would be that date_hierarchy template tag would likely benefit from this optimization as well

elif year_lookup and month_lookup:
days = cl.queryset.filter(**{year_field: year_lookup, month_field: month_lookup})
days = getattr(days, 'dates')(field_name, 'day')
return {
'show': True,
'back': {
'link': link({year_field: year_lookup}),
'title': str(year_lookup)
},
'choices': [{
'link': link({year_field: year_lookup, month_field: month_lookup, day_field: day.day}),
'title': capfirst(formats.date_format(day, 'MONTH_DAY_FORMAT'))
} for day in days]
}

to_date = from_date + datetime.timedelta(days=1)

elif month:
assert from_date.day == 1

This comment has been minimized.

@charettes

charettes Dec 16, 2017

Member

You can drop this assertion, the way from_date is constructed and that this branch is behind the if day one makes it impossible to reach.

int(day if day is not None else 1),
)
if settings.USE_TZ:
from_date = get_default_timezone().localize(from_date)

This comment has been minimized.

@charettes

charettes Dec 16, 2017

Member

Use django.utils.timezone.make_aware instead.

@hakib hakib force-pushed the hakib:ticket_28933 branch from f51a467 to 974e858 Dec 16, 2017

@hakib

This comment has been minimized.

Copy link
Contributor Author

hakib commented Dec 16, 2017

Thanks for the quick review @charettes.

Made all the changes you pointed out. The date_hierarchy template tag is actually simpler now - no need to apply the filter again, it's already handled by the changelist.

@hakib hakib force-pushed the hakib:ticket_28933 branch from 974e858 to ad87051 Dec 16, 2017

to_date = from_date.replace(year=from_date.year + 1)

lookup_params['{}__gte'.format(self.date_hierarchy)] = from_date
lookup_params['{}__lt'.format(self.date_hierarchy)] = to_date

This comment has been minimized.

@felixxm

felixxm Dec 16, 2017

Member

I think we should protect this filter against invalid inputs from users, e.g.:

{}__year=a
{}__year=0
{}__month=a
{}__month=0
{}__month=13
{}__day=a
{}__day=0
{}__day=32

Currently it throws ValueError in most cases.

This comment has been minimized.

@hakib

hakib Dec 16, 2017

Author Contributor

We can catch ValueError when from_date is created (few lones above). Catching ValueError will cover both invalid values ('x,'y','z') and invalid dates (month 13 etc...).

Would IncorrectLookupParameters be appropriate?

This comment has been minimized.

@carltongibson

carltongibson Feb 6, 2018

Member

There are tests for these invalid cases added at the end of the new test_should_filter_by_date_hierarchy method.

@@ -367,8 +367,7 @@ def link(filters):
'choices': [{'title': capfirst(formats.date_format(day, 'MONTH_DAY_FORMAT'))}]
}
elif year_lookup and month_lookup:
days = cl.queryset.filter(**{year_field: year_lookup, month_field: month_lookup})
days = getattr(days, 'dates')(field_name, 'day')
days = getattr(cl.queryset, 'dates')(field_name, 'day')

This comment has been minimized.

@charettes

charettes Dec 16, 2017

Member

Shouldn't we keep filtering the queryset in both cases but use the __gte/__lt approach instead? Were these filters superfluous the whole time?

This comment has been minimized.

@hakib

hakib Dec 16, 2017

Author Contributor

date_heirarchy oprerate on the filtered queryset so at the point the query is already filtered. TBH I wasn't sure why it was repplied in the first place.

Am I missing something?

This comment has been minimized.

@charettes

charettes Dec 16, 2017

Member

Am I missing something?

Probably not but we'd need to assert this works correctly and I'm not sure the current test coverage accounts for that. We maybe have some assertions against it in admin_views tests?

This comment has been minimized.

@charettes

charettes Dec 16, 2017

Member

In all cases, if the django/contrib/admin/templatetags/admin_list.py changes are not required to fix this PR then they should be moved to another commit which can live in this PR as well.

e.g. Refs #28933 -- Removed unnecessary filtering in date_hierarchy template tag.

This comment has been minimized.

@hakib

hakib Dec 16, 2017

Author Contributor

OK, seperated the commits.

The first commit also adds tests to date_heirarchy.

@hakib hakib force-pushed the hakib:ticket_28933 branch from ad87051 to 8980e07 Dec 16, 2017

@hakib

This comment has been minimized.

Copy link
Contributor Author

hakib commented Dec 16, 2017

OK, @felixxm - catching ValueError now and raising IncorrectLookupParameters.

@hakib hakib force-pushed the hakib:ticket_28933 branch from 8980e07 to 4e55f98 Dec 16, 2017

@carltongibson
Copy link
Member

carltongibson left a comment

This looks really good. Thanks for the effort @hakib!

There is only one existing test which explicitly covers `date_heirarchy:

@override_settings(TIME_ZONE='America/Sao_Paulo', USE_TZ=True)
def test_date_hierarchy_timezone_dst(self):
# This datetime doesn't exist in this timezone due to DST.
date = pytz.timezone('America/Sao_Paulo').localize(datetime.datetime(2016, 10, 16, 15), is_dst=None)
q = Question.objects.create(question='Why?', expires=date)
Answer2.objects.create(question=q, answer='Because.')
response = self.client.get(reverse('admin:admin_views_answer2_changelist'))
self.assertEqual(response.status_code, 200)
self.assertContains(response, 'question__expires__day=16')
self.assertContains(response, 'question__expires__month=10')
self.assertContains(response, 'question__expires__year=2016')

This is really targeting the DST handling. As such, the new tests are a real bonus.

This is a purely internal change so no new docs needed. (I think) — Is it worth something for the release notes though?

to_date = from_date.replace(year=from_date.year + 1)

lookup_params['{}__gte'.format(self.date_hierarchy)] = from_date
lookup_params['{}__lt'.format(self.date_hierarchy)] = to_date

This comment has been minimized.

@carltongibson

carltongibson Feb 6, 2018

Member

There are tests for these invalid cases added at the end of the new test_should_filter_by_date_hierarchy method.

def test_should_filter_by_date_hierarchy(self):
modeladmin = BookAdminWithDateHierarchy(Book, site)

def _test_date_hierarchy(query, expected_from_date, expected_to_date):

This comment has been minimized.

@carltongibson

carltongibson Feb 6, 2018

Member

Query: do we prefer these helper functions to be named like assertDateHierarchy?

This comment has been minimized.

@hakib

hakib Feb 6, 2018

Author Contributor

I followed the convntion used a few tests up.

I also think a name like assertDateHierarchy might imply it's avaliable as part of TestCase (such as other Django asserts like assertNumQueries).

@hakib

This comment has been minimized.

Copy link
Contributor Author

hakib commented Feb 6, 2018

This is a purely internal change so no new docs needed. (I think) — Is it worth something for the release notes though?

@carltongibson I think so to. I'm not sure where I should do that though (should this PR be assigned to a version before writing the release notes?).

Also, it's been a couple of month (and two Django releases) since this PR was submited - should I reabse on latest master?

Thanks.

@carltongibson

This comment has been minimized.

Copy link
Member

carltongibson commented Feb 6, 2018

@hakib. Yes. A rebase would be great!

For the release notes try: django/docs/releases/2.1.txt

@hakib hakib force-pushed the hakib:ticket_28933 branch 2 times, most recently from 871b217 to 7a8b537 Feb 6, 2018

@hakib

This comment has been minimized.

Copy link
Contributor Author

hakib commented Feb 6, 2018

Done.

@hakib hakib force-pushed the hakib:ticket_28933 branch from 7a8b537 to c337dc3 Feb 8, 2018

):
Book.objects.create(title='title', date_registered=date_registered)

def _test_date_hierarchy_choices(query, expected_choices):

This comment has been minimized.

@timgraham

timgraham Feb 13, 2018

Member

Instead of helper method like this, you can use a loop and subTest(). See f04e673 for an example.

@@ -1099,3 +1104,43 @@ def test_list_filter_queryset_filtered_by_default(self):
changelist = modeladmin.get_changelist_instance(request)
changelist.get_results(request)
self.assertEqual(changelist.full_result_count, 4)

def test_should_show_date_hierarchy_choices(self):

This comment has been minimized.

@timgraham

timgraham Feb 13, 2018

Member

The admin_filters app is for testing django.contrib.admin.templatetags.filters. It looks like this test could go in tests/admin_views/test_templatetags.py. The test passes on master without any changes in this PR, correct? Please send it as a separate pull request. (without the changes to admin_list.py) We don't like to mix additional test coverage with code changes since it gives the impression that behavior is changing as a result of those changes.

@timgraham timgraham force-pushed the hakib:ticket_28933 branch 2 times, most recently from c264062 to aa7724e Feb 14, 2018

@timgraham timgraham changed the title Fixed #28933 -- Custom range-based filter for date_hierarchy Fixed #28933 -- Improved the efficiency of ModelAdmin.date_hierarchy queries. Feb 14, 2018

@timgraham timgraham force-pushed the hakib:ticket_28933 branch 2 times, most recently from 349675c to 94d3a90 Feb 14, 2018

@timgraham timgraham force-pushed the hakib:ticket_28933 branch from 94d3a90 to ff55179 Feb 15, 2018

@timgraham timgraham merged commit ff55179 into django:master Feb 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment