Opened 8 years ago

Closed 19 months ago

Last modified 19 months ago

#9596 closed New feature (fixed)

Add a __date lookup for DateTimeField

Reported by: django@… Owned by: Tim Graham <timograham@…>
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords: lookup_type date datetimefield compare comparison query_term field lookup
Cc: jarek.zgoda@…, flosch@…, hv@…, JMagnusson, curtis@…, jon.dufresne@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Many times dates are stored in datetime fields even though the majority of lookup operations are going to be based on dates without times.

I am proposing a __date field lookup to make it easier to compare a DateTimeField to a date.

In plain SQL this is very easy; in Mysql/Sqlite it is just "date(fieldname)". The reason I mention this is not to indicate that it would be possible to make Django do it (though it is, and I will), but because it struck me that the operation is much more obscure using the Django DB layer than it is writing in plain SQL. For example, this is how it is done in django.views.date_based:

{'date_field__range': (datetime.datetime.combine(date, datetime.time.min), datetime.datetime.combine(date, datetime.time.max))}

And the way I am proposing:

{'date_field__date': date} # date is a datetime.date object

I have attatched a patch which alters three files and makes this work under Sqlite, and should also make it work under MySQL, however it implementation is by no means thorough. If this is deemed a good idea I will look into improving it.

Another idea is to allow comparing DateTimeFields to arbitrary length tuples which are in the format (YYYY, MM, DD, HH, MM, SS), but are between 1 and 6 elements long... So that for example I could do:

{'date_field': date.timetuple()[:2]}

And that would be true if the date was the same month.

Attachments (9)

dj_date_lookup.patch (2.2 KB) - added by Scott <django@…> 8 years ago.
Incomplete patch for date lookup type
dj_date_lookup_b.patch (6.6 KB) - added by Scott <django@…> 8 years ago.
patch b
dj_date_lookup_c.patch (2.6 KB) - added by Scott <django@…> 8 years ago.
patch c
dj_date_lookup_c.2.patch (2.6 KB) - added by Scott <django@…> 8 years ago.
patch c_2
dj_date_lookup_c_3.patch (3.7 KB) - added by Scott <django@…> 8 years ago.
patch c_3 with tests
t9596-r9441-date_lookup.diff (3.8 KB) - added by Scott <django@…> 8 years ago.
patch with test fixed
t9596-r9441-date_lookup.2.diff (3.9 KB) - added by Scott 8 years ago.
Updated patch
t9596-r10032-date_lookup.diff (3.9 KB) - added by Scott 8 years ago.
Silly me patch with corrected name.
9596.diff (5.7 KB) - added by Chris Beaven 7 years ago.
new patch (with failing test, showing backwards incompatibility)

Download all attachments as: .zip

Change History (45)

Changed 8 years ago by Scott <django@…>

Attachment: dj_date_lookup.patch added

Incomplete patch for date lookup type

comment:1 Changed 8 years ago by Collin Grady

I like date better than the tuple idea, which wouldnt be clear

comment:2 Changed 8 years ago by Collin Grady

woops, i mean __date :)

comment:3 Changed 8 years ago by Scott <django@…>

milestone: post-1.0
Needs tests: set
Patch needs improvement: set
Version: 1.0SVN

Patch B is an implementation which adds code to all the backends. It works by truncating the date a la "DATE(datetimefield)", like the first patch. The downside is that the datetime column must be truncated, which adds a large overhead to the query especially if the column is indexed.

Changed 8 years ago by Scott <django@…>

Attachment: dj_date_lookup_b.patch added

patch b

comment:4 Changed 8 years ago by Scott <django@…>

Patch needs improvement: unset

Patch C is a better implementation which essentially turns the date lookup into a range lookup. It doesn't add or modify any backend specific code and there is much less code overall.

Documentation is also included in the patch. I'm a bit lost as to how to add the tests, so if someone can give me an indication of where to start I'll have a go.

Changed 8 years ago by Scott <django@…>

Attachment: dj_date_lookup_c.patch added

patch c

Changed 8 years ago by Scott <django@…>

Attachment: dj_date_lookup_c.2.patch added

patch c_2

comment:5 Changed 8 years ago by Ian Kelly

I would support adding this. But I would suggest implementing this using the existing date_trunc_sql operations method, which is used for .dates() queries.

comment:6 Changed 8 years ago by Ian Kelly

For tests: take a look at the structure of tests/modeltests/lookup/models.py, and add them to the tests there.

comment:7 in reply to:  5 Changed 8 years ago by Scott <django@…>

Replying to ikelly:

I would support adding this. But I would suggest implementing this using the existing date_trunc_sql operations method, which is used for .dates() queries.

In patch B I chose the date_extract_sql function because date_trunc_sql is always returning a full datetime value, wheras the return format of date_extract_sql depends on the parameters. In patch C none of this applies, because it is turning the date lookup into a range lookup and so doesn't need any new backend code.

Changed 8 years ago by Scott <django@…>

Attachment: dj_date_lookup_c_3.patch added

patch c_3 with tests

comment:8 Changed 8 years ago by Scott <django@…>

Needs tests: unset

comment:9 Changed 8 years ago by Scott <django@…>

Patch needs improvement: set

Test is broken right now

Changed 8 years ago by Scott <django@…>

patch with test fixed

comment:10 Changed 8 years ago by Scott <django@…>

Patch needs improvement: unset

comment:11 Changed 8 years ago by Jarek Zgoda

Cc: jarek.zgoda@… added

comment:12 Changed 8 years ago by (none)

milestone: post-1.0

Milestone post-1.0 deleted

comment:13 Changed 8 years ago by Jacob

Triage Stage: UnreviewedAccepted

Good idea.

Changed 8 years ago by Scott

Updated patch

Changed 8 years ago by Scott

Silly me patch with corrected name.

comment:14 Changed 8 years ago by Alex Gaynor

Any particular reason this patch is implemented as an range as opposed to doing year, month, and day (genuinely curious).

comment:15 Changed 8 years ago by anonymous

Cc: flosch@… added

comment:16 Changed 8 years ago by Thomas Güttler

Cc: hv@… added

comment:17 in reply to:  14 Changed 8 years ago by Scott

Replying to Alex:

Any particular reason this patch is implemented as an range as opposed to doing year, month, and day (genuinely curious).

It didn't occur to me at the time, but I don't think it would be better. I'm not sure how the year, month and day lookups are implemented in Django, but if they aren't also done as ranges then the SQL engine has to convert column for each row, which means that it can't use any indexes that may exist on that column. If they are done as ranges it would produce three range lookups instead of one.

comment:18 Changed 7 years ago by Chris Beaven

Patch needs improvement: set
Triage Stage: AcceptedDesign decision needed

This looks good, the only one problem here is that it's a backwards incompatible change.

I'm attaching a patch with modified tests (which fail) based on the most recent patch in this ticket to show the incompatibility. We need a design decision about where to go from here... either we document the backwards incompatibility or the internal lookup logic has to change somehow so it doesn't swallow the related field lookup.

Changed 7 years ago by Chris Beaven

Attachment: 9596.diff added

new patch (with failing test, showing backwards incompatibility)

comment:19 Changed 6 years ago by Luke Plant

Severity: Normal
Type: New feature

comment:20 Changed 6 years ago by JMagnusson

Cc: JMagnusson added
Easy pickings: unset
UI/UX: unset

comment:21 Changed 6 years ago by Ulrich Petri

Related: #16187 Refactor of the lookup system

comment:22 Changed 5 years ago by curtis@…

Cc: curtis@… added

comment:23 Changed 5 years ago by Aymeric Augustin

Triage Stage: Design decision neededAccepted

This ticket is in DDN because the current technique fails if the __date lookup is used on a DateField.

There is exactly the same problem with the year lookup; its implementation depends on the field it's applied to:

# django/db/models/fields/__init__.py
        elif lookup_type == 'year':
            if self.get_internal_type() == 'DateField':
                return connection.ops.year_lookup_bounds_for_date_field(value)
            else:
                return connection.ops.year_lookup_bounds(value)

The implementation of year_lookup_bounds and year_lookup_bounds_for_date_field is database dependent. This allows setting the upper bound to YYYY-12-31 instead of YYYY-12-31 23:59:59.999999 for DateFields under Oracle.

I think we could mirror this solution for the __date lookup. This means implementing connection.ops.date_lookup_bounds(value) connection.ops.date_lookup_bounds_for_date_field(value) (in a backend dependent fashion if necessary).

