[Django] #22394: Several built in Lookups should actually be Transforms
Django
----------------------------------------------+---------------------
Reporter: smeatonj | Owner: nobody
Type: Uncategorized | Status: new
Component: Database layer (models, ORM) | Version: master
Severity: Normal | Keywords: lookups
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+---------------------
The following built in lookups should be Transforms rather than Lookups,
because we're transforming the date/time into a date/time part:
`Year, Month, Day, WeekDay, Hour, Minute, Second.`
Converting the above into Transforms then allows a Lookup to proceed:
`qs.filter(date_field__year__gte=2001)`
--
Ticket URL: <https://code.djangoproject.com/ticket/22394>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: smeatonj | Owner: nobody
Type: Uncategorized | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: lookups | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by smeatonj):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
Discussion on IRC mentions that the `Year` lookup does a range query
(BETWEEN x AND y) which is able to use date based indexes. If a Transform
were to be used, any field based indexes would be invalid, as a computed
/function-based index would need to be created instead to preserve
performance.
However, all other operations based on date fields do an EXTRACT(), which
can't use traditional indexes anyway. Having Year as a Lookup but the
other date/time lookups as Transforms doesn't really make sense either,
and would probably be more confusing than it is now.
--
Ticket URL: <https://code.djangoproject.com/ticket/22394#comment:1>
Django
Reporter: smeatonj | Owner: mjtamlyn
Type: Uncategorized | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: lookups | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by mjtamlyn):
* status: new => assigned
* owner: nobody => mjtamlyn
Comment:
Doing something with this issue is part of my kickstarter, so it would
probably be unfair for someone else to work on it.
I believe the current machinery is enough to provide "optimal" queries in
all circumstances. This will work by making `__year` a `Transform` which
is actually a no-op, and that transform having a set of lookups available
to it distinct from the normal lookups, which each both extract the year
*and* do the comparison. This allows us to use `EXTRACT` for the exact
case but not for the less than case (for example). (Note that if `__year`
becomes a transform all calls to `__year` explicitly become
`__year__exact`). At present, lookups and only used in filter queries. We
will need to be more intelligent here when considering their use in values
calls, custom index etc as then a `__year` transform cannot be a noop.
To work out the best option for each possible call here, I will be running
benchmarks and checking query plans, with and without indexes, for all the
possible queries. We can then work out what the best query is for each db
in each case - they need not necessarily be the same.
It is also possible with custom indexes that there may be a case for the
`__year` transform having a `__extract` transform which forces the use of
`EXTRACT()` at SQL level. This would be an unusual use case for year, but
perhaps more likely for `__date` or `__week` where a user could add an
index for this particular extract and then perform efficient queries using
it without needing to index the entire table. This could be much more
efficient with logging-like tables, but until I try it out I don't know.
--
Ticket URL: <https://code.djangoproject.com/ticket/22394#comment:2>
Django
Reporter: smeatonj | Owner: mjtamlyn
Type: Uncategorized | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: lookups | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by akaariai):
There is a way to have year lookup as a Transform, and also have
specialized Lookups for exact and friends. The way to do this is to write
YearExact's as_sql() in a way that it accesses directly self.lhs.lhs, that
is YearExact doesn't execute self.lhs.as_sql() at all, instead it skips
directly to the column of self.lhs. This works as self.lhs is known to be
a YearTransform instance, only way to access YearExact is through
YearTransform.
As it happens this has already been written as a test case in
custom_lookups. Check YearTransform and YearExact in
custom_lookups/tests.py. The implementation is PostgreSQL specific, so
there is some work to make it generic.
BTW for the kickstarter part - I think it is completely fair if somebody
else has time to work on this and implements it for you for your
kickstarter project. If you would be able to just merge patches others
wrote I think that would be completely fair way to accomplish your
kickstarter project. Of course, this is just my opinion.
--
Ticket URL: <https://code.djangoproject.com/ticket/22394#comment:3>
Django
Reporter: smeatonj | Owner: mjtamlyn
Type: Uncategorized | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: lookups | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by mjtamlyn):
Ah yes, I'd forgotten about accessing `lhs.lhs` directly that is a good
approach to this.
And that is a good point re the kickstarter - worst case is it gives me
more time to work towards supporting transforms elsewhere in the code
base.
--
Ticket URL: <https://code.djangoproject.com/ticket/22394#comment:4>
Django
Reporter: smeatonj | Owner: mjtamlyn
Cleanup/optimization | Version: master
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: lookups | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by timo):
* type: Uncategorized => Cleanup/optimization
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/22394#comment:5>
Django
Reporter: smeatonj | Owner: mjtamlyn
Type: | Status: assigned
Cleanup/optimization |
(models, ORM) |
Keywords: lookups | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jdufresne):
* cc: jon.dufresne@… (added)
* has_patch: 0 => 1
Comment:
Added an initial PR: https://github.com/django/django/pull/4393
This PR does not yet address the year `BETWEEN` indexing issue outline
above. I will look into it. Feedback on the initial status is welcome.
--
Ticket URL: <https://code.djangoproject.com/ticket/22394#comment:6>
Django
Reporter: smeatonj | Owner: mjtamlyn
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: lookups | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jdufresne):
My PR has addressed the index concern for `__year` lookups. The PR is
ready for review. Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/22394#comment:7>
Django
Reporter: smeatonj | Owner: mjtamlyn
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Accepted => Ready for checkin
Comment:
Looks good to me (awaiting +1 from Josh).
--
Ticket URL: <https://code.djangoproject.com/ticket/22394#comment:8>
Django
Reporter: smeatonj | Owner: mjtamlyn
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: lookups | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jarshwah):
+1 once extract_type is removed.
--
Ticket URL: <https://code.djangoproject.com/ticket/22394#comment:9>
Django
Reporter: smeatonj | Owner: mjtamlyn
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
* stage: Ready for checkin => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/22394#comment:10>
Django
Reporter: smeatonj | Owner: mjtamlyn
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: lookups | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jdufresne):
* needs_better_patch: 1 => 0
Comment:
Added docs and tests to PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/22394#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: smeatonj | Owner: mjtamlyn
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/22394#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: smeatonj | Owner: mjtamlyn
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Keywords: lookups | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"b5e0eede406633b88c6222031171f80876d8f5e1" b5e0eede]:
{{{
#!CommitTicketReference repository=""
revision="b5e0eede406633b88c6222031171f80876d8f5e1"
Fixed #22394 -- Refactored built-in datetime lookups to transforms.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22394#comment:13>