-
Notifications
You must be signed in to change notification settings - Fork 41
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
Remove use of distinct in event listing #649
Conversation
1c27f6a
to
19b0627
Compare
Codecov Report
@@ Coverage Diff @@
## develop #649 +/- ##
===========================================
+ Coverage 75.45% 76.06% +0.60%
===========================================
Files 237 239 +2
Lines 17297 17357 +60
===========================================
+ Hits 13052 13203 +151
+ Misses 4245 4154 -91
... and 4 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
19b0627
to
56bee3e
Compare
Yritetty tarkistaa seuraavat relaatiot Events many-to-manyin_language Events one-to-manyvideos |
56bee3e
to
a00fc95
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The negated excludes are definitely not intuitive and hurt readability, but I'd say the benefits outweigh the cons. LGTM 👍
d867d2c
to
e47cf32
Compare
e47cf32
to
8f60a93
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can tell, this looks awesome! 🍪
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 🌞
The use of distinct is extremely slow. It's use can be avoided in one-to-many and many-to-many relations by using exclude(~Q(relation=something). When excluding relations, Django automatically uses SQL EXISTS. Negating the Q statement will essentially create a NOT NOT EXISTS which the database then optimizes into plain EXISTS. This appears to be still notably faster than using filter(Exists(subquery)) which creates an INNER JOIN inside the exists clause. Ref LINK-1408
48c5714
to
3324685
Compare
Kudos, SonarCloud Quality Gate passed! |
The use of distinct is extremely slow in the event listing.
The need for distinct can be replaced in one-to-many and many-to-many relations by using
exclude(~Q(relation=something)
. When excluding relations, Django automatically uses SQL EXISTS. Negating the Q statement will essentially create a NOT NOT EXISTS which the database then optimizes into plain EXISTS.DISTINCT can be avoided by utilizing EXISTS queries in filters that can create duplicate rows. Additionally some filter queries on m2m relations can be made still significantly faster if we target the through table directly to avoid unnecessary INNER JOIN by django.