It would be even better to convert the lookup to an exact match for DateField, but I don't believe that's doable with the current implementation.

Note that the tests in the patch need to be rewritten as unittests.

comment:24 Changed 3 years ago by Anssi Kääriäinen

Now that #16187 is fixed this should be relatively easy to implement:

class DateTransform(Transform):
    lookup_name = 'as_date'  # or just date, implementer's choice
    output_type = DateField()

    def as_sql(self, qn, connection):
        lhs, lhs_params = qn.compile(self.lhs)
        # Biggest problem is the following line - implement date cast template
        # for different backends. MySQL seems to be date(%s), PostgreSQL %s::date.
        date_cast_sql = connection.ops.date_cast_template
        return date_cast_template % lhs, lhs_params
DateTimeField.register_lookup(DateTransform)

comment:25 Changed 3 years ago by Aymeric Augustin

Still, I would support adding proper support for this feature in Django, if only to make sure time zones are handled correctly when USE_TZ is True. It's very easy to mess them up!

comment:26 Changed 2 years ago by Aymeric Augustin

Owner: changed from nobody to Aymeric Augustin
Status: newassigned

comment:27 Changed 21 months ago by Jon Dufresne

Cc: jon.dufresne@… added

Now that #16187 is fixed this should be relatively easy to implement:
...
DateTimeField.register_lookup(DateTransform)

I've started down the path suggested above with commit: https://github.com/jdufresne/django/commit/5bb3f1881982bc1248d5891a887725eeff7841bd

However, I'm not sure where this code should live if added to Django generally. What is the correct area of the code to call register_lookup()?

From the docs:

We can now use foo__ne for any field foo. You will need to ensure that this registration happens before you try to create any querysets using it. You could place the implementation in a models.py file, or register the lookup in the ready() method of an AppConfig.

As far as I can tell, these options don't really apply to *built-in* transforms.

I notice there is a default_lookups in django/db/models/lookups.py, but I'm not sure how to make this work for field specific transforms, instead of baisc lookups.

I tried putting register_lookup() inside django/db/fields/__init__.py, however this lead to issues with settings.

django.core.exceptions.ImproperlyConfigured: Requested setting DEFAULT_INDEX_TABLESPACE, but settings are not configured. You must either define the environment variable DJANGO_SETTINGS_MODULE or call settings.configure() before accessing settings.

Any thoughts?

Last edited 21 months ago by Jon Dufresne (previous) (diff)

comment:28 Changed 21 months ago by Aymeric Augustin

Owner: Aymeric Augustin deleted
Status: assignednew

comment:29 Changed 21 months ago by Aymeric Augustin

The code looks correct when USE_TZ = False. You also need to handle the more complicated case when USE_TZ = True.

See the current implementation datetime_trunc_sql for an idea of what's needed.

comment:30 Changed 21 months ago by Jon Dufresne

Patch needs improvement: unset

Created a PR that is ready for review.

https://github.com/django/django/pull/4281

comment:31 Changed 21 months ago by Tim Graham

Patch needs improvement: set

Some test failures on Oracle need to be fixed.

comment:32 Changed 21 months ago by Jon Dufresne

Patch needs improvement: unset

Test fixes and other feedback has been incorporated into the latest version of the PR. Additional feedback is welcome.

comment:33 Changed 20 months ago by Tim Graham

Patch needs improvement: set
Summary: Comparing a DateTimeField to a date is too hardAdd a __date lookup for DateTimeField

Left some mostly cosmetic comments, and tests are still failing on Oracle (but passing on my local version of Oracle, so might need some expertise about Oracle version differences to resolve that).

comment:34 Changed 19 months ago by Jon Dufresne

Patch needs improvement: unset

Updated patch for Oracle based on feedback. All tests are now passing.

comment:35 Changed 19 months ago by Tim Graham <timograham@…>

Owner: set to Tim Graham <timograham@…>
Resolution: fixed
Status: newclosed

In 44f3ee77:

Fixed #9596 -- Added date transform for DateTimeField.

comment:36 Changed 19 months ago by Thomas Güttler

Thank you very much :-)

Note: See TracTickets for help on using tickets.
Back to Top