Generate inner queries instead of join+distinct #605
Conversation
When applying filters with related entities, CanCan used to generate a LEFT JOIN plus a DISTINCT clause to avoid repeated records. This is problematic for a number of reasons: 1. Postgres is picky about setting ORDER BY with a column not on the DISTINCT clause, because it applies the ORDER after the DISTINCT so it cannot 'see' the referenced column. 2. Groups and Counts become more difficult to do because `foo.distinct.count` is not the same as `foo.count`. The way to avoid this problem is to use inner queries that don't multiply the number of returned records. We do this by running the query we were previously using inside a WHERE IN block.
fsateler
force-pushed
the
feature/dont-distinct
branch
from
9166512 to
1edb451
Compare
|
I can't reproduce the CI failures locally. AFAICS, the only difference is with the postgres version (I'm using 11, CI appears to use 9.6). |
Without an order by clause, order is undefined. Therefore if more than one result is returned, we don't know which one is first. Ensure we have a single record to make sure we have the one we want
|
I have reproduced this with postgres 9.6. The problem is that |
Otherwise, if the query already has a joins, we might add it twice. This also makes it unnecessary to remove the order and joins
fsateler
force-pushed
the
feature/dont-distinct
branch
from
a2767d2 to
67640e7
Compare
|
@fsateler Great work!
One more reason to avoid DISTINCT — it would fail when used on tables with |
If the user changed it, then it wouldn't work
|
I really like this PR! 👍 I'd like to run a couple of tests locally before merging it. Thanks for the nice work! |
|
Any update on this? Excited to see this get merged in. |
|
closed in #619 |
| @@ -76,7 +76,7 @@ class Editor < ActiveRecord::Base | |||
|
|
|||
| describe 'preloading of associatons' do | |||
| it 'preloads associations correctly' do | |||
| posts = Post.accessible_by(ability).includes(likes: :user) | |||
| posts = Post.accessible_by(ability).where(published: true).includes(likes: :user) | |||
There was a problem hiding this comment.
i don't think these changes had any affect on tests?
There was a problem hiding this comment.
It did. See the explanation #605 (comment)
The query changed just enough to get postgres to return a different record (because the query contained 2 but we applied limit 1).
There was a problem hiding this comment.
Ahhh yep right you are. I was only testing locally in sqlite when I wrote that comment. It seemed to not make a difference there.
When applying filters with related entities, CanCan used to generate a LEFT JOIN plus
a DISTINCT clause to avoid repeated records. This is problematic for a number of reasons:
foo.distinct.countis not the same asfoo.count.The way to avoid this problem is to use inner queries that don't multiply the number of returned records. We do this by running the query we were previously using inside a WHERE IN block.
This is a different approach than #600.
cc @ghiculescu
Fixes #596