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

Handle aliased attributes in AR::Relation #7839

Conversation

@chancancode
Copy link
Member

chancancode commented Oct 4, 2012

I started experimenting with alias_attribute in my STI models and got quite frustrated by the second-class support for attributes aliasing. Ideally if you aliased an attribute all the AR methods should be able to resolve the real column name automatically, just like if you have set self.table_name = ... then all of AR (associations, etc) will be made aware of that and do the right thing.

This commit added support for aliased attributes in the finders, calculation methods, counting and pluck. This doesn't cover everything, but I believe it's a step in the right direction, and fixed this in places where it matters the most (finders).

@jonleighton and @rafaelfranca can you take a look? (This basically builds on top of #6800)

@carlosantoniodasilva
carlosantoniodasilva reviewed Oct 7, 2012
View changes
activemodel/lib/active_model/attribute_methods.rb Outdated
@@ -66,7 +67,7 @@ module AttributeMethods

included do
class_attribute :attribute_aliases, :attribute_method_matchers, instance_writer: false
self.attribute_aliases = {}
self.attribute_aliases = HashWithIndifferentAccess.new

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Oct 7, 2012 Member

You should probably use the full ActiveSupport::HashWithIndifferentAccess constant here, since it's how it's used inside Rails.

@chancancode
Copy link
Member Author

chancancode commented Oct 8, 2012

@carlosantoniodasilva Fixed. Thanks for the feedback.

@schneems
Copy link
Member

schneems commented Oct 16, 2012

We're doing some additional hash lookups, and HashWithIndifferentAccess is typically more expensive than a literal. Any performance degradation between this and the current master?

You can get a cross section of different SQL query performance running $ bundle exec ruby examples/performance.rb with and without your change from within the active record directory.

@chancancode
Copy link
Member Author

chancancode commented Oct 16, 2012

mbp activerecord [master] $ bundle exec ruby examples/performance.rb 
Generating data...
Inserting 20000 users and exhibits...
Calculating -------------------------------------
            Model#id     14025 i/100ms
Model.new (instantiation)
                          3063 i/100ms
Model.new (setting attributes)
                          1769 i/100ms
         Model.first       250 i/100ms
Model.all limit(100)         7 i/100ms
Model.all limit(100) with relationship
                             3 i/100ms
Model.all limit(10,000)
                             1 i/100ms
   Model.named_scope      1268 i/100ms
        Model.create       124 i/100ms
Resource#attributes=      1501 i/100ms
     Resource#update       134 i/100ms
    Resource#destroy       165 i/100ms
   Model.transaction       795 i/100ms
      Model.find(id)       381 i/100ms
   Model.find_by_sql      1100 i/100ms
           Model.log      4009 i/100ms
   AR.execute(query)      1254 i/100ms
-------------------------------------------------
            Model#id  2474889.9 (±4.1%) i/s -   49241775 in  19.958908s
Model.new (instantiation)
                        54733.3 (±32.5%) i/s -     967908 in  19.989201s
Model.new (setting attributes)
                        24973.9 (±25.0%) i/s -     463478 in  20.001136s
         Model.first     2584.4 (±19.0%) i/s -      49750 in  20.014064s
Model.all limit(100)       75.4 (±22.5%) i/s -       1428 in  20.090384s
Model.all limit(100) with relationship
                           48.7 (±24.6%) i/s -        897 in  20.000046s
Model.all limit(10,000)
                            0.7 (±0.0%) i/s -         14 in  20.989994s
   Model.named_scope    16537.3 (±26.4%) i/s -     303052 in  20.040075s
        Model.create     1284.0 (±17.7%) i/s -      24924 in  20.020046s
Resource#attributes=    18428.8 (±22.3%) i/s -     348232 in  20.050107s
     Resource#update     1444.1 (±19.5%) i/s -      27738 in  20.079911s
    Resource#destroy     1757.4 (±19.0%) i/s -      33825 in  20.029993s
   Model.transaction     9257.8 (±24.3%) i/s -     173310 in  20.044709s
      Model.find(id)     4120.1 (±18.5%) i/s -      79248 in  20.034182s
   Model.find_by_sql    12599.4 (±17.5%) i/s -     240900 in  19.995763s
           Model.log    69369.1 (±30.7%) i/s -    1222745 in  19.999424s
   AR.execute(query)    14747.0 (±18.7%) i/s -     282150 in  20.060213s
mbp activerecord [master] $ git co handle_aliased_attributes_in_ar_relation 
Switched to branch 'handle_aliased_attributes_in_ar_relation'
mbp activerecord [handle_aliased_attributes_in_ar_relation] $ bundle exec ruby examples/performance.rb 
Generating data...
Inserting 20000 users and exhibits...
Calculating -------------------------------------
            Model#id     14026 i/100ms
Model.new (instantiation)
                          3192 i/100ms
Model.new (setting attributes)
                          1772 i/100ms
         Model.first       257 i/100ms
Model.all limit(100)         7 i/100ms
Model.all limit(100) with relationship
                             4 i/100ms
Model.all limit(10,000)
                             1 i/100ms
   Model.named_scope      1296 i/100ms
        Model.create       124 i/100ms
Resource#attributes=      1481 i/100ms
     Resource#update       139 i/100ms
    Resource#destroy       166 i/100ms
   Model.transaction       797 i/100ms
      Model.find(id)       376 i/100ms
   Model.find_by_sql      1096 i/100ms
           Model.log      4045 i/100ms
   AR.execute(query)      1269 i/100ms
-------------------------------------------------
            Model#id  2519443.8 (±2.9%) i/s -   50227106 in  19.959355s
Model.new (instantiation)
                        55677.5 (±33.0%) i/s -     983136 in  19.999870s
Model.new (setting attributes)
                        24780.2 (±25.0%) i/s -     460720 in  20.033538s
         Model.first     2614.6 (±19.9%) i/s -      50115 in  20.011464s
Model.all limit(100)       76.3 (±22.3%) i/s -       1442 in  20.023843s
Model.all limit(100) with relationship
                           47.8 (±23.0%) i/s -        904 in  20.096761s
Model.all limit(10,000)
                            0.7 (±0.0%) i/s -         14 in  21.069093s
   Model.named_scope    17040.2 (±25.4%) i/s -     314928 in  19.998347s
        Model.create     1272.4 (±17.8%) i/s -      24800 in  20.110847s
Resource#attributes=    18387.1 (±22.2%) i/s -     348035 in  20.085234s
     Resource#update     1454.2 (±20.3%) i/s -      27800 in  20.080670s
    Resource#destroy     1777.1 (±19.0%) i/s -      34196 in  20.031587s
   Model.transaction     9259.7 (±24.3%) i/s -     172949 in  19.997887s
      Model.find(id)     4052.2 (±18.0%) i/s -      78208 in  20.060394s
   Model.find_by_sql    12611.0 (±17.5%) i/s -     242216 in  20.083935s
           Model.log    69056.2 (±30.5%) i/s -    1225635 in  20.070705s
   AR.execute(query)    14826.3 (±19.0%) i/s -     282987 in  20.058236s
@frodsan
frodsan reviewed Oct 26, 2012
View changes
activerecord/test/cases/finder_test.rb Outdated
@@ -28,7 +28,8 @@ def test_find_with_string
def test_exists
assert Topic.exists?(1)
assert Topic.exists?("1")
assert Topic.exists?(:author_name => "David")
assert Topic.exists?(:title => "The First Topic")

This comment has been minimized.

@frodsan

frodsan Oct 26, 2012 Contributor

Please, use 1.9 hash syntax here.

@frodsan
frodsan reviewed Oct 26, 2012
View changes
activerecord/test/cases/finder_test.rb Outdated
@@ -28,7 +28,8 @@ def test_find_with_string
def test_exists
assert Topic.exists?(1)
assert Topic.exists?("1")
assert Topic.exists?(:author_name => "David")
assert Topic.exists?(:title => "The First Topic")
assert Topic.exists?(:heading => "The First Topic")

This comment has been minimized.

@frodsan

frodsan Oct 26, 2012 Contributor

ditto.

This comment has been minimized.

@chancancode

chancancode Oct 26, 2012 Author Member

Hmm, I think someone else is doing a mass conversion from hash rocket to 1.9 hash syntax? If I switched this one test case it would make it inconsistent with the surrounding test cases, making any mass conversion effort more difficult / error-prone. Or should I change the other test cases in the same file while I'm at that?

Sent from my phone

On 2012-10-25, at 7:01 PM, Francesco Rodríguez notifications@github.com wrote:

In activerecord/test/cases/finder_test.rb:

@@ -28,7 +28,8 @@ def test_find_with_string
def test_exists
assert Topic.exists?(1)
assert Topic.exists?("1")

  • assert Topic.exists?(:author_name => "David")
  • assert Topic.exists?(:title => "The First Topic")
  • assert Topic.exists?(:heading => "The First Topic")
    ditto.


Reply to this email directly or view it on GitHub.

This comment has been minimized.

@frodsan

frodsan Oct 26, 2012 Contributor

We can't do a mass conversion to 1.9 hash syntax in activerecord because it will affect many pull requests. Instead, we encourage people to write the changes or new code in 1.9 syntax if it is master branch. I don't think it will be more difficult, we don't have any agressive conversion plan, but this will make the transition smoother. Thanks.

This comment has been minimized.

@chancancode

chancancode Oct 26, 2012 Author Member

I see! I'll make the changes and rebase

Sent from my phone

On 2012-10-25, at 10:58 PM, Francesco Rodríguez notifications@github.com wrote:

In activerecord/test/cases/finder_test.rb:

@@ -28,7 +28,8 @@ def test_find_with_string
def test_exists
assert Topic.exists?(1)
assert Topic.exists?("1")

  • assert Topic.exists?(:author_name => "David")
  • assert Topic.exists?(:title => "The First Topic")
  • assert Topic.exists?(:heading => "The First Topic")
    We can't do a mass conversion to 1.9 hash syntax in activerecord because it will affect many pull requests. Instead, we encourage people to write the changes or new code in 1.9 syntax. I don't think it will be more difficult, we don't have any agressive conversion plan, but this will make the transition smoother. Thanks.


Reply to this email directly or view it on GitHub.

@steveklabnik
Copy link
Member

steveklabnik commented Nov 28, 2012

@chancancode hey! still interested in keeping this up?

@chancancode
Copy link
Member Author

chancancode commented Nov 29, 2012

Oops I thought I already pushed. Gimme a minute!

@chancancode
Copy link
Member Author

chancancode commented Nov 29, 2012

In the first commit I only changed the code I touched, but that leaves the rest of the file with two different hash syntax styles, so I added a second commit to convert the rest of the file too.

@carlosantoniodasilva
carlosantoniodasilva reviewed Nov 29, 2012
View changes
activerecord/test/cases/finder_test.rb Outdated
@@ -818,13 +819,13 @@ def test_find_one_message_with_custom_primary_key
Toy.primary_key = :name
begin
Toy.find 'Hello World!'
rescue ActiveRecord::RecordNotFound => e
rescue ActiveRecord:RecordNotFound: e

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Nov 29, 2012 Member

That's not valid syntax.

@carlosantoniodasilva
Copy link
Member

carlosantoniodasilva commented Nov 29, 2012

@chancancode thanks for your changes, but I don't think that it's necessary to change the entire file right now. Also it makes harder to see the real diff of your pull request, can you please remove that? I know it's annoying to have the two hash syntaxes there, but we'll have to leave with it for a while. Thanks!

@carlosantoniodasilva
Copy link
Member

carlosantoniodasilva commented Nov 29, 2012

It'd also need a changelog entry :)

@chancancode
Copy link
Member Author

chancancode commented Nov 29, 2012

Done!

Sent from my phone

On 2012-11-28, at 5:55 PM, Carlos Antonio da Silva notifications@github.com wrote:

It'd also need a changelog entry :)


Reply to this email directly or view it on GitHub.

@steveklabnik
Copy link
Member

steveklabnik commented Nov 29, 2012

Already out of date. Damn CHANGELOGs...

@chancancode
Copy link
Member Author

chancancode commented Nov 29, 2012

rebased

On 2012-11-28, at 9:45 PM, Steve Klabnik wrote:

Already out of date. Damn CHANGELOGs...


Reply to this email directly or view it on GitHub.

@senny
Copy link
Member

senny commented Apr 2, 2013

ping...

what is the status on this one?

@senny
Copy link
Member

senny commented Apr 2, 2013

Would this one also fix #6177 ?

@chancancode
Copy link
Member Author

chancancode commented May 1, 2013

@senny once this gets in i'll take a look

When using symbol keys, ActiveRecord will now translate aliased attribute names to the actual column name used in the database:

With the model

  class Topic
    alias_attribute :heading, :title
  end

The call

  Topic.where(heading: 'The First Topic')

should yield the same result as

  Topic.where(title: 'The First Topic')

This also applies to ActiveRecord::Relation::Calculations calls such as `Model.sum(:aliased)` and `Model.pluck(:aliased)`.

This will not work with SQL fragment strings like `Model.sum('DISTINCT aliased')`.

Github #7839

*Godfrey Chan*
@chancancode
Copy link
Member Author

chancancode commented May 1, 2013

@tenderlove this should be good to go! Thanks for the help <333 !

tenderlove added a commit that referenced this pull request May 2, 2013
…n_ar_relation

Handle aliased attributes in AR::Relation
@tenderlove tenderlove merged commit dd1f360 into rails:master May 2, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.