Upgrading to 1.2.0 breaks total_count for distinct subqueries

[oehlschl]

Encountered this issue when upgrading from 1.1.1 to 1.2.0, and it seems similar to #1015; likely also related to change #979.

The query below is a little contrived, but it demonstrates the issue and this test passes in 1.1.1 but fails in 1.2.0:

test 'total_count with distinct and group in subquery' do
   assert_equal 3, User.joins(:authorships).select('COUNT(*)').distinct.group(:id).order('COUNT(*) DESC').page(1).total_count
end

In 1.1.1, the code above issues the following query:

SELECT COUNT(DISTINCT "users"."id") AS count_id, "users"."id" AS users_id FROM "users" INNER JOIN "authorships" ON "authorships"."user_id" = "users"."id" GROUP BY "users"."id"

In 1.2.0, it becomes:

SELECT COUNT(*) FROM (SELECT DISTINCT 1 FROM "users" INNER JOIN "authorships" ON "authorships"."user_id" = "users"."id" GROUP BY "users"."id") subquery

Effectively, the distinct and select 1 don't work in combination. Admittedly probably an edge case but I thought it worth reporting nonetheless.

Thanks!

[yuki24]

@oehlschl I just reverted #979 as an attempt to fix the count-related issue. Could you try using the master branch on GitHub so we can be more confident about the fix?

[bendilley]

I understand why this presents as a bug but I would contest that using DISTINCT and GROUP BY in the same SQL query (whether you're building that using AR or not) is likely to indicate/result in problems anyway...

Let's think of DISTINCT as a lazy GROUP BY all_of, the, columns (please correct me if this assertion is wrong!). If you then add GROUP BY specific_column(s) to a query that uses DISTINCT, that DISTINCT becomes redundant: GROUP BY will always give you distinct results, by definition.

I know the example here is contrived, but if I were confronted with

.select('COUNT(*)').distinct.group(:id).order('COUNT(*) DESC')

then I'd immediately say "lose the distinct" - it's not doing anything because you're already grouping by id, so the results will be distinct. While leaving it in is theoretically harmless, it probably indicates an intention which isn't reflected in the outcome (or maybe a refactor has made it redundant, or a scope has been built-up from disparate expressions, etc.). In any case, I'd like to understand the author's intent at this point.

Thinking about it, maybe AR should apply distinct(false) as soon as group is scoped! 🤔

[pjshwa]

I think the revert commit of #979 deserves its own release.

[phlegx]

@yuki24 I have the same problem and tried out the master branch. Master branch works again! Please revert #979 and release.

[yuki24]

Now that v1.2.1 is out this should be fixed now. Thanks for your patience!