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

activerecord: Use a simpler query condition for aggregates with one mapping #34963

Merged

Conversation

dylanahsmith
Copy link
Contributor

@dylanahsmith dylanahsmith commented Jan 17, 2019

Problem

When using an aggregate with one mapping, e.g.

class Customer < ActiveRecord::Base
  composed_of :balance, class_name: "Money", mapping: %w(balance amount)
end

The SQL query from Customer.where(balance: (1..50).map { |amount| Money.new(amount) }) would be something like

SELECT "customers".* FROM "customers" WHERE ((((((((((((((((((((((((((((((((((((((((((((((((("customers"."balance" = 1 OR "customers"."balance" = 2) OR "customers"."balance" = 3) OR "customers"."balance" = 4) OR "customers"."balance" = 5) OR "customers"."balance" = 6) OR "customers"."balance" = 7) OR "customers"."balance" = 8) OR "customers"."balance" = 9) OR "customers"."balance" = 10) OR "customers"."balance" = 11) OR "customers"."balance" = 12) OR "customers"."balance" = 13) OR "customers"."balance" = 14) OR "customers"."balance" = 15) OR "customers"."balance" = 16) OR "customers"."balance" = 17) OR "customers"."balance" = 18) OR "customers"."balance" = 19) OR "customers"."balance" = 20) OR "customers"."balance" = 21) OR "customers"."balance" = 22) OR "customers"."balance" = 23) OR "customers"."balance" = 24) OR "customers"."balance" = 25) OR "customers"."balance" = 26) OR "customers"."balance" = 27) OR "customers"."balance" = 28) OR "customers"."balance" = 29) OR "customers"."balance" = 30) OR "customers"."balance" = 31) OR "customers"."balance" = 32) OR "customers"."balance" = 33) OR "customers"."balance" = 34) OR "customers"."balance" = 35) OR "customers"."balance" = 36) OR "customers"."balance" = 37) OR "customers"."balance" = 38) OR "customers"."balance" = 39) OR "customers"."balance" = 40) OR "customers"."balance" = 41) OR "customers"."balance" = 42) OR "customers"."balance" = 43) OR "customers"."balance" = 44) OR "customers"."balance" = 45) OR "customers"."balance" = 46) OR "customers"."balance" = 47) OR "customers"."balance" = 48) OR "customers"."balance" = 49) OR "customers"."balance" = 50)

instead of the simpler query that we would get from not using composed_of (i.e. querying using Customer.where(balance: (1..50)))

SELECT "customers".* FROM "customers" WHERE "customers"."balance" IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50)

https://stackoverflow.com/questions/782915/mysql-or-vs-in-performance shows this could have a performance impact, at least in older versions of mysql. Mostly I just want to avoid a database regression by using composed_of to introduce an abstraction.

Solution

The code already had a special case for a single mapping, so I just separated this code path more to avoid building multiple conditions joined with OR.

build(table.arel_attribute(field_attr), object)
else
if mapping.length == 1
column_name, aggr_attr = mapping.first
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer using the same variable names in both path.

Suggested change
column_name, aggr_attr = mapping.first
field_attr, aggregate_attr = mapping.first

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That causes "shadowing outer local variable" ruby warnings

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh... I see...

…tected methods by default

Co-Authored-By: dylanahsmith <dylan.smith@shopify.com>
@rafaelfranca rafaelfranca merged commit 41dc4d9 into rails:master Jan 17, 2019
rafaelfranca added a commit that referenced this pull request Jan 17, 2019
…-field-query

activerecord: Use a simpler query condition for aggregates with one mapping
@dylanahsmith dylanahsmith deleted the better-composed-of-single-field-query branch January 18, 2019 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants