#25774 closed New feature (fixed)
Refactor of datetime expressions and better, official support for right-hand-side date part extraction
| Reported by: | David Filipovic | Owned by: | Josh Smeaton |
|---|---|---|---|
| Component: | Database layer (models, ORM) | Version: | dev |
| Severity: | Normal | Keywords: | db, expressions, date, time, extract, transform 1.10 |
| Cc: | josh.smeaton@…, info+coding@…, aksheshdoshi@… | Triage Stage: | Ready for checkin |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description (last modified by )
As previously stated in this ticket and this thread, I have begun work on the following:
- making the so called datetime transforms (currently residing in
django.db.models.lookups) part of the officially supported ORM API as part of the db expressions, - making these transforms easily usable on the right hand side of lookups / Q objects
To this effect, I propose the following:
- replace module
django.db.models.expressionswith a package of the same name - add module
django.db.models.expressions.datetimes - rename
XTransformtoXExtractand move them fromdjango.db.models.lookupstodjango.db.models.lookupstodjango.db.models.expressions.datetimes - change
YearLookupandYearComparisonLookupto allow for functions on the right hand side
- extensive testing of these db expressions
- adding this to the release notes
- any other change that might support this
I'm also adding a PR with some of these already implemented. What remains to be done is:
- replace references
XTransformtype classes indjango.db.models.lookupswithXExtractclasses fromdjango.db.models.expressions.datetimes - replace these references anywhere else in the project (I've noticed there are some in the tests, for instance)
With the API in the PR it becomes possible to do the following lookup:
from django.db.models.expressions.datetimes import DateExtract
Person.objects.filter(birth_date__year=DateExtract('job__start_date', lookup_name='year'))
which is equivalent to:
from django.db.models.expressions.datetimes import YearExtract
Person.objects.filter(birth_date__year=YearExtract('job__start_date'))
Additionally, @jarshwah suggested that these should maybe be named: ExtractX instead of XExtract.
Change History (19)
comment:1 by , 10 years ago
| Description: | modified (diff) |
|---|---|
| Needs documentation: | set |
| Needs tests: | set |
comment:2 by , 10 years ago
| Triage Stage: | Unreviewed → Accepted |
|---|
comment:3 by , 10 years ago
Josh, I've actually tweaked YearLookup and YearComparisonLookup already.
I've reversed the order of process_lhs and process_rhs to make process_rhs occur first, and then based on the result of that processing, specifically, based on whether there are any rhs_params returned, the lhs processing branches to accommodate right hand side based on both a parameter and a function.
comment:4 by , 10 years ago
| Cc: | added |
|---|
comment:5 by , 10 years ago
| Owner: | changed from to |
|---|---|
| Status: | new → assigned |
Since David appears to have left this ticket I'll pick this up. Initial patch: https://github.com/django/django/pull/6243
comment:6 by , 10 years ago
| Needs documentation: | unset |
|---|---|
| Needs tests: | unset |
| Triage Stage: | Accepted → Ready for checkin |
Patch is ready for review.
comment:7 by , 10 years ago
| Triage Stage: | Ready for checkin → Accepted |
|---|
The review queue is "Accepted + Has patch".
comment:8 by , 10 years ago
| Cc: | added |
|---|
comment:9 by , 10 years ago
| Patch needs improvement: | set |
|---|
comment:10 by , 10 years ago
| Patch needs improvement: | unset |
|---|
comment:11 by , 9 years ago
| Keywords: | 1.10 added |
|---|---|
| Patch needs improvement: | set |
Left comments for improvement on the PR.
comment:12 by , 9 years ago
| Patch needs improvement: | unset |
|---|---|
| Triage Stage: | Accepted → Ready for checkin |
Ready for merge unless feature required for timezone support requiring pytz as mentioned https://github.com/django/django/pull/6243#issuecomment-219282120
comment:13 by , 9 years ago
| Triage Stage: | Ready for checkin → Accepted |
|---|
I added another commit to provide tzinfo support for Extract subclasses. New commit is: https://github.com/django/django/pull/6243/commits/4f08553fb0431184fdcc3e0eb9c3c52fa2bba09e
comment:14 by , 9 years ago
| Triage Stage: | Accepted → Ready for checkin |
|---|
I think you're on the right track for most of these points. I'm not yet convinced that adding
DateExtractis necessary, but once the patch is written I'll take another look then. I just worry that there'll be two ways to extract a date part and that may lead to confusion.Would like others opinions on ExtractX rather than XExtract. I think it reads and sorts better.
YearLookup and YearComparisonLookup are going to be interesting. They'll still need to work the same way when used on the left hand side. Perhaps we can rename to YearLookupOptimised (for example, that's a bad name) and register that as a lookup, but provide a different ExtractYear that does what you'd expect on the right hand side. The optimised version would then just be a hidden implementation detail that users shouldn't touch themselves.
I'll have a look at the actual PR tomorrow. Great work so far.