Opened 3 years ago

Closed 3 years ago

#17886 closed Bug (fixed)

LOUTER join not promoted across filter expression

Reported by: milosu Owned by: nobody
Component: Database layer (models, ORM) Version: 1.4-beta-1
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

In my database I have a little bit complicated relations. The attached test case fails without applied patch.

The core of the problem is actually something like:

ModelA <-- ModelB ---0--> Model C --> Model D
                  <--0--> Model C --> Model D

When I put ModelA into django.contrib.admin changelist and try to search - the search fields containing fields from all above outlined models, the resulting queryset contains one INNER JOIN for the link between the second relation of Model C and Model D which removes from the result set those models for which the many2many link is empty - which I guess should not happen.

I think that the LEFT OUTER JOIN should propagage in this case across relations. Maybe this patch solves also other ORM OR-queries related problems present or already patched earlier..

The attached patch solves this problem and I hope it passes the whole Django test suite - although due to the time limitations I tried it only with "queries" and "aggregation_regress tests". With this patch I was running my app in production for 2 years now, so I expect it *should* work.

This bug was present in the Django ORM since the qs refactor branch was merged a few years ago so I think it does not need to be marked as 1.4 release bloker. But anyway..

Attachments (2)

louter.diff (4.3 KB) - added by milosu 3 years ago.
louter2.diff (4.1 KB) - added by milosu 3 years ago.
polished patch

Download all attachments as: .zip

Change History (6)

Changed 3 years ago by milosu

comment:1 Changed 3 years ago by aaugustin

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

The explanation makes sense to me.

But I'm not good enough with SQL and the ORM to guarantee that we want this change.

Also I'm a bit worried about backwards compatibility, since I can't tell exactly what this change would affect.

comment:2 Changed 3 years ago by akaariai

  • Patch needs improvement set

The idea in the patch seems correct to me. Closely related to #16715 and #10790.

However, the patch isn't to proper level (it is to +++ ../Django-1.4c1/django/db/models/sql/query.py), and there are stylistic errors (tab before space, missing newline between the test method at least).

I hope you can reformat the patch (using git is recommended) and repost it or link to github. If not, I think I will do that anyways, as to me it seems this patch is needed alongside #16715 for correct join promotions.

Changed 3 years ago by milosu

polished patch

comment:3 Changed 3 years ago by akaariai

  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Ready for checkin

I now know what was the reason for the wrong query. The basic problem was that we had an existing LOUTER JOIN, and joined a non-nullable FK to that. In that case we must LOUTER JOIN the new join, too, but the current code doesn't consider the case where the previous join was a LOUTER JOIN. The patch just directly creates the join as LOUTER JOIN, thus there is no need to fix promote_alias() logic.

#16715 would fix this issue, but even if I am planning to commit that one soon, I do think the idea of generating LOUTER JOIN automatically in case we are joining to an existing LOUTER JOIN chain is obviously correct, and thus it is a good idea to apply this.

The cleaned up patch is here, the main change is that the test case is reduced to the minimal set of filters displaying the issue in this ticket.

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

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

In [a193372753ad9d1d15ad5e2d6d06bbc07ca3f433]:

Fixed #17886 -- Fixed join promotion in ORed nullable queries

The ORM generated a query with INNER JOIN instead of LEFT OUTER JOIN
in a somewhat complicated case. The main issue was that there was a
chain of nullable FK -> non-nullble FK, and the join promotion logic
didn't see the need to promote the non-nullable FK even if the
previous nullable FK was already promoted to LOUTER JOIN. This resulted
in a query like a LOUTER b INNER c, which incorrectly prunes results.

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