Key abbreviation #353

Closed
wants to merge 10 commits into
from

Conversation

Projects
None yet
8 participants

spinosa commented Nov 23, 2011

Thanks for taking the time to look over the first pull request. Appreciate and agree with the advice.

The key option is now :abbr and I've overrode keys#to_mongo instead of keys#attributes(true/false).

Thoughts?

...because Mongo stores the entire key with each document, and that's terrible with large datasets.

This patch allows you to add the :abbr option to any key which will define what string is actually stored in Mongo as the key. This is done transparently, so your code can continue to use the human-readable long key while persisting in mongo with the (presumably shorter) abbreviation.

For example:

class User
  include MongoMapper::Document
  key :email_address, String, :abbr => :e
end

u = User.find_by_email_address("spinosa@gmail.com")
u.email_address = "dan@shelby.tv" 
u.save

#user in db looks like {_id: 'NormalObjectId', e: 'dan@shelby.tv' }

Wouldn't it be easier to just iterate through keys and create the hash. Then you never have to set the instance var when inherited and such.

Owner

spinosa replied Nov 30, 2011

I was following the pattern already in place... Since we already receive each key (just below) and build the @Keys hash at that time, I figured it was easier to build & understand the abbreviation hash if it followed in the same footsteps (hence dup when inherited).

If not, I'm curious how/when you would iterate the keys to build the abbreviation hash?

You already have the key at this point. I don't see any purpose to do another method call just to get the abbreviation. I would add a method to key that is persisted_name or something that returns abbreviation if there is one otherwise just returns the key name. Then you can just do key.persisted_name instead of doing a method call and such.

@jnunemaker Have you all decided whether you're going to accept this pull request? Seeing that MongoDB doesn't have an easy way to rename keys other than iterating over all the documents in a collection and updating, this feature would make renaming keys much more painless.

This way, if developers decide another name is better for a key, they can just change it and leave the abbreviation the same. MongoDB doesn't need migrations to add/remove "fields", but renames are hard compared to databases with schemas. This would make actually renaming the keys in the database unnecessary.

spinosa commented Mar 14, 2012

@danielberkompas check out https://github.com/spinosa/mongomapper/tree/key_abbreviation

Even though my feature branch hasn't made it into master, I merged mongomapper/master back into there about a month ago, so it's fairly up to date.

fyi: have also been using this in production w/o any problems

Contributor

jnunemaker commented Mar 14, 2012

Nice. I missed the new commits. We'll take a look

@brianhempel brianhempel commented on the diff Mar 14, 2012

lib/mongo_mapper/plugins/keys.rb
@@ -204,7 +226,25 @@ def attributes
end
end
end
- alias :to_mongo :attributes
@brianhempel

brianhempel Mar 14, 2012

Contributor

Is #attributes still around?

@brianhempel

brianhempel Mar 14, 2012

Contributor

nm I was confused, #attributes is still around.

Contributor

brianhempel commented Mar 14, 2012

@jnunemaker is this the kind of translation layer you were looking for in #397 that we could leverage for, say, converting date queries?

(On that note, I can't think of a case where using to_mongo to convert a query value would cause problems, unless you have legacy data in the database with weird types. Similarly, I can't think of a case where you'd want a query converter more fancy than to_mongo. If that's true, I can work on the query value date translation thing once this is pulled...)

@brianhempel brianhempel commented on an outdated diff Mar 14, 2012

lib/mongo_mapper/plugins/querying/decorator.rb
@@ -20,7 +20,21 @@ def find!(*ids)
end
end
end
-
+
+ %w(first last all where find_each exist? exists? count).each do |meth|
@brianhempel

brianhempel Mar 14, 2012

Contributor

I'm not sure this list is complete (paginate,fine_one, distinct, amend, filter, ignore, only, and order also take criteria and/or field names) . If you override Pluck::Query#amend you'll get several of these methods for free, but there needs to be a more robust way to get all of the plucky methods...something like the Plucky::Methods (which I need to update anyway for #387) but one for methods that take criteria and another for methods that take options ... or have abbreviations handled inside of plucky. Plucky already handles aliasing :id to :_id (though in two places in the code, once for criteria and once for options.)

Thoughts, @jnunemaker @bkeepers ?

Contributor

bkeepers commented Mar 30, 2012

I would love to get this merged. My one concern is the rewriting of query options seems incomplete. It doesn't override all of the plucky methods, and it won't work with embedded docs.

I would be tempted to just remove the rewriting of keys, and force people to use the abbreviations when querying. If you're using scopes or defining custom model methods for querying, it won't be that weird.

Contributor

jnunemaker commented Mar 30, 2012

Agreed.

On Mar 30, 2012, at 4:32 PM, Brandon Keepersreply@reply.github.com wrote:

I would love to get this merged. My one concern is the rewriting of query options seems incomplete. It doesn't override all of the plucky methods, and it won't work with embedded docs.

I would be tempted to just remove the rewriting of keys, and force people to use the abbreviations when querying. If you're using scopes or defining custom model methods for querying, it won't be that weird.


Reply to this email directly or view it on GitHub:
jnunemaker#353 (comment)

Contributor

jnunemaker commented Apr 16, 2012

If we pull the rewriting of keys on querying and change to just have a keys class/instance method I'll happily pull this. I'd just make a Key#persisted_name method, as I did in toystore, and use that rather than have two separate locations for keys (keys and keys_by_abbr).

shreeve commented Dec 7, 2012

Any update on this?

@spinosa spinosa Merge branch 'master' into key_abbreviation
Conflicts:
	lib/mongo_mapper/plugins/keys.rb
	lib/mongo_mapper/plugins/keys/key.rb
6239dd5

+1 for this. In very large collections the key lengths really get to be important.
Abbreviated keys in the data would be very useful.

Owner

cheald commented Jul 7, 2013

I took a stab at this one --

https://github.com/cheald/mongomapper/compare/alias_keys

I'm not doing any key translation, but I provide a Model::persisted_name (aliased to abbr) method that allows people to use full field names in queries:

class Model
  key :full_field_name, String, :abbr => :ffn
end

# These two are functionally equivalent:
Model.where(Model.abbr(:full_field_name) => "whatever")
Model.where(:ffn => "whatever")

(This is somewhat shades of Arel's schema reflection)

Association names aren't aliasable, though foreign keys obviously are. Seems to work nicely, and it's a very small change from current master. Thoughts?

Contributor

jnunemaker commented Jul 8, 2013

@cheald your stuff looks fine to me.

cheald closed this in f001de0 Jul 8, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment