New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #2443 -- Added DurationField #2995

Closed
wants to merge 19 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@mjtamlyn
Copy link
Member

mjtamlyn commented Jul 29, 2014

This has been added to core as well following design decision taken in https://code.djangoproject.com/ticket/2443

This field was never merged due to UI related issues. Personally I think a CharField with DD HH:MM:SS.uuuuuu formatting in it is sufficient - this is basically what a TimeField does anyway.

Uses bigint of microseconds for non-psql databases, interval on postgres.

Todo:

  • docs
  • form field

@mjtamlyn mjtamlyn changed the title [WIP][contrib.postgres] Fixed #2443 -- Add DurationField [contrib.postgres] Fixed #2443 -- Add DurationField Aug 7, 2014

@mjtamlyn

This comment has been minimized.

Copy link
Member Author

mjtamlyn commented Aug 7, 2014

Rebased due to conflicts and finished adding documentation and form field.

Reviews please!

@timgraham

View changes

docs/ref/forms/fields.txt Outdated

.. class:: DurationField(**kwargs)

* Default widget: :class:`TextInput`.

This comment has been minimized.

@timgraham

timgraham Aug 12, 2014

Member

remove the period for consistency? or add one after None on next line.

@timgraham

View changes

docs/ref/forms/fields.txt Outdated

* Default widget: :class:`TextInput`.
* Empty value: ``None``
* Normalises to: A python ``timedelta``.

This comment has been minimized.

@timgraham

timgraham Aug 12, 2014

Member

Normalizes
python -> Python
:class:~datetime.timedelta

@timgraham

View changes

docs/ref/forms/fields.txt Outdated
* Empty value: ``None``
* Normalises to: A python ``timedelta``.
* Validates that the given value is a string which can be converted into a
timedelta.

This comment has been minimized.

@timgraham

timgraham Aug 12, 2014

Member

`` around timedelta

@timgraham

View changes

docs/ref/forms/fields.txt Outdated
* Error message keys: ``required``, ``invalid``.

Accepts any format understood by
:function:`~django.utils.dateparse.parse_duration`.

This comment has been minimized.

@timgraham

timgraham Aug 12, 2014

Member

does "function" work? usually we just use "func"

@timgraham

View changes

docs/ref/models/fields.txt Outdated

.. class:: DurationField([**options])

A field for storing periods of time - modelled in python by

This comment has been minimized.

@timgraham

timgraham Aug 12, 2014

Member

modeled in Python

This comment has been minimized.

@mjtamlyn

mjtamlyn Aug 12, 2014

Author Member

shakes fist across the pond

@timgraham

View changes

docs/ref/models/fields.txt Outdated

A field for storing periods of time - modelled in python by
:class:`~python:datetime.timedelta`. When used on PostgreSQL, the data type
used is an `interval`, otherwise a `bigint` of microseconds is used.

This comment has been minimized.

@timgraham

timgraham Aug 12, 2014

Member

need double `` for formatting to work properly

@timgraham

View changes

docs/ref/utils.txt Outdated

Parses a string and returns a :class:`datetime.timedelta`.

Expects data in the format `"DD HH:MM:SS.uuuuuu"` or as specified by

This comment has been minimized.

@timgraham

timgraham Aug 12, 2014

Member

need double ``
what happens if the value is an unexpected format?

This comment has been minimized.

@mjtamlyn

mjtamlyn Aug 16, 2014

Author Member

The answer to your question is mentioned at the top of the section

@timgraham

This comment has been minimized.

Copy link
Member

timgraham commented Aug 12, 2014

Needs 1.8 release notes.

@mjtamlyn

This comment has been minimized.

Copy link
Member Author

mjtamlyn commented Aug 16, 2014

Rebased master and docs tweaks made. Should be considered blocked by #3047 as per the ticket.

