Skip to content
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

Performance Rails Cop proposal: Replace Pluck with Select within where clauses or subqueries #246

Closed
ghost opened this issue May 19, 2020 · 2 comments · Fixed by #269
Closed
Labels
feature request Request for new functionality

Comments

@ghost
Copy link

ghost commented May 19, 2020

Is your feature request related to a problem? Please describe.

Too often I see folks using pluck as part of a query when they really should choose select. Examples that come most readily to mind are part of where clauses, especially when a subquery is involved. For example, I sometimes see something like

Widget.where(id: FooFactory.spectacular.pluck(:widget_id))

which will be very inefficient as the number of FooFactorys increases, since the ever-larger plucked Ruby array size will slow things down, making the following preferable (if a subquery is necessary at all):

Widget.where(id: FooFactory.spectacular.select(:widget_id))

Describe the solution you'd like

Introduce a Rubocop rule that will find, and possibly auto-correct, usages of pluck (and/or other finder methods??) in a subquery.

Describe alternatives you've considered

It's obviously possible to socialize knowledge about this, and I have tried, but a Rubocop rule will enforce even for those whom such socialization doesn't reach.

Additional context

Please forgive if I'm asking about this in the wrong way or with too little detail. Whereas I'm a huge Rubocop fan and grateful user, this is my first time requesting a feature or considering contributing to Rubocop.

@koic koic transferred this issue from rubocop/rubocop May 20, 2020
@andyw8
Copy link
Contributor

andyw8 commented May 20, 2020

Interesting idea. Another advantage of select approach is that it means we make only one roundtrip call to database rather than two.

@aaronkelton
Copy link

What about a scenario where you're already selecting on the same model? Widget.select here returns an array of widgets, from which we can pluck(:id) or map(&:id).

Widget.where(id: Widget.select(&:active?).pluck(:id))
# Rails/PluckInWhere: Use select instead of pluck within where query method.

The aforementioned scenario works, but Rubocop complains about the pluck. However, replacing it with a select in this case is not the correct solution because Array#select is not the same as ActiveRecord::QueryMethods#select. A straight swap will raise an error:

Widget.where(id: Widget.select(&:active?).select(:id))
# ArgumentError: wrong number of arguments (given 1, expected 0)

Should the code be modified to not warn when pluck is called on an Array? Or is there some other Rubocop rule that would help here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Request for new functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants