Support for annotate(end=F('start') + F('duration'), output_field=DateTimeField)

[Michael Angeletti]

I've modeled a Ticket as such:

class Ticket(models.Model):
    # ... other fields omitted
    active_at = models.DateTimeField()
    duration = models.DurationField()

Using duration this way is more valuable than simply defining an expires_at DateTimeField, because it has the same amount of information and also allows me to do things like Ticket.objects.filter(duration__gt=timedelta(days=1)). However, I ran into a problem when I tried to run the following:

Ticket.objects.annotate(
    expires_at=models.Sum(
        models.F('active_at') + models.F('duration'),
        output_field=models.DateTimeField()
    )
)

The exception occurs during datetime parsing when a float is encountered during an attempted regexp search. Does this imply that all Sum results will be of the type float? If so, does that disqualify supporting the above example as a possible feature?

[Josh Smeaton]

What would you expect the SQL output of SUM(date_field) to be? Most databases (all?) do not allow Sum(date). The argument to Sum has to be numeric or convertable to numeric. Sum(interval) works for some backends.

What I suspect you really want though is this:

Ticket.objects.annotate(
    expires_at=(F('active_at') + F('duration'), output_field=models.DateTimeField())
)

Note that I've removed the Sum. We're now just adding two fields. I'm not 100% sure that this is supported on all backends, but if it's not it should be. Also note that I've wrapped both F() expressions in brackets so that I can apply the output_field to the combination of both. Can you give that a go and report back please?

[Michael Angeletti]

@jarshwah F only takes a name argument, so adding an output_field kwarg results in a TypeError. Without the ability to define which output field to use, I'm left with:

Ticket.objects.annotate(expires_at=F('active_at') + F('duration'))

Which results in:

FieldError: Expression contains mixed types. You must set output_field

I did, for the sake of completion, try:

Ticket.objects.annotate(expires_at=F('active_at') + F('duration'), output_field=models.DateTimeField())

This resulted in:

AttributeError: 'DateTimeField' object has no attribute 'resolve_expression'

[Josh Smeaton]

So this has convinced me even more that we need some kind of wrapper type to provide a top level output_field attribute to the expression, unless someone has a nicer idea?

I'd suggest something along the lines of:

