Opened 7 years ago

Closed 5 years ago

#10154 closed (fixed)

allow the combination of an F expression with a timedelta

Reported by: Koen Biermans <koen.biermans@…> Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Keywords: expressions
Cc: gt4329b@…, coderanger Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: UI/UX:

Description

The proposal is to allow combining a F expression with a timedelta.

eg:

>>> longevents = Event.objects.filter(enddate__gt=F('startdate')+datetime.timedelta(days=5))

would return the events longer than 5 days.

I am attaching a first patch, which obviously needs refining.

Attachments (6)

dateexpressions.diff (6.3 KB) - added by Koen Biermans <koen.biermans@…> 7 years ago.
first stab at comining expressions with timedeltas
dateexpressions_2.diff (7.0 KB) - added by Koen Biermans <koen.biermans@…> 6 years ago.
new patch without the leaf
10154_dateexpressions_3.diff (8.0 KB) - added by ikelly 6 years ago.
Updated patch with Oracle syntax
10154_dateexpressions_4.diff (11.4 KB) - added by yourcelf 5 years ago.
Updated for R12380. Support for mysql, postgres, sqlite, oracle. Includes tests and docs.
10154.diff (20.1 KB) - added by kmtracey 5 years ago.
10154-r14446.diff (19.9 KB) - added by kmtracey 5 years ago.

Download all attachments as: .zip

Change History (24)

Changed 7 years ago by Koen Biermans <koen.biermans@…>

first stab at comining expressions with timedeltas

comment:1 Changed 7 years ago by Reflejo

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

I'm mostly -1 regarding how Leaf.evaluate() is implemented. Maybe is more simple if timedelta is converted to seconds (delta.seconds + delta.day * 86400), so you don't need to do that "mods" things :-).

This way you can move representations to each backend.

Changed 6 years ago by Koen Biermans <koen.biermans@…>

new patch without the leaf

comment:2 Changed 6 years ago by Koen Biermans <koen.biermans@…>

  • Needs documentation set
  • Patch needs improvement set

You are right, we can easily do without the leaf. see new patch.

The dispatching of the nodes should be made more generic though, but I don't know which way would be best to go about that.

I have also added a few more tests.

Changed 6 years ago by ikelly

Updated patch with Oracle syntax

comment:3 Changed 6 years ago by ikelly

Suggest adding tests for timedeltas with fractional seconds. The sqlite and postgres implementations do not appear to support that at the moment.

comment:4 Changed 6 years ago by Alex

  • Triage Stage changed from Unreviewed to Accepted

comment:5 Changed 6 years ago by yourcelf

Just added 10154_dateexpressions_4.diff, which implements and has tests for fractional seconds in sqlite and postgres, handles the case of a zero timedelta, and adds documentation.

comment:6 Changed 6 years ago by gt4329b

I'm definitely a big juicy +1 on this, as I need it right now for one of my sites

comment:7 Changed 6 years ago by gt4329b

  • Cc gt4329b@… added

Changed 5 years ago by yourcelf

Updated for R12380. Support for mysql, postgres, sqlite, oracle. Includes tests and docs.

comment:8 Changed 5 years ago by yourcelf

Added a new patch that passes tests against trunk (R12380) for this functionality, and implements MySQL. While microseconds are implemented, I removed tests for microseconds, because it seems MySQL and SQLite don't fully implement date comparisons involving microseconds.

Also added full support for MySQL.

I did not test with Oracle as I don't have an Oracle installation to test with (this patch relies on ikelly's patch for the implementation).

comment:9 Changed 5 years ago by yourcelf

  • Needs documentation unset
  • Patch needs improvement unset

Can someone test oracle with this patch?

If it passes, I think this ticket is ready for checkin after review from a triager/developer.

comment:10 Changed 5 years ago by aneil

Feature request: A timedelta database field with support for updates.

For example:

class Subscribers(Model):
    ...
    next_notification_time = DateTimeField()
    last_notification_time = DateTimeField()
    notification_timedelta = TimeDeltaField()

Subscribers.objects.all().update(next_notification_time=F('last_notification_time')+F('notification_timedelta'))

A snippet that implements time delta as an integer field here: http://www.djangosnippets.org/snippets/1060/.

comment:11 Changed 5 years ago by kmtracey

  • Needs tests set
  • Patch needs improvement set

It's clear why microseconds don't work on MySQL, since the doc states "...microseconds cannot be stored into a column of any temporal data type. Any microseconds part is discarded." In fact Django discards them, if they are specified, before they ever reach the DB, in order to avoid triggering a warning.

However, I cannot find anything in the SQLite doc (or tickets) to explain why microseconds would not work there. On the other hand I do see a couple of possible problems with the code as implemented in the patch for SQLite, so at the moment I'm suspicious the reason the tests failed for SQLite was that the implementation of the function in the patch for SQLite is not quite right.

First, patch uses the DATETIME() function, but the linked doc shows that that function maps to an STRFTIME format string that discards microseconds. It may be that we need to use STRFTIME with a format string that includes fractional seconds (%f) in order to get this feature to work properly. (Which may be a bit of a trick to get the percents in the STRFTIME format sting in the generated SQL here to pass through & get logged properly when DEBUG is True...or maybe I was just doing something wrong in a quick attempt to change the DATETIME to STRFTIME, and I've run out of time to look further at this right now.)

Second, this code (also from the SQLite changes):

        if timedelta.seconds:
            fs = timedelta.seconds + (timedelta.microseconds / 1000000.)
            modifiers.append(u'%s seconds' % fs)

isn't going to work properly when the seconds part of the timedelta is 0 but the microseconds part is non-zero.

Finally, we should not just fail to test some aspect of a function just because one (or even two...but I don't yet see why the SQLite implementation of this cannot be made to work) backend does not support it. We should have the tests and code them so that they are run on backends that support the function and skipped on backends that don't. We already do that in other places in the tests and perhaps in the future will have a better way of doing it, but completely dropping the fractional seconds tests because they don't run properly on all backends is not the right answer.

Changed 5 years ago by kmtracey

comment:12 Changed 5 years ago by kmtracey

Updated patch.

  • Reimplemented the sqlite support using a custom function; use of the sqlite-provided strftime causes problems because it will only format 3 digits of fractional second information, plus I do not see how to properly format both dates and datetimes in a way that will make them compare properly -- including trailing zeros for time information breaks comparisons against simple DATE values in sqlite.
  • Disallowed any ops other than addition and subtraction, letting others through to be caught by the DB leads to varying and potentially puzzling database errors, so it is better to make them fail early.
  • Beefed up the tests and made them unit tests, not doctests.
  • Some of the mixed (date, datetime) comparison tests fail on sqlite. That may be unavoidable due to the way it is just (I believe) comparing string values. I'm not sure we can code this to know when it is and is not right to drop zero-time information, but I have not fully looked into that so for now I have not yet coded that test to be skipped on sqlite.


comment:13 Changed 5 years ago by glic3rinu

kmtracey, Your patch update works good with latest trunk revision (14232) and mysql. Would be nice if someone can test it with others databases in order to include it in trunk soon :)

comment:14 Changed 5 years ago by kmtracey

I am hoping to get time to work on it this weekend. I have tested on DBs including Oracle so I'm pretty comfortable there, mostly I need to check again that there is nothing better we can do for some of the DBs for mixed date & datetime comparisons and maybe beef up the docs a bit.

comment:15 follow-up: Changed 5 years ago by Alex

My one note is I'd really like to keep the SQL specific stuff in the SQL specific classes (ie sql/expressions.py rather than expressions.py).

comment:16 in reply to: ↑ 15 Changed 5 years ago by kmtracey

Replying to Alex:

My one note is I'd really like to keep the SQL specific stuff in the SQL specific classes (ie sql/expressions.py rather than expressions.py).

OK...but I'm not really familiar enough with the ORM structure and long-term plans for adding non-sql support to understand what exactly to do here in response. Are you saying all of DateModifierNode should be moved to sql/expressions.py? Or...?

comment:17 Changed 5 years ago by coderanger

  • Cc coderanger added

I've been looking in to the problem on SQLite and it looks like it won't be easy to fix without fairly invasive changes. With normal values the field gets a chance to coerce values into the correct format, but SQLEvaluators bail out before this happens. I am going to try hooking up a mechanism for the field to be sent to the SQLEvaluator as a type hint.

Changed 5 years ago by kmtracey

comment:18 Changed 5 years ago by kmtracey

  • Resolution set to fixed
  • Status changed from new to closed

(In [15018]) Fixed #10154: Allow combining F expressions with timedelta values.

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