Skip to content
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

Preserve user supplied joins order as much as possible #36805

Merged

Conversation

@kamipo
Copy link
Member

commented Jul 29, 2019

Currently, string joins are always applied as last joins part, and Arel
join nodes are always applied as leading joins part (since #36304), it
makes people struggled to preserve user supplied joins order.

To mitigate this problem, preserve the order of string joins and Arel
join nodes either before or after of association joins.

Fixes #36761.
Fixes #34328.
Fixes #24281.
Fixes #12953.

@rails-bot rails-bot bot added the activerecord label Jul 29, 2019
Currently, string joins are always applied as last joins part, and Arel
join nodes are always applied as leading joins part (since #36304), it
makes people struggled to preserve user supplied joins order.

To mitigate this problem, preserve the order of string joins and Arel
join nodes either before or after of association joins.

Fixes #36761.
Fixes #34328.
Fixes #24281.
Fixes #12953.
@kamipo kamipo force-pushed the kamipo:user_supplied_joins_order_should_be_preserved branch from 03461c0 to b4478ae Jul 29, 2019
@kamipo kamipo merged commit 1e5c0a7 into rails:master Jul 30, 2019
2 checks passed
2 checks passed
buildkite/rails Build #62689 passed (13 minutes, 11 seconds)
Details
codeclimate All good!
Details
@kamipo kamipo deleted the kamipo:user_supplied_joins_order_should_be_preserved branch Jul 30, 2019
kamipo added a commit that referenced this pull request Jul 30, 2019
…d_be_preserved

Preserve user supplied joins order as much as possible
@eileencodes

This comment has been minimized.

Copy link
Member

commented Jul 31, 2019

We're hitting an issue where this changed behavior in GitHub for Rails 6.0. I haven't quite tracked down what's happening except that there are extra joins in the query. We use a lot of chained scopes and often those scopes will join on tables that perhaps another scope joined on. Previously I believe this was just ignored. It's not as simple as chained scopes though and I haven't been able to reproduce outside of the app yet. I think there is a bug in this change.

One of our scopes used to create sql like this:

LEFT OUTER JOIN `table_a` `column_a`
ON `column_a`.`target_type` = 'User'
AND `column_a`.`target_id` = `column_b`.`id`
AND `column_a`.`name` = 'disable'
AND `column_a`.`value` = 'TRUE' INNER JOIN `table_b`
ON `table_b`.`issue_column_d_id` = `table_c`.`id` INNER JOIN `table_d`
ON `table_b`.`subject_type` = 'Project'
AND `table_b`.`subject_id` = `table_d`.`id` WHERE `table_c`.`column_c` = 53 AND `table_c`.`column_d` IN ('added') AND ((`table_d`.`owner_type` = 'User')

and now creates sql like this:

LEFT OUTER JOIN `table_a` `column_a`
  ON `column_a`.`target_type` = 'User'
 AND `column_a`.`target_id` = `column_b`.`id`
 AND `column_a`.`name` = 'disable'
 AND `column_a`.`value` = 'TRUE' WHERE `table_c`.`column_c` = 58 AND `table_c`.`column_d` IN ('added')
 AND ((`table_d`.`owner_type` = 'User')

There are 2 extra inner joins in the first one. Do you know what's going on here @kamipo?

This results in a not unique alias method.

@kamipo

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2019

Weird... could you share the joins_values, left_outer_joins_values (just in case eager_load_values and includes_values) in the relation?

I guess the relation from the first one sql, it seems constructed by TableX(or TableB).left_joins(:table_a).merge(TableC.joins(table_b: :table_d)) (since Active Record has almost no way to apply LEFT OUTER JOIN before INNER JOIN).

If so, the values will be joins_values = [#<JoinDependency: for TableC.joins(table_b: :table_d)>], left_outer_joins_values = [:table_a].

If the INNER JOINs are lost, it seems that #<JoinDependency: for TableC.joins(table_b: :table_d)> is lost in joins_values or buckets[:stashed_join] somehow.

@eileencodes

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

@kamipo omg I'm so sorry, I swapped the queries. The inner joins aren't lost, we have extras.
The query we have is gigantic and it's really hard to track down.

What I provided is one part of a bunch of queries that get unioned together. Earlier in the query we join table_b and table_d already, so the second attempt to join causes a unique table alias error.

I think (and haven't yet reproduced because this query is so complex) that Rails used to drop the extra joins and now it's keeping them around.

kamipo added a commit to kamipo/rails that referenced this pull request Aug 1, 2019
rails#36805 have one possible regression that failing deduplication if
`joins_values` have complex order (e.g. `joins_values = [join_node_a,
:comments, :tags, join_node_a]`).

This fixes the deduplication to take it in the first phase before
grouping.
@kamipo kamipo referenced this pull request Aug 1, 2019
@codeodor

This comment has been minimized.

Copy link
Contributor

commented Sep 2, 2019

@eileencodes did you all find a workaround for this issue in Rails 6? I'm running up against something similar: in one app I maintain, it just obliterates the left join produced by includes + references and in another app it moves it to below the additional joins we added, so that it is now out of order to where the columns don't exist in the query before being referenced.

@codeodor

This comment has been minimized.

Copy link
Contributor

commented Sep 2, 2019

@eileencodes Never mind, I see #36834 seems to have fixed it for you. I guess I should probably open a new issue, since I think that made its way into Rails 6, and I'm still having a similar problem.

kamipo added a commit to kamipo/rails that referenced this pull request Sep 18, 2019
If a relation has eager_load and string joins only, string joins will be
regarded as leading joins unlike before, due to rails#36805.

To maintain that joining order as before, check eager loading join
first before string joins.

Fixes rails#37133.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.