Refactor year, month, day lookups?

[Jon Dufresne]

Hi,

I have been spending some time learning and investigating the custom
lookups feature that was newly introduced in 1.7 [0]. While
investigating, I wanted to learn by example so I started looking
through the Django code. In the end, I didn't find many examples.
However, I did notice that there exists lookups for the year, month,
day, (and more) value from a database date or datetime value [1].
These appear to be implemented as special case "builtin" lookups and
not through the new lookup mechanism. Is there a specific reason for
this or is it historical momentum?

I started investigating if these could be refactored to use the common
code path and implemented using the new lookup mechanism. To my
delight it was not very difficult and I now have all tests passing
after refactoring these lookups. Right now, this lives in a branch of
mine and not in a ticket or pull request. The WIP branch is located
at: <https://github.com/jdufresne/django/tree/date-register-lookup>

Would this be something welcome as a ticket and pull request? While
there is no outward change in functionality, I see it as a beneficial
refactoring because with this change:

1. The year, month, day, etc lookups are no longer treated as special
cases, but instead use the common code path.
2. There now exists complete and useful examples of registering new
lookups in the Django code itself. This might help others build more
lookups.
3. The lookups are now limited to the correct Field types where as
previously this was not true. I demonstrate this with a unit test.

If this looks like it could be a welcome change I will can go forward
with a typical ticket and pull request.

Cheers,
Jon

[0] https://docs.djangoproject.com/en/dev/howto/custom-lookups/
[1] https://docs.djangoproject.com/en/dev/ref/models/querysets/#year

[Josh Smeaton]

Hi,

Firstly (and least importantly) opening a PR makes it a lot easier to review code, especially when there are lots of commits. A [WIP] pull request is common and useful. If you get a chance, you should open one with this change.

I think it's a good idea. So much so that I opened a ticket about a year ago: https://code.djangoproject.com/ticket/22394. You'll note some comments there about retaining the Year based behaviour as a `BETWEEN X and Y` rather than `Extract(YEAR)`. Otherwise, I think the support is rather positive. At a high level, your code looks fairly solid and I think would be a useful addition.

Another thing I would really like to see is transform based annotations. I'm not 100% sure on whether .annotate(F('X__transform')) is supported or not, but if it is, we'll get some really nice behaviour from the use of transforms.

Think:

sales_per_month = Model.objects.annotate(month=F('mydate__month')).values('month').Aggregate(sales=Sum('sale'))

If Transforms don't yet work with annotate, that'll probably be what I'd like to implement next. But the first step is having transforms to work with, where date based transforms are (to me) the most useful.

Cheers,

[Anssi Kääriäinen]

+1 for implementing these. They weren't part of the original patch because the patch was a large one already without these.

[Alex Hill]

Hi Josh,

With a suitably-defined Month function your example could be:

sales_per_month = Model.objects.annotate(month=Month(F('mydate'))).values('month').Aggregate(sales=Sum('sale'))

ISTM there's a bit of overlap between Func and Transform that could be cleared up - I'll start a new thread about it.

Cheers,

Alex

[Jon Dufresne]

On Tue, Mar 24, 2015 at 9:24 PM, Josh Smeaton <josh.s...@gmail.com> wrote:
> Hi,
>
> Firstly (and least importantly) opening a PR makes it a lot easier to review
> code, especially when there are lots of commits. A [WIP] pull request is
> common and useful. If you get a chance, you should open one with this
> change.
>
> I think it's a good idea. So much so that I opened a ticket about a year
> ago: https://code.djangoproject.com/ticket/22394. You'll note some comments
> there about retaining the Year based behaviour as a `BETWEEN X and Y` rather
> than `Extract(YEAR)`. Otherwise, I think the support is rather positive. At
> a high level, your code looks fairly solid and I think would be a useful
> addition.

Thank you for pointing me to this. I have added a PR to that ticket.
Future review and discussion of the changes can continue in the ticket
and PR.

> Another thing I would really like to see is transform based annotations. I'm
> not 100% sure on whether .annotate(F('X__transform')) is supported or not,
> but if it is, we'll get some really nice behaviour from the use of
> transforms.

AFAICT this does not work. Both before and after my change doing:

Article.objects.annotate(month=F('pub_date__month'))

Yields:
---
File "/home/jon/devel/django/tests/lookup/tests.py", line 257, in
test_values_with_month_lookup
values = Article.objects.values('pub_date__month')
File "/home/jon/devel/django/django/db/models/manager.py", line 127,
in manager_method
return getattr(self.get_queryset(), name)(*args, **kwargs)
File "/home/jon/devel/django/django/db/models/query.py", line 703, in values
clone = self._values(*fields)
File "/home/jon/devel/django/django/db/models/query.py", line 698, in _values
query.add_fields(field_names, True)
File "/home/jon/devel/django/django/db/models/sql/query.py", line
1603, in add_fields
name.split(LOOKUP_SEP), opts, alias, allow_many=allow_m2m)
File "/home/jon/devel/django/django/db/models/sql/query.py", line
1377, in setup_joins
names, opts, allow_many, fail_on_missing=True)
File "/home/jon/devel/django/django/db/models/sql/query.py", line
1345, in names_to_path
" not permitted." % (names[pos + 1], name))
FieldError: Cannot resolve keyword u'month' into field. Join on
'pub_date' not permitted.
---

Just to be clear, are you suggesting support for this be a part of my
change? Personally, I see this feature as orthogonal to the transform
refactoring. Seeing as it doesn't work with the built-in lookups nor
transforms there should be little BC concerns.

[Josh Smeaton]

Just to be clear, are you suggesting support for this be a part of my 
change? Personally, I see this feature as orthogonal to the transform 
refactoring. Seeing as it doesn't work with the built-in lookups nor 
transforms there should be little BC concerns. 

No, I was just wondering if it worked already. Since it doesn't, that'll be something I look to implement after your patch is ready.

Cheers 

[Anssi Kääriäinen]

There remains a bit of work to do to make transforms usable everywhere in the ORM. The example should work with just .values('pub_date__month').annotate(...)

[Alexander Hill]

In the dicts produced by a values query like that, what key would the result be found under? The full lookup string i.e. 'pub_date__month'?

On Thu, Mar 26, 2015 at 2:36 AM, Anssi Kääriäinen <akaa...@gmail.com> wrote:

There remains a bit of work to do to make transforms usable everywhere in the ORM. The example should work with just .values('pub_date__month').annotate(...)

--
You received this message because you are subscribed to a topic in the Google Groups "Django developers  (Contributions to Django itself)" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/django-developers/WYWrQkBJ2hs/unsubscribe.
To unsubscribe from this group and all its topics, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/1b62a33e-f0b6-4153-bbc3-2e48ebdb7ac7%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[Anssi Kääriäinen]

Maybe .values(month='pub_date__month') could work? Otherwise the month would be the full lookup string.

You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.

To post to this group, send email to django-d...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CA%2BKBOKx%2BOYZu%3D9OSKRFMgzgHvQV%2B54yrhKC86jF%2BT9%2B8Mi2e%2Bw%40mail.gmail.com.

[Michał Modzelewski]

I just recently needed to do date based aggregation at work, and I discovered that this functionality already exists in Django 1.8 after the query expression refactoring, but is undocumented. Your example can be written like this:

from django.db.models import Sum
from django.db.models.expressions import Date, DateTime
from django.utils.timezone import get_current_timezone

sales_per_month = Model.objects.annotate(month=Date('mydate', 'month'))\
    .values('month')\
    .annotate(sales=Sum('sale'))\
    .order_by('month')
# or
sales_per_month = Model.objects.annotate(month=DateTime('mydate', 'month', get_current_timezone()))\
    .values('month')\
    .annotate(sales=Sum('sale'))\
    .order_by('month')

I was thinking of writing some documentation for this so that users of Django 1.8 know they can use it and how. An obvious problem however, is that there is an error message in DateTime that refers specifically to it's usage in QuerySet.datetimes. Maybe this could be treated as a bug, and a patch for the error message could be backported?

This would also close https://code.djangoproject.com/ticket/10302

For 1.9 Date and DateTime could become importable from django.db.models like all the other expression classes.

[Josh Smeaton]

I'd prefer users not to use the existing Date and DateTime expressions directly. They really are internal, and were moved from elsewhere. 

Small wrapper expressions could use them internally though. That's what I'd prefer to see I think. Thoughts?

[Paulo Maciel]

+1