ActiveRecord::Calculations#pluck accepts multiple columns #6500

Closed
wants to merge 3 commits into
from

Projects

None yet

5 participants

@jeroeningen

Rails currently supports this:
Person.pluck(:id) # SELECT people.id FROM people

I added two possibilities:
Person.pluck(:id) # SELECT people.id FROM people
Person.pluck([:id, :name]) # SELECT people.id, people.name FROM people
Person.pluck(:id, :name) # SELECT people.id, people.name FROM people

Because I made some stupid mistakes while updating my own fork of rails/rails this is a new fork and a follow up of the discussion located here: #5472

@steveklabnik
Member

Mentioning #5472 so that GitHub links them.

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff May 29, 2012
activerecord/lib/active_record/relation/calculations.rb
@@ -154,11 +162,13 @@ def pluck(column_name)
}
result.map do |attributes|
- raise ArgumentError, "Pluck expects to select just one attribute: #{attributes.inspect}" unless attributes.one?
-
- value = klass.initialize_attributes(attributes).values.first
-
- column.type_cast(value)
+ if attributes.one?
+ value = klass.initialize_attributes(attributes).values.first
+
+ column.type_cast(value)
+ else
+ column_name.collect{|c| attributes[c.to_s]}
+ end
@carlosantoniodasilva
carlosantoniodasilva May 29, 2012 Member

Columns need to be typecasted in both scenarios, otherwise pluck would work differently when using 1 and more columns.

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff May 29, 2012
activerecord/lib/active_record/relation/calculations.rb
@@ -139,11 +145,13 @@ def calculate(operation, column_name, options = {})
# # SELECT DATEDIFF(updated_at, created_at) FROM people
# # => ['0', '27761', '173']
#
- def pluck(column_name)
- if column_name.is_a?(Symbol) && column_names.include?(column_name.to_s)
- column_name = "#{table_name}.#{column_name}"
+ def pluck(*column_name)
+ if column_name.size == 1 && column_name.first.is_a?(Symbol) && column_names.include?(column_name.first.to_s)
+ column_name = "#{table_name}.#{column_name.first}"
+ elsif column_name.is_a?(Array)
+ column_name.flatten!
end
@carlosantoniodasilva
carlosantoniodasilva May 29, 2012 Member

The same check to add the table name has to happen for each column name, otherwise the method would work differently for each case.

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff May 29, 2012
activerecord/test/cases/calculations_test.rb
@@ -477,11 +477,26 @@ def test_pluck_with_selection_clause
assert_equal [50 + 53 + 55 + 60], Account.pluck('SUM(DISTINCT(credit_limit)) as credit_limit')
end
- def test_pluck_expects_a_single_selection
- assert_raise(ArgumentError) { Account.pluck 'id, credit_limit' }
+ def test_pluck_expects_a_single_or_multiple_selection
+ assert_nothing_raised(ArgumentError) { Account.pluck 'id, credit_limit' }
end
@carlosantoniodasilva
carlosantoniodasilva May 29, 2012 Member

There's no need to assert for nothing raised, just make sure you're getting the right response from the sql piece. Try to test some different sqls as well.

@jonleighton
Member

I think we should probably have this feature as people keep asking about it. However I think we should decide between Post.pluck([:a, :b]) and Post.pluck(:a, :b) and just support one of them. I think probably the latter.

@jeroeningen

@carlos, you were right with your comment my mistakes. Just fixed and pushed it!

@jon In my previous pull request, several people liked it (see #5472). So IMO that's not the problem. But why do you want to support just one of the possibilities of Post.pluck([:a, :b]) and Post.pluck(:a, :b) and not both?

@carlosantoniodasilva carlosantoniodasilva added a commit that closed this pull request Jun 22, 2012
@carlosantoniodasilva carlosantoniodasilva Merge branch 'pluck-multiple-columns'
Allow ActiveRecord::Relation#pluck to accept multiple columns. Returns an
array of arrays containing the type casted values:

    Person.pluck(:id, :name)
    # SELECT people.id, people.name FROM people
    # [[1, 'David'], [2, 'Jeremy'], [3, 'Jose']]

Closes #6500
d59b2ab
@carlosantoniodasilva

@jeroeningen I've pulled your changes and modified as per @jonleighton comments: d59b2ab. Thanks for your work on that :).

@RKushnir
Contributor

How about if pluck would also account for the select arguments? So Model.select([:id, :name]).pluck would project id and name.

@carlosantoniodasilva

@RKushnir we've just merged a change that will ignore the select values in 3a33572. If you think it can be improved, please feel free to open a new topic in the core mailing list. Thanks.

@sgerrand sgerrand pushed a commit to sgerrand/rails that referenced this pull request Nov 2, 2013
@carlosantoniodasilva carlosantoniodasilva Refactor pluck with multiple columns
Ensure it works with mix of symbols and strings, and with a select
clause possibly containing more than one column.

Also remove support for pluck with an array of columns, in favor of
passing the list of attributes:

    Model.pluck(:a, :b)

See comments: rails#6500 (comment)
6aae17e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment