Fix ActiveRecord::Calculations#ids for a composite primary key model

[nvasilevski]

Given a model with a composite primary key like:
TravelRoute.primary_key = [:from, :to]

Calling TravelRoute.ids would return an array of identifiers which is represented by an array of arrays:

TravelRoute.all.ids # => [["Ottawa", "New York"], ["London", "Paris"]]

Implementation details

The basic case for a non-loaded relation has already been working so I only added a test to cover that.

So I only fixed the case for a loaded relation to make sure we Array#pluck all columns of the composite primary key

[nvasilevski]

This change wasn't necessary. At least it won't break added test cases. However it is still seems incorrect to call arel_columns like arel_columns([["from", "to"]]) (nested array) as it does nothing and simply returns the passed value:

rails/activerecord/lib/active_record/relation/query_methods.rb

Lines 1719 to 1720 in 1217127

	else
	field

Compared to arel_columns(["from", "to"]) which returns an Array or Arel attributes. Though I couldn't come up with a test case to show the need for a change.

Basically by not making this change we will loose the benefits which are provided by the arel_column method:

rails/activerecord/lib/active_record/relation/query_methods.rb

Lines 1725 to 1739 in 1217127

	def arel_column(field)
	field = klass.attribute_aliases[field] || field
	from = from_clause.name || from_clause.value
	if klass.columns_hash.key?(field) && (!from || table_name_matches?(from))
	table[field]
	elsif field.match?(/\A\w+\.\w+\z/)
	table, column = field.split(".")
	predicate_builder.resolve_arel_attribute(table, column) do
	lookup_table_klass_from_join_dependencies(table)
	end
	else
	yield field
	end
	end

But I can't tell which one we could test to show the difference. I don't think aliases handling applies to the primary key case but perhaps we could show the difference by using TravelRoute.primary_key = ["travel_routes.from", "travel_routes.to"] which doesn't look like a very realistic use-case

[nvasilevski]

Maybe that's a sign that we can simplify it to columns = Array(primary_key)
It looks like historically arel_columns was needed because it was part of the pluck method logic:
Shopify@71f0df9
But now ids has a separate implementation so we may not need to call arel_columns at all knowing that the primary_key won't contain aliased names or column names in any other form that just a plain column name

[paarthmadan]

I think it's better to maintain the current "internal" behaviour which is to return an Array of Arel attributes, ie. what you've done.

We can revisit whether it's needed at all in a follow up, but for now, despite it not explicitly affecting our test, changing the intended usage of arel_columns seems unwanted.

[MatheusRich]

I'm not an expert in this area, but the code LGTM.