# timedelta
iso8601_duration_re = re.compile(
r'^P'
r'(?:(?P<days>\d+(.\d+)?)D)?'

This comment has been minimized.

@schinckel

schinckel Aug 16, 2014

Contributor

IIRC, only the last component of an ISO8601 duration may be fractional.

http://en.wikipedia.org/wiki/ISO_8601#Durations

The smallest value used may also have a decimal fraction, as in "P0.5Y" to indicate half a year. This decimal fraction may be specified with either a comma or a full stop, as in "P0,5Y" or "P0.5Y".

Does your logic take this into account?

This comment has been minimized.

@mjtamlyn

mjtamlyn Aug 16, 2014

Author Member

I don't take it into account in the validation, it will interpret ISO8601 values correctly and also a few which do not strictly meet the criteria, such as P0.5DT0.5H. I don't think this is a major issue and this regex is complex enough anyway


The preferred format for durations in Django is '%d %H:%M:%S.%f'.

Also supports ISO 8601 representation.

This comment has been minimized.

@schinckel

schinckel Aug 16, 2014

Contributor

"Also supports ISO 8601 representation, with the largest component of day" (or similar). I think it should also be in documentation, and perhaps the reason why.

@mjtamlyn mjtamlyn force-pushed the mjtamlyn:dcp-interval branch Sep 4, 2014

@timgraham

View changes

django/db/backends/mysql/base.py Outdated
@@ -405,6 +409,12 @@ def convert_booleanfield_value(self, value, field):
value = bool(value)
return value

def convert_durationfield_value(self, value, field):

This comment has been minimized.

@timgraham

timgraham Sep 4, 2014

Member

would it be worth having this method on the base backend with a feature flag that controls whether or not it's called? not sure what the situation with 3rd party backends will be, but it would be nice for each backend that needs it not to have to copy/paste

This comment has been minimized.

@mjtamlyn

mjtamlyn Sep 6, 2014

Author Member

Possibly, though it feels a little like we're introducing two APIs for the same thing there...

@timgraham

View changes

django/db/models/fields/__init__.py Outdated
)

def get_db_prep_value(self, value, connection, prepared=False):
if connection.vendor == 'postgresql':

This comment has been minimized.

@timgraham

timgraham Sep 4, 2014

Member

use a feature flag or is PG the only DB on the planet that supports this?

This comment has been minimized.

@mjtamlyn

mjtamlyn Sep 6, 2014

Author Member

This is a much better argument IMO for a feature flag so I've done both.

@timgraham

View changes

django/forms/fields.py Outdated
@@ -528,6 +530,26 @@ def strptime(self, value, format):
return datetime.datetime.strptime(force_str(value), format)


class DurationField(Field):
widget = TextInput

This comment has been minimized.

@timgraham

timgraham Sep 4, 2014

Member

needed? looks like it's the default.

@timgraham

View changes

django/db/models/fields/__init__.py Outdated
@@ -1,5 +1,5 @@
# -*- coding: utf-8 -*-
from __future__ import unicode_literals
from __future__ import division, unicode_literals

This comment has been minimized.

@timgraham

timgraham Sep 4, 2014

Member

I don't see any division in the changes, have I missed why it's needed?

@timgraham

View changes

docs/ref/forms/fields.txt Outdated

* Default widget: :class:`TextInput`
* Empty value: ``None``
* Normalises to: A Python :class:`~python:datetime.timedelta`.

This comment has been minimized.

@timgraham

timgraham Sep 4, 2014

Member

Normalizes

@timgraham

View changes

docs/ref/models/fields.txt Outdated

.. class:: DurationField([**options])

A field for storing periods of time - modeled in python by

This comment has been minimized.

@timgraham

timgraham Sep 4, 2014

Member

Python

@timgraham

This comment has been minimized.

Copy link
Member

timgraham commented Sep 4, 2014

1.8 release notes please.

@mjtamlyn

This comment has been minimized.

Copy link
Member Author

mjtamlyn commented Sep 6, 2014

Note that there is still an outstanding issue to tackle here - namely the use of DurationField in F() expressions. We actually have some complex code in django.db.models.expressions.DateModifierNode which handles F('datetimefield') ± timedelta() and converts it into the relevant SQL (using a custom function on SQLite) - all of our databases support INTERVAL as a data type for arithmetic, just not as a column type. I think I should be able to do something similar, perhaps even cleverer to make F('datetimefield') ± F('durationfield') work properly on all Databases, or at worst raise ImproperlyConfigured for databases where it isn't easily doable.

@mjtamlyn mjtamlyn changed the title [contrib.postgres] Fixed #2443 -- Add DurationField Fixed #2443 -- Add DurationField Sep 6, 2014

@mjtamlyn

This comment has been minimized.

Copy link
Member Author

mjtamlyn commented Sep 6, 2014

On further inspection, I think that it will actually be rather hard to properly support this.... until #2496 is merged. Then the lines where we currently inspect for timedelta and apply the DateModifierNode can be extended with checks for Expression.output_type as well. For the moment, I'm not totally averse to writing something simple to just kill F('durationfield'), but I think #2496 should be merged, and this can be delayed until then.

@mjtamlyn mjtamlyn force-pushed the mjtamlyn:dcp-interval branch Nov 15, 2014

@mjtamlyn

This comment has been minimized.

Copy link
Member Author

mjtamlyn commented Nov 18, 2014

@akaariai please see a0a924c for my changes regarding DateModifierNode. What I've done:

  • Remove DateModifierNode
  • Add DurationValue as a subclass of Value used whenever the value is a timedelta.
  • Add DurationExpression which is used to generate the sql when 1) the database does not support a real timedelta field and 2) one side of the expression has output field DurationField (which DurationValue will have)
  • Refactor database backend operations as needed

The following used to work:

  • F(datetime) ± timedelta

The following now works:

  • F(datetime) ± timedelta
  • timedelta ± F(datetime)
  • F(datetime) ± F(duration)
  • F(duration) ± F(datetime)
  • More complex maths with the same objects.

The following works only on postgres, and does give bad answers on other databases. Not much I can do here unfortunately, the setup is almost impossible to detect.

  • durationfield__lt=F(datetime) - F(datetime)

I would like to un-document Value - I don't see much use case for it by end users and now Value(timedelta) would not work. Thoughts @jarshwah?

@jarshwah

This comment has been minimized.

Copy link
Member

jarshwah commented Nov 19, 2014

The only reason to continue to support/document Value is so that users can annotate their results with strings, numbers and bools. The downside, obviously, is that more complex values like datetime/interval aren't supported but sound like they should be.

I believe the support for basic values comes down to the backend database driver - so whatever they support is what Value supports (I haven't checked this - happy to be informed by someone with more knowledge).

I'm not sure if users will get much value (pun?) out of Value, but it is definitely useful internally. If it's useful to other expressions, would library writers not also find it useful? Maybe it could just be moved to the "technical information" section, but that feels like avoiding the decision.

We could wrap basic values no problem (and hide Value externally), but then you get into issues with mismatched output_fields. We could solve this with a wrapping type Cast(*expressions, output_field), which I think would be a good addition anyway, to minimise providing output_fields on multiple expressions to prevent mismatched typing.

I'm not against undocumenting Value as long as we wrap user values internally.

@timgraham

View changes

django/db/backends/sqlite3/base.py Outdated
connector, timedelta.days, timedelta.seconds, timedelta.microseconds)
def date_interval_sql(self, timedelta):
"""
We don't transform the value to a string here so we can detect it later

This comment has been minimized.

@timgraham

timgraham Dec 9, 2014

Member

This function seems to return a string so I don't understand this comment.

@timgraham

View changes

django/db/backends/__init__.py Outdated
@@ -548,6 +550,9 @@ class BaseDatabaseFeatures(object):

supports_binary_field = True

# Is there a true datatype for timedeltas?
supports_duration_field = False

This comment has been minimized.

@timgraham

timgraham Dec 9, 2014

Member

I was if something like has_duration_field_type might be more clear. Since DurationField will work on DBs where this is False it could be misleading unless you read the comment.

@timgraham

View changes

tests/expressions/tests.py Outdated
raised = True
self.assertTrue(raised, "TypeError not raised on attempt to binary or a datetime with a timedelta.")
def test_invalid_operator(self):
with self.assertRaises(Exception):

This comment has been minimized.

@timgraham

timgraham Dec 9, 2014

Member

assertRaisesMessage(TypeError, 'Invalid connector for timedelta: *)?

This comment has been minimized.

@mjtamlyn

mjtamlyn Dec 9, 2014

Author Member

The raised error is different depending on the backend - we get a TypeError where we are handling the functionality ourselves (e.g. on sqlite) but where a true data type exists we don't do extra validation and let the database throw its own errors. This is what all other field types do - if you do F('charfield') * F('datefield') for example then you get a DatabaseError.

This comment has been minimized.

@timgraham

timgraham Dec 9, 2014

Member

Thanks for clarifying. It would be nice to make the test more specific in some way (maybe backend specific), but if you feel that's too much work, please at least include this explanation in the test.

@timgraham timgraham changed the title Fixed #2443 -- Add DurationField Fixed #2443 -- Added DurationField Dec 9, 2014

@timgraham

This comment has been minimized.

Copy link
Member

timgraham commented Dec 9, 2014

LGTM unless you want to get a review from someone more familiar with the expressions stuff.

@timgraham

This comment has been minimized.

Copy link
Member

timgraham commented Dec 17, 2014

buildbot, test on trusty.

@mjtamlyn mjtamlyn force-pushed the mjtamlyn:dcp-interval branch to aa5b826 Dec 20, 2014

@mjtamlyn

This comment has been minimized.

Copy link
Member Author

mjtamlyn commented Dec 20, 2014

Fixed in 5755444

@mjtamlyn mjtamlyn closed this Dec 20, 2014

@mjtamlyn mjtamlyn deleted the mjtamlyn:dcp-interval branch Dec 20, 2014

@akaariai

This comment has been minimized.

Copy link
Member

akaariai commented Dec 23, 2014

I'm getting a failure on current master on SQLite:

======================================================================
FAIL: test_durationfield_add (expressions.tests.FTimeDeltaTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/akaariai/projects/django/tests/expressions/tests.py", line 775, in test_durationfield_add
    self.assertEqual(end_less, ['e2'])
AssertionError: Lists differ: [] != [u'e2']

Second list contains 1 additional elements.
First extra element 0:
e2

- []
+ [u'e2']
@@ -1251,8 +1256,16 @@ def get_db_converters(self, internal_type):
Some field types on some backends do not provide data in the correct
format, this is the hook for coverter functions.
"""
if not self.connection.features.has_native_duration_field and internal_type == 'DurationField':

This comment has been minimized.

@akaariai

akaariai Dec 23, 2014

Member

Ì'm a little late to the party, but wouldn't it be better to do this in the field's convert_value() method?

This comment has been minimized.

@mjtamlyn

mjtamlyn Dec 23, 2014

Author Member

That would mean a converter method is added for every backend, when it is not needed on PG (and now Oracle).

This comment has been minimized.

@akaariai

akaariai Dec 23, 2014

Member

Sorry, using convert_value() isn't what I had in mind, I meant using field.get_db_converters() to add connection.convert_duration() when not has_native_duration_field.

This comment has been minimized.

@mjtamlyn

mjtamlyn Dec 23, 2014

Author Member

Seems reasonable and will reduce how often the logic is run. See #3778

@mjtamlyn

This comment has been minimized.

Copy link
Member Author

mjtamlyn commented Dec 23, 2014

That test failure was reported to django-core-mentorship as well, but appears to not affect most installations. Perhaps it is a SQLite version issue. Can you find out exactly what queries are being run (and the .values() in the db would help too)?

        for e in Experiment.objects.all():
            print e.name, e.start, e.end, e.estimated_time
        print Experiment.objects.filter(end__lt=F('start') + F('estimated_time')).query
        end_less = [e.name for e in
            Experiment.objects.filter(end__lt=F('start') + F('estimated_time'))]
        print end_less
>>> ./runtests.py expressions.tests.FTimeDeltaTests.test_durationfield_add
<snip>
e0 2010-06-25 12:15:30.747000 2010-06-25 12:15:30.747000 0:00:00
e1 2010-06-26 12:15:30.747000 2010-06-26 12:15:31 0:00:00.253000
e2 2010-06-25 12:15:30.747000 2010-06-25 12:16:14.747000 1:00:00
e3 2010-06-29 12:15:30.747000 2010-06-30 09:23:30.747000 21:08:00
e4 2010-06-25 12:15:30.747000 2010-07-05 12:15:30.747000 9 days, 0:00:00
SELECT "expressions_experiment"."id", "expressions_experiment"."name", "expressions_experiment"."assigned", "expressions_experiment"."completed", "expressions_experiment"."estimated_time", "expressions_experiment"."start", "expressions_experiment"."end" FROM "expressions_experiment" WHERE "expressions_experiment"."end" < ((django_format_dtdelta('+', "expressions_experiment"."start", "expressions_experiment"."estimated_time"))) ORDER BY "expressions_experiment"."name" ASC
[u'e2']
.
----------------------------------------------------------------------
Ran 1 test in 0.010s

OK
@akaariai

This comment has been minimized.

Copy link
Member

akaariai commented Dec 23, 2014

I have a fix for the sqlite issue. The number value is long on my computer where the code assumes durations are only integers. I believe it is 32bit vs 64bit issue. See #3777.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment