Combining .count with .group and ordering by :count generates invalid SQL in 5.2.3

[Szeliga]

There seems to be some sort of regression in ActiveRecord. This snippet generates different SQLs for different versions

Person.group(:school_id).order(:count).count

In 5.2.2.1

SELECT COUNT(*) AS count_all, "people"."school_id" AS people_school_id FROM "people" GROUP BY "people"."school_id" ORDER BY "people"."count" ASC

In 5.2.3 - generates invalid SQL

SELECT COUNT(*) AS count_all, "people"."school_id" AS people_school_id FROM "people" GROUP BY "people"."school_id" ORDER BY "count" ASC

Steps to reproduce

Group by a foreign key, count and sort by :count in 5.2.2.1 and 5.2.3
Equivalent snippet

Person.group(:school_id).order(:count).count

Expected behavior

It should generate a valid SQL

Actual behavior

The SQL generated in 5.2.3 is invalid

System configuration

Rails version: 5.2.3

Ruby version: 2.5.3

[xxx]

Also seeing this. In our case we were doing a .select(:name) from the model, and now have to order by count_name; so I can work around it, but until I know that this is actually intended behavior, I'm going to sit tight.

[daveallie]

Could you please provide some more information about which database adapter you're using and the column types of Person.

I just created a test repo on 5.2.3 pointing to a SQLite3 DB, and the query generated matches your 5.2.2.1 query.

[Szeliga]

The adapter is postgres, and the column is an int

[st0012]

@Szeliga It'd be really helpful if you can give us a sample code that can reproduce the issue with this issue template

[st0012]

I created a template:

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails", "5.2.3"
  gem "pg"
end

require "active_record"
require "minitest/autorun"
require "logger"

ActiveRecord::Base.establish_connection(adapter: "postgresql", database: "activerecord_unittest")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :people, force: true do |t|
    t.integer :group_id
    t.integer :count, default: 0
  end

  create_table :groups, force: true do |t|
    t.string name
  end
end

class Person < ActiveRecord::Base
  belongs_to :group
end

class Group < ActiveRecord::Base
  has_many :people
end

class BugTest < Minitest::Test
  def test_query_correction
    Person.group(:group_id).order(:count).count
  end
end

And the query seemed to be correct:

SELECT COUNT(*) AS count_all, "people"."group_id" AS people_group_id FROM "people" GROUP BY "people"."group_id" ORDER BY "people"."count" ASC

The query would still fail because of

ActiveRecord::StatementInvalid: PG::GroupingError: ERROR:  column "people.count" must appear in the GROUP BY clause or be used in an aggregate function
LINE 1: ...OM "people" GROUP BY "people"."group_id" ORDER BY "people"."...

But I think that's a different issue

[xxx]

@st0012 I'm using this template, which shows the issue. With 5.2.3, you now need to know what name AR is giving the field (it's not always count_all), which seems wrong to me.

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  # gem "rails", "5.2.2.1"
  gem "rails", "5.2.3"
  gem "pg"
end

require "active_record"
require "minitest/autorun"
require "logger"

ActiveRecord::Base.establish_connection(adapter: "postgresql", database: "activerecord_unittest", username: 'docker', host: 'localhost')
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :people, force: true do |t|
    t.integer :group_id
  end

  create_table :groups, force: true do |t|
    t.string name
  end
end

class Person < ActiveRecord::Base
  belongs_to :group
end

class Group < ActiveRecord::Base
  has_many :people
end

class BugTest < Minitest::Test
  def test_query_correction
    Person.group(:group_id).order(:count).count # passes on 5.2.2.1, fails on 5.2.3
  end

  def test_query_correction_again
    Person.group(:group_id).order(:count_all).count # passes on 5.2.3, fails on 5.2.2.1
  end
end

[st0012]

@xxx According to the correct SQL in author's description, it looks like the people table should have the count column: .... ORDER BY "people"."count" ASC

[xxx]

@st0012 You don't need an actual count column.
https://dba.stackexchange.com/questions/129410/order-by-count-and-bitmap-heap-scan sheds some light as to why.

Under 5.2.2.1, with the template I posted, I get

SELECT COUNT(*) AS count_all, "people"."group_id" AS people_group_id FROM "people" GROUP BY "people"."group_id" ORDER BY "people"."count" ASC

which works.

Under 5.3, I get

SELECT COUNT(*) AS count_all, "people"."group_id" AS people_group_id FROM "people" GROUP BY "people"."group_id" ORDER BY "count" ASC

which fails.

Same as the OP.

[itsWill]

I can reproduce this on the latest master with @xxx reproduction script, this regression seems to have been introduced on this commit by @kamipo: 311f001

I can't immediately think of a nice way of handling this other than treating count as a special case with something like:

if arg == :count
 arel_attribute(:count)
else
 ...
end

But even this isn't a good fix since in postgresql this query is valid:

 SELECT COUNT(*) AS count_all, `people`.`group_id` AS people_group_id FROM `people` GROUP BY people_group_id ORDER BY `people`.`count`

but it's not valid in mysql. I'm not sure if we can make this query work consistently across adapters.