Re: Refactor year, month, day lookups?

[Paulo Maciel]

+1

Em quarta-feira, 25 de março de 2015 01:24:44 UTC-3, Josh Smeaton escreveu:
>
> 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,
>
> On Wednesday, 25 March 2015 13:39:39 UTC+11, Jon Dufresne wrote:
>>
>> 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:  
>>
>> 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 
>>
>

-- 
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-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/d85a3625-c0c6-4001-8c4e-fea17715cedf%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: Refactor year, month, day lookups?

[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?

On Sunday, 3 May 2015 01:53:21 UTC+10, Michał Modzelewski wrote:
>
> 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.
>
>
> On Wednesday, March 25, 2015 at 5:24:44 AM UTC+1, Josh Smeaton 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.
>>
>> 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,
>>
>> On Wednesday, 25 March 2015 13:39:39 UTC+11, Jon Dufresne wrote:
>>>
>>> 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:  
>>>
>>> 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] 

Re: Refactor year, month, day lookups?

[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.


On Wednesday, March 25, 2015 at 5:24:44 AM UTC+1, Josh Smeaton 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.
>
> 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,
>
> On Wednesday, 25 March 2015 13:39:39 UTC+11, Jon Dufresne wrote:
>>
>> 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:  
>>
>> 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 
>>
>

-- 
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-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at 

Re: Refactor year, month, day lookups?

[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 
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-developers+unsubscr...@googlegroups.com.
> To post to this group, send email to django-developers@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.
>

-- 
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-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@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.
For more options, visit https://groups.google.com/d/optout.

Re: Refactor year, month, day lookups?

[Anssi Kääriäinen]

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

On Wednesday, March 25, 2015, Alexander Hill  wrote:

> 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  > 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-developers+unsubscr...@googlegroups.com
>> 
>> .
>> To post to this group, send email to django-developers@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.
>>
>
>  --
> 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-developers+unsubscr...@googlegroups.com
> 
> .
> To post to this group, send email to django-developers@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
> 
> .
> For more options, visit https://groups.google.com/d/optout.
>

-- 
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-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@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/CALMtK1H4U3RGJCYkUMGh-krFfXe8e97xM0vvMOyiziV%3D%2ButtVQ%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

Re: Refactor year, month, day lookups?

[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(...)

-- 
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-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@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.

Re: Refactor year, month, day lookups?

[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 

On Wednesday, 25 March 2015 23:36:11 UTC+11, Jon Dufresne wrote:
>
> On Tue, Mar 24, 2015 at 9:24 PM, Josh Smeaton  > 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. 
>

-- 
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-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@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/6362d673-617b-461f-9c77-b6bbf3e0b90d%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: Refactor year, month, day lookups?

[Jon Dufresne]

On Tue, Mar 24, 2015 at 9:24 PM, Josh Smeaton  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.

-- 
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-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@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/CADhq2b6Z9P-XH487MKgJ_Sfm_TnMGry-5-WAg6pq33580we5cg%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

Re: Refactor year, month, day lookups?

[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

On Wednesday, March 25, 2015 at 12:24:44 PM UTC+8, Josh Smeaton 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.
>
> 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,
>
> On Wednesday, 25 March 2015 13:39:39 UTC+11, Jon Dufresne wrote:
>>
>> 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:  
>>
>> 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 
>>
>

-- 
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-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@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/b120711a-575e-4ecf-a1a3-ca79e2e1d0c4%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: Refactor year, month, day lookups?

[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.

-- 
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-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@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/4e98e7d1-f4d5-4c5d-af11-cb787edb407a%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: Refactor year, month, day lookups?

[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,

On Wednesday, 25 March 2015 13:39:39 UTC+11, Jon Dufresne wrote:
>
> 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:  
>
> 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 
>

-- 
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-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@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/6b07e38a-d45a-4f48-9702-d185aa512b74%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

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: 

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

-- 
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-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@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/CADhq2b4C4y5ZUHvNEugizHrqjL4EWoQJQcc8nne%2BjxsmU-L3uA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.