-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Avoid unscope(:order)
when limit_value
is presented for count
#26972
Conversation
(@rails-bot has picked a reviewer for you, use r? to override) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a CHANGELOG entry?
If `limit_value` is presented, records fetching order is very important for performance. Should not unscope the order in the case.
5f54840
to
62bb8b9
Compare
I added a CHANGELOG entry! |
`.distinct` with `.size` caused PostgreSQL errors introduced by rails/rails#26972
`.distinct` with `.size` caused PostgreSQL errors introduced by rails/rails#26972
`.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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the report. I created the PR #29848 that fixed the issue.
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.
If
limit_value
is presented, records fetching order is very importantfor performance. Should not unscope the order in the case.