Skip to content

Stash left_joins into joins to deduplicate redundant LEFT JOIN#35864

Merged
kamipo merged 1 commit intorails:masterfrom
kamipo:improve_left_joins
Apr 4, 2019
Merged

Stash left_joins into joins to deduplicate redundant LEFT JOIN#35864
kamipo merged 1 commit intorails:masterfrom
kamipo:improve_left_joins

Conversation

@kamipo
Copy link
Copy Markdown
Member

@kamipo kamipo commented Apr 4, 2019

Originally the JoinDependency has the deduplication for eager loading
(LEFT JOIN). This re-uses that deduplication for left_joins.

And also, This makes left join order into part of joins, i.e.:

Before:

association joins -> stash joins (eager loading, etc) -> string joins -> left joins

After:

association joins -> stash joins (eager loading, left joins, etc) -> string joins

Now string joins are able to refer left joins.

Fixes #34325.
Fixes #34332.
Fixes #34536.

Originally the `JoinDependency` has the deduplication for eager loading
(LEFT JOIN). This re-uses that deduplication for `left_joins`.

And also, This makes left join order into part of joins, i.e.:

Before:

```
association joins -> stash joins (eager loading, etc) -> string joins -> left joins
```

After:

```
association joins -> stash joins (eager loading, left joins, etc) -> string joins
```

Now string joins are able to refer left joins.

Fixes rails#34325.
Fixes rails#34332.
Fixes rails#34536.
@kamipo kamipo merged commit 3e5e1f9 into rails:master Apr 4, 2019
@kamipo kamipo deleted the improve_left_joins branch April 4, 2019 23:58
kamipo added a commit to kamipo/rails that referenced this pull request Apr 27, 2019
This fixes a regression for rails#35864.

Usually, stashed joins (mainly eager loading) are performed as LEFT
JOINs.
But the case of merging joins/left_joins of different class, that
(stashed) joins are performed as the same `join_type` as the parent
context for now.
Since rails#35864, both (joins/left_joins) stashed joins might be contained
in `joins_values`, so each stashed joins should maintain its own
`join_type` context.

Fixes rails#36103.
kamipo added a commit to kamipo/rails that referenced this pull request May 19, 2019
…joins

rails#36293 was an issue for through association with `joins` for a long
time, but since rails#35864 through association with `left_joins` would also
be affected by the issue.

Implicit through table joins should be appeared before user supplied
joins, otherwise loading through association with joins will cause a
statement invalid error.

Fixes rails#36293.

```
% ARCONN=postgresql bundle exec ruby -w -Itest test/cases/associations/has_many_through_associations_test
.rb -n test_through_association_with_joins
Using postgresql
Run options: -n test_through_association_with_joins --seed 7116

# Running:

E

Error:
HasManyThroughAssociationsTest#test_through_association_with_joins:
ActiveRecord::StatementInvalid: PG::UndefinedTable: ERROR:  missing FROM-clause entry for table "posts"
LINE 1: ... "comments_posts" ON "comments_posts"."post_id" = "posts"."i...
                                                             ^
: SELECT "comments".* FROM "comments" INNER JOIN "comments" "comments_posts" ON "comments_posts"."post_id" = "posts"."id" INNER JOIN "posts" ON "comments"."post_id" = "posts"."id" WHERE "posts"."author_id" = $1

rails test test/cases/associations/has_many_through_associations_test.rb:61

Finished in 0.388657s, 2.5730 runs/s, 0.0000 assertions/s.
1 runs, 0 assertions, 0 failures, 1 errors, 0 skips
```
@kreintjes
Copy link
Copy Markdown
Contributor

kreintjes commented Jun 19, 2019

@kamipo Nice work! Any chance this will be back-ported to Rails 5.2? I'm unable to reference an automatically generated left join in a custom join now. Only solution I can think of for now is reverting the left_joins to custom joins('LEFT JOIN ...'), but that feels like a step back.

kamipo added a commit to kamipo/rails that referenced this pull request May 13, 2020
…associations

rails#38597 is caused by rails#35864.

To reproduce this issue, at least it is required four different models
and three left joins from different relations.

When merging a relation from different model, new stashed (left) joins
should be placed before existing stashed joins, but rails#35864 had broken
that expectation if left joins are stashed multiple times.

This fixes that stashed left joins order as expected.

Fixes rails#38597.
@pixelpax
Copy link
Copy Markdown

pixelpax commented Apr 7, 2021

@kreintjes I had to do

        if search.joins_values.include?(association_name.to_sym) || search.left_outer_joins_values.include?(association_name.to_sym)
          return search
        else
          return search.joins(association_name.to_sym)
        end

To avoid redundantly joining. Not really an acceptable solution in some cases, but hopefully a workaround for some in lieu of an official patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

3 participants