Ticket.objects.annotate(expires=Output(F('active_at')+F('duration'), output_field=DurationField())

But I'm not sold on the name (or the look of it to be honest).

In the mean time though, you can capture the output of F()+F() outside the queryset construction, and modify the output_field directly:

In [16]: expires = F('active_at')+F('duration')

In [17]: expires.output_field = models.DurationField()

In [18]: qs = Ticket.objects.annotate(expires_at=expires)

In [19]: for q in qs:
   ....:     print(q.expires_at)
   ....:
2015-03-15 23:53:18.065764+00:00
2015-03-16 02:53:53.953573+00:00
2015-03-15 18:53:56.391507+00:00
2015-03-15 21:53:58.441390+00:00

[Carl Meyer]

If we went with a wrapper type (and I don't see a better solution), I would prefer a name like Expression (since that's what it is/wraps), rather than Output (which seems very purpose-specific).

[Josh Smeaton]

I partially agree, although the only reason (that I can foresee now.. later hindsight may show this to be totally wrong) to have a wrapper type like this is purely to designate the output type.

The name Expression is already used, and has the signature Expression(left, connector, right, output_field):

Expression(F('active_at'), '+', F('duration'), output_field=DurationField())

There is already a type that could be used although it's not exported in django.db.models:

ExpressionNode(F('active_at')+F('duration'), output_field=DurationField())

I don't like the idea of exposing ExpressionNode purely because of the name. It's an internal implementation detail, but it fits the signature. I guess we could import it under any name we choose though:

# django.db.models.py

from django.db.expressions import ExpressionNode as Expr?

[Josh Smeaton]

Pull Request: ​https://github.com/django/django/pull/4329

I think documenting is enough at this point. We already have a type exposed that is capable of wrapping the expression, and the use-case of combining different types is small enough to not need anything complicated. If there are others that run into this problem and it seems to cause a bit of pain, then we can investigate a different approach then.

[Anssi Kääriäinen]

Documentation should be good enough for now. If we need something more, then a possibility is to extend usage of F(). Maybe F(F('active_at')+F('duration'), output_field=DurationField()) could be made to work?

[Michael Angeletti]

@jarshwah @akaariai - excellent, thanks for explaining that and adding a documentation patch for it. I had a suspicion that a missing wrapper was the only problem, so I found myself trying to use Aggregate, etc. I agree that a documentation patch is perfectly sufficient, since it's is already possible to do. This documentation also helps the reader better comprehend the result of operating on two F instances.

[Tim Graham]

Commit messages to fix this referenced the wrong ticket number.

In 820381d3:

Fixed #24486 -- Documented method to provide output_field to mixed F expressions

In 09062e95:

[1.8.x] Fixed #24486 -- Documented method to provide output_field to mixed F expressions

Backport of 820381d38bc02ea8b92837ce869e7332a7db9913 from master

[Carl Meyer]

I think the documented solution is fine from an API perspective (the general approach of wrapping the entire construct in something with an output_field argument seems right), except that ExpressionNode is a bit of an internal-feeling name.

I don't think using F would be a clarity improvement - the expression may include fields, but as a whole it is not a field.

I still think the best name for the wrapper would be simply Expression, since that's what the whole thing is; it's too bad that name is already taken by something with an incompatible signature. From an API perspective (not considering internals), I think it would be quite sensible to be able to instantiate an Expression with nothing more than a sub-expression and an output field (rather than requiring a lhs, connector and rhs), but in terms of the implementation that would essentially mean having the Expression constructor return an ExpressionNode, which doesn't work. The implementation of Expression is quite tied to having both an lhs and rhs.

Importing ExpressionNode as Expr might be a small improvement, but results in having both Expr and Expression, which differ in a way that's not at all indicated by their names. That's not great either.

I don't have a better proposal, and I agree that if this is an edge use case we can just live with documenting ExpressionNode for now, and maybe for good. Just wanted to get these API thoughts down in case we ever do revisit.

[Tim Graham <timograham@…>]

In 88d798d:

Refs #24485 -- Renamed some expression types

[Tim Graham <timograham@…>]

In a0cebe8:

[1.8.x] Refs #24485 -- Renamed some expression types

Backport of 88d798d71a20662bdf5335f0586fb9eb6e660c57 from master

[Michael Angeletti]

Sorry to reopen this ticket, guys. I've been so busy the past few days I didn't get a chance to try out the new code / docs until after work tonight. The issue with the docs update and code updates, as I see them, is that Expression takes only 2 arguments, namely self and output_field, so the example in the docs (fitted to my schema):

from django.db import models
from myapp.models import Ticket

Ticket.objects.annotate(
    expires_at=models.Expression(
        models.F('active_at') + models.F('duration'),
        output_field=models.DateTimeField()
    )
)

results in TypeError: __init__() got multiple values for argument 'output_field'

The aforementioned (by @jarshwah) method of simply adding an output_field to an a combined F didn't work either:

expires_at = models.F('active_at') + models.F('duration')
expires_at.output_field = models.DateTimeField()
Ticket.objects.annotate(expires_at=expires_at)

That resulted in:

django/db/models/query.py in iterator(self)
    259             if annotation_col_map:
    260                 for attr_name, col_pos in annotation_col_map.items():
--> 261                     setattr(obj, attr_name, row[col_pos])
    262 
    263             # Add the known related objects to the model, if there are any

AttributeError: can't set attribute

[Josh Smeaton]

You're right, I thought I had tested and written the result here, but that was for 24486. Doing both tickets at the same time was a mistake. I'll take a look at this now, and any fix I come up (and document) will also be replicated in the tests to ensure I don't not-fix it again.

[Josh Smeaton <josh.smeaton@…>]

In 02a2943e:

Fixed #24485 -- Allowed combined expressions to set output_field

[Josh Smeaton <josh.smeaton@…>]

In e654123f:

Fixed #24485 -- Allowed combined expressions to set output_field

[Tim Graham <timograham@…>]

In ce6062db:

[1.8.x] Fixed backport of refs #24485 tests.