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

Fix count(:all) with eager loading and explicit select and order #35850

Conversation

kamipo
Copy link
Member

@kamipo kamipo commented Apr 4, 2019

This follows up ebc09ed.

We've still experienced a regression for size (count(:all)) with
eager loading and explicit select and order when upgrading Rails to 5.1.

In that case, the eager loading enforces distinct to subselect but
still keep the custom select, it would cause the ORDER BY with DISTINCT
issue.

% ARCONN=postgresql bundle exec ruby -w -Itest test/cases/relations_test.rb -n test_size_with_eager_loading_and_custom_select_and_order
Using postgresql
Run options: -n test_size_with_eager_loading_and_custom_select_and_order --seed 8356

# Running:

E

Error:
RelationTest#test_size_with_eager_loading_and_custom_select_and_order:
ActiveRecord::StatementInvalid: PG::InvalidColumnReference: ERROR:  for SELECT DISTINCT, ORDER BY expressions must appear in select list
LINE 1: ..." ON "comments"."post_id" = "posts"."id" ORDER BY comments.i...
                                                             ^

As another problem on distinct is enforced, the result of count
becomes fewer than expected if select is given explicitly.

e.g.

Post.select(:type).count
# => 11

Post.select(:type).distinct.count
# => 3

As long as distinct is enforced, we need to care to keep the result of
count.

This fixes both the count with eager loading problems.

This follows up ebc09ed.

We've still experienced a regression for `size` (`count(:all)`) with
eager loading and explicit select and order when upgrading Rails to 5.1.

In that case, the eager loading enforces `distinct` to subselect but
still keep the custom select, it would cause the ORDER BY with DISTINCT
issue.

```
% ARCONN=postgresql bundle exec ruby -w -Itest test/cases/relations_test.rb -n test_size_with_eager_loading_and_custom_select_and_order
Using postgresql
Run options: -n test_size_with_eager_loading_and_custom_select_and_order --seed 8356

# Running:

E

Error:
RelationTest#test_size_with_eager_loading_and_custom_select_and_order:
ActiveRecord::StatementInvalid: PG::InvalidColumnReference: ERROR:  for SELECT DISTINCT, ORDER BY expressions must appear in select list
LINE 1: ..." ON "comments"."post_id" = "posts"."id" ORDER BY comments.i...
                                                             ^
```

As another problem on `distinct` is enforced, the result of `count`
becomes fewer than expected if `select` is given explicitly.

e.g.

```ruby
Post.select(:type).count
# => 11

Post.select(:type).distinct.count
# => 3
```

As long as `distinct` is enforced, we need to care to keep the result of
`count`.

This fixes both the `count` with eager loading problems.
@kamipo kamipo merged commit c9981ae into rails:master Apr 4, 2019
@kamipo kamipo deleted the fix_count_all_with_eager_loading_and_select_and_order branch April 4, 2019 08:32
kamipo added a commit that referenced this pull request May 8, 2019
…g_and_select_and_order

Fix `count(:all)` with eager loading and explicit select and order
rmacklin referenced this pull request in mislav/will_paginate Mar 27, 2020
Active Record 6 introduces a private method `select_for_count` which
conflicts with the one from will_paginate.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant