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

Avoid `unscope(:order)` when `limit_value` is presented for `count` #26972

Merged
merged 1 commit into from Nov 6, 2016

Conversation

Projects
None yet
5 participants
@kamipo
Member

kamipo commented Nov 5, 2016

If limit_value is presented, records fetching order is very important
for performance. Should not unscope the order in the case.

@rails-bot

This comment has been minimized.

rails-bot commented Nov 5, 2016

r? @rafaelfranca

(@rails-bot has picked a reviewer for you, use r? to override)

@rafaelfranca

Could you add a CHANGELOG entry?

Avoid `unscope(:order)` when `limit_value` is presented for `count`
If `limit_value` is presented, records fetching order is very important
for performance. Should not unscope the order in the case.

@kamipo kamipo force-pushed the kamipo:avoid_unscope_order_when_limit_value_present branch to 62bb8b9 Nov 5, 2016

@kamipo

This comment has been minimized.

Member

kamipo commented Nov 5, 2016

I added a CHANGELOG entry!

@rafaelfranca rafaelfranca merged commit 2b2c096 into rails:master Nov 6, 2016

2 checks passed

codeclimate Code Climate didn't find any new or fixed issues.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kamipo kamipo deleted the kamipo:avoid_unscope_order_when_limit_value_present branch Nov 6, 2016

damianlegawiec added a commit to spark-solutions/spree that referenced this pull request Jun 25, 2017

Fixed Product#available scope for Rails 5.1
`.distinct` with `.size` caused PostgreSQL errors introduced by
rails/rails#26972

damianlegawiec added a commit to spark-solutions/spree that referenced this pull request Jun 25, 2017

Fixed Product#available scope for Rails 5.1
`.distinct` with `.size` caused PostgreSQL errors introduced by
rails/rails#26972

damianlegawiec added a commit to spark-solutions/spree that referenced this pull request Jun 25, 2017

Fixed Product#available scope for Rails 5.1
`.distinct` with `.size` caused PostgreSQL errors introduced by
rails/rails#26972
@@ -223,17 +223,17 @@ def operation_over_aggregate_column(column, operation, distinct)
end
def execute_simple_calculation(operation, column_name, distinct) #:nodoc:
# PostgreSQL doesn't like ORDER BY when there are no GROUP BY
relation = unscope(:order)

This comment has been minimized.

@teohm

teohm Jul 19, 2017

Contributor

@kamipo @rafaelfranca This change has caused an unexpected SQL error when a query contains both .distinct and .limit/.offset:

ActiveRecord::StatementInvalid: PG::InvalidColumnReference: ERROR:  for SELECT DISTINCT, ORDER BY expressions must appear in select list

In most SQL servers (e.g. PostSQL, SQL Server etc), columns listed in DISTINCT must also be listed in ORDER BY. This simple query that used to run in Rails 5.0 is now failing in Rails 5.1:

Model.distinct.order(updated_at: :desc).limit(10).count

because "count" operation injects column to DISTINCT list but keep the existing ORDER BY column list:

SELECT
    COUNT(DISTINCT count_column)
FROM (
    SELECT
        DISTINCT "models"."id" AS count_column
    FROM
        "models"
    ORDER BY
        "models"."updated_at" DESC  /* Error, column list not same as DISTINCT */
    LIMIT 10) subquery_for_count

See issues reported by others: kaminari/kaminari#888, spark-solutions/spree@b5785ad

This comment has been minimized.

@kamipo

kamipo Jul 19, 2017

Member

Thanks for the report. I created the PR #29848 that fixed the issue.

kamipo added a commit to kamipo/rails that referenced this pull request Jul 21, 2017

Fix `COUNT(DISTINCT ...)` with `ORDER BY` and `LIMIT`
Since rails#26972, `ORDER BY` is kept if `LIMIT` is presented for
performance. But in most SQL servers (e.g. PostgreSQL, SQL Server, etc),
`ORDER BY` expressions must appear in select list for `SELECT DISTINCT`.
We should not replace existing select list in that case.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment