Opened 3 years ago

Closed 2 years ago

Last modified 21 months ago

#18375 closed Bug (fixed)

F() doesn't work as expected across multijoin relations

Reported by: FunkyBob Owned by: akaariai
Component: Database layer (models, ORM) Version: 1.4
Severity: Release blocker Keywords:
Cc: Kronuz Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by ramiro)

When trying to compare a field to another on the same related record, F() will compare against a separate alias of the table, thus not guaranteeing filtering against the same row.

There doesn't appear to be anything in the docs or tests to indicate what the 'correct' behavior is. Also, there's no apparent way to control it.

Using the attached models, I get the following:

(InteractiveConsole)
>>> from sample import models
>>> from django.db.models import Q, F
>>> qset = models.Uber.objects.filter(unter__available__gt=F('unter__used'))
>>> str(qset.query)
'SELECT "sample_uber"."id", "sample_uber"."title" FROM "sample_uber" INNER JOIN "sample_unter" ON ("sample_uber"."id" = "sample_unter"."uber_id") INNER JOIN "sample_unter" T3 ON ("sample_uber"."id" = T3."uber_id") WHERE T3."available" >  "sample_unter"."used"'

The SQL nicely formatted shows:

SELECT "sample_uber"."id", "sample_uber"."title"
FROM "sample_uber"
INNER JOIN "sample_unter" ON ("sample_uber"."id" = "sample_unter"."uber_id")
INNER JOIN "sample_unter" T3 ON ("sample_uber"."id" = T3."uber_id")
WHERE T3."available" >  "sample_unter"."used"

So, the F('unter__used') is using the 'sample_unter' join, whereas the unter__available__gt is using the T3 alias.

Attachments (3)

models.py (238 bytes) - added by FunkyBob 3 years ago.
ticket_18375_master.diff (4.7 KB) - added by akaariai 2 years ago.
ticket_18375_15.diff (4.3 KB) - added by akaariai 2 years ago.

Download all attachments as: .zip

Change History (25)

Changed 3 years ago by FunkyBob

comment:1 Changed 3 years ago by akaariai

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Summary changed from F() doesn't work as expected across relations to F() doesn't work as expected across multijoin relations
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Bug

To me this seems like a bug, or at least non-wanted behavior. Usually references inside one operation target the same alias, for example filter(unter__available__gt=1, unter__exact=foo) should target the same unter reference, while filter(unter__available__gt=1).filter(unter__exact=foo) should target different references.

If this can be fixed at this point needs at least some consideration. The change could break existing queries, and there wouldn't be any way to get the original behavior back, as you can split one filter clause to two different filter operations.

I have been thinking that it would be useful to be able to annotate the relations in the query. So, you could do:

.annotate(unter_ref='unter').filter(unter_ref__available__gt=F('unter_ref__used'))
OR
.annotate(unter_ref1='unter', unter_ref2='unter').filter(unter_ref1__available__gt=F('unter_ref2__used'))

The first would generate the query you are looking for, the second would generate what you get now. However there is a long way to get to the point where you can use the annotate() like in the example.

I am marking this as accepted. The reasoning is that the reference under the same .filter() call should target the same relation, and that the current behavior doesn't seem that useful. Backwards compatibility could turn this decision however.

For the time being I can't figure out a way to generate the query you want without using qs.extra().

comment:2 Changed 3 years ago by FunkyBob

A thought that came to me was a new keyword argument for F() to suggest it try to use an existing join to that table...

comment:3 Changed 3 years ago by akaariai

  • Has patch set

What happens is that when a filter is added to query, the right hand value (F-obj in this case) is added first. At this point no joins exist. Then, the left hand side is added to the query (the lookup is resolved), but it can't reuse the join generated by the F-obj. At this point a new join is generated -> problems.

Usually the F-obj will reuse any existing join, regardless if the join is generated in the same filter call or not. I am calling this a bug. Actually, this _must_ be a bug. Doing

.filter(unter__available__gt=0, unter__available__gt=F('unter__used'))

the F will target the existing join if the unter__available__gt=0 is evaluated first. If not then it will generate a new join. This means that randomized dict ordering will cause random results from the above query.

See https://github.com/akaariai/django/tree/ticket_18375 for one possible solution for this.

comment:4 Changed 3 years ago by akaariai

There might be an easier fix: just pass the reusable joins set from add_filter to the SQLEvaluator, then to prepare and finally to setup_joins. This means just adding the parameter to couple of methods (it can be by default the empty tuple), and things should work.

comment:5 Changed 2 years ago by akaariai

I have verified that dict-randomization will randomize the result of the query mentioned in comment:3.

I think we have to go with what FunkyBob suggests in comment:2 - add a "reuse_joins" argument to F() (better name welcome). If we change how F() is added to query without adding a way to define the join-reuse then we will be leaving some users in a hard situation: upgrade to 1.5 breaks their project with no way to fix the breakage.

I suggest we default to "reuse_joins=True", this way every clause under same .filter() call will target the same joins, unless explicitly asked to work otherwise.

I will try to write a patch for this.

comment:6 Changed 2 years ago by clelland

akaariai, shouldn't the query in comment:3 fail with a Syntax error? You can't specify the same keyword argument twice. The only way to filter twice on the same keyword is to call .filter() twice, which is well defined (in execution semantics, at least), or include duplicate Q objects (or mix Q objects with a keyword argument). Otherwise, you at least need to use a different operator, to filter twice on the same database field. Either

.filter(Q(unter__available__gt=0), Q(unter__available__gt=F('unter__used')))

or

.filter(Q(unter__available__gte=1, unter__available__gt=F('unter__used'))

should trigger this behaviour, though.

comment:7 Changed 2 years ago by akaariai

True, that example isn't valid Python.

The simplest example is something like this:

.filter(unter__id=1, id=F('unter__id'))

where unter is any reverse relation.

Depending on the ordering of the kwargs dict the F() either creates a new join or not. This means the same query will execute differently on different runs if dict randomization is turned on.

comment:8 Changed 2 years ago by aaugustin

Since we aim for Python 3.3 support in Django 1.5, this is a release blocker.

comment:9 Changed 2 years ago by akaariai

  • Owner changed from nobody to akaariai
  • Severity changed from Normal to Release blocker

comment:10 Changed 2 years ago by akaariai

Hmmh, I think this one is even more complex than I previously thought - how about

qs.filter(someval=F('revrel__somecol'), revrel__othercol=F('someval'))

If the first kwarg is seen first, then we will create a join for revrel. However, this join is _not_ reusable for the second filter's lookup. If these are seen in the other order, then we first generate revrel for the lookup, then reuse it in F().

I know we can fix the above. But I am wondering if we could just weasel out of this situation: document that use Q() lookups instead if dict ordering might be a problem. Q() lookups do have a defined order, so there is no problem in that case.

I have a work in progress patch at: https://github.com/akaariai/django/compare/ticket_18375

comment:11 Changed 2 years ago by akaariai

I rebased the ticket_18375 branch with another attempt. Now I like what it is doing in code, but I still wonder how to document this in release notes. What I have now seems a little complex...

Docs/API design help welcomed!

comment:12 Changed 2 years ago by nicolas

I think the documentation for this is overly complicated and requires detailed knowledge of what the SQL translation will be (which the ORM users shouldn't need).

The fix to only use one reference would probably work for 90% of the cases and for the other cases where you need to control exactly how the SQL query would be written you can fallback to raw SQL. I don't see a need for DupeMultisF; this fine grained control belongs in the SQL layer and not the ORM layer. Documenting that this generates a single join in 1.5 should be enough.

comment:13 Changed 2 years ago by ramiro

  • Description modified (diff)

comment:14 Changed 2 years ago by ptone

I'm confused with what you are doing with the dupe_multis value in the patch.

You removed its use in:

https://github.com/django/django/commit/68847135bc9acb2c51c2d36797d0a85395f0cd35

and the only reference to it currently in Django is a forgotten removal of it in the docstring for setup_joins

What would be a use case of multiple references be?

comment:15 Changed 2 years ago by akaariai

The patch is stale. The compare view is misleading, as it now compares my current master instead of the master at the time of when I created that patch... Maybe using that compare view isn't such a good idea after all. Updated patch available from here: https://github.com/akaariai/django/commit/bf6f1def617176503f2fc1c1e81c45c4b1b6fff2

As for what is the use case? Something like People.objects.filter(friends__age__gt=F('friends__age') * 2). This of course can't be true for single join, but multijoin case returns any people who have friends with 2x difference in age. Not too realistic example, but to me it seems there could be some real use cases for this.

IMO if we can get good notes about the backwards compatibility hack it doesn't cost us much in code complexity.

comment:16 Changed 2 years ago by akaariai

After some thought the backwards compatibility hack is now gone. If somebody complains about the change behavior then lets consider adding it.

So, the change is this: when previously a query like

qs.filter(revfk__somecol=F('revfk__somecol2'))

would have generated two separate joins for the reverse relation, and this:

qs.filter(revfk__somecol__contains='foo', revfk__somecol=F('revfk__somecol2'))

would have generated either 1 or 2 joins depending on dict ordering (if in the order above, then 1, if F() first, then 2).

Now both queries will generate just one join. This is correct when considering the principle "things under one .filter() call target the same m2m join".

Changed 2 years ago by akaariai

Changed 2 years ago by akaariai

comment:17 Changed 2 years ago by aaugustin

I'm not really capable to validate the code, but I fully support the behavior described in comment 16.

comment:18 Changed 2 years ago by akaariai

The previous patch had a problem where two chained filter calls would misuse joins. This was the failing test case:

   Employee.objects.filter(
            company_ceo_set__num_employees=F('pk')
   ).filter(
            company_ceo_set__num_employees=F('company_ceo_set__num_employees')
   )

Now, first filter adds a join to company_ceo_set. The second filter's F() will reuse any joins, so the join to company_ceo_set is reused. And, finally the second company_ceo_set will reuse the join used by F(), and we have two separate filter() calls reusing the same join.

The updated patch fixes this, and now F() expressions will behave just like other lookups in filter clauses when it comes to join reuse.

Patch available here: https://github.com/akaariai/django/commit/109a2b5598b7fc31e2b1d403e4da3a2a470748eb

This widens the scope of users possibly hit. Any query where there is a pre-existing revfk (or m2m) join and which then gets a F() added targeting the same relation will behave differently. Seems somewhat likely scenario to me.

To me it seems the patched version is doing what https://docs.djangoproject.com/en/dev/topics/db/queries/#spanning-multi-valued-relationships tells the behaviour should be.

comment:19 Changed 2 years ago by Anssi Kääriäinen <akaariai@…>

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

In 90b86291d022a09031d1df397d7aaebc30e435f7:

Fixed #18375 -- Removed dict-ordering dependency for F-expressions

F() expressions reuse joins like any lookup in a .filter() call -
reuse multijoins generated in the same .filter() call else generate
new joins. Also, lookups can now reuse joins generated by F().

This change is backwards incompatible, but it is required to prevent
dict randomization from generating different queries depending on
.filter() kwarg ordering. The new way is also more consistent in how
joins are reused.

comment:20 Changed 2 years ago by Anssi Kääriäinen <akaariai@…>

In 90c7aa074095311862b71f3b4ee7220369785375:

[1.5.x] Fixed #18375 -- Removed dict-ordering dependency for F-expressions

F() expressions reuse joins like any lookup in a .filter() call -
reuse multijoins generated in the same .filter() call else generate
new joins. Also, lookups can now reuse joins generated by F().

This change is backwards incompatible, but it is required to prevent
dict randomization from generating different queries depending on
.filter() kwarg ordering. The new way is also more consistent in how
joins are reused.

Backpatch of 90b86291d022a09031d1df397d7aaebc30e435f7

comment:21 Changed 2 years ago by akaariai

So, the final decision made was to just alter the behaviour of F() without a backwards compatibility escape.

I am not too worried about the changed behaviour itself. The queries hit are going to be rare. The bigger problem is that if somebody had a correctly working query using the old behaviour there is simply no way to get the old behaviour back without resorting to raw SQL.

It will be really easy to add a backwards compatibility F if needed (or a backwards compat flag to F()). See: https://github.com/akaariai/django/commit/6f50382b099410346f5305ea06cec93d6d006941

So, lets wait and see if somebody will complain about the change. If so, then lets add and document the backwards compatibility escape for those who need it. We can do this even in a point release in my opinion. Adding it just in case is also an option but if we add & document it, then we need to support it...

comment:22 Changed 21 months ago by Kronuz

  • Cc Kronuz added

I would definitely go for an annotate(), for the akaariai's example:

People.objects.filter(friends__age__gt=F('friends__age') * 2)

This, after the fix, makes just one join.

If one, however, wants the friends with 2x the age (two separate joins), it'd need annotate() , like so:

People.objects.annotate(twice_age=F('friends__age') * 2).filter(friends__age__gt=F('twice_age'))

How far is django from such behavior? ...and also, it doesn't still feel entirely good (annotate always creating new joins for such cases), since one might actually want to get the reused join... I would probably suggest the order of the calling of annotate to change things (if putting annotate after the filter, it'd reuse the joins from the previous filter, otherwise it would start afresh). This way, the next query would be equivalent to the first one:

People.objects.filter(friends__age__gt=F('twice_age')).annotate(twice_age=F('friends__age') * 2)

Provided one could use F() of not yet "annotated" things.

Any thoughts regarding this?

Last edited 21 months ago by Kronuz (previous) (diff)
Note: See TracTickets for help on using tickets.
Back to Top