Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Composite keys implementation #452

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
2 participants

Concerning #451 Added some specing to some changes. Couldn't exactly see how could I correctly specced the psych_ext:102 update, since it is enclosed under a huge case statement which code routine seems to be the same implemented in the active_record file. As suggestion I would isolate this routine in some method and call it in both places, this way I could spec the routine in isolation. But, as stated before, goal is here to provide some sort of multiple primary keys support without breaking the main standard case, which is numerical and incremental id.

I've thought about the performance gap discussion and (unless it is possible with currying, which I don't think it is, but one could still investigate) I don't think it is a dealbreaker. In the standard case scenario (one primary key) we are talking about allocating memory for the new array, whereas its values should be the same ( try this: a = {:a => 1} ; a[:a].object_id == a.values_at(:a).first.object_id ). I don't really see this overhead becoming such a nuisance. I even thought about calling conditionally calling the values_at on cases where the id would be more than one, but even then one could have the same discussion about having an additional cpu cycle :) .

As a last request: if for some other reason you decide not to accept this pull request, could you at least follow the above mentioned suggestion and isolate the ActiveRecord find calls into one function? Because if delayedjob doesn't support composite primary keys, I would still like to have the option to cleanly patch this functionality. If it is all in one place, it will be patched much more easier.

Tiago Cardoso added some commits Nov 15, 2012

Tiago Cardoso payload object's object is no prepared to respond to the possibility …
…of a composite key implementation while still working for the standard of one primary key only (this was a fix to make djobs work with the gem composite_primary_keys)
73a2b96
Tiago Cardoso corrected another implementation which does not get along with compos…
…ite keys (see pull request 450)
949c3f6
Tiago Cardoso corrected an unfortunate leftover 64ab15b
Tiago Cardoso rewrote the exception error a bit to include possibility of multiple …
…keys
5206810
Tiago Cardoso added yaml_new spec to test if it would work for a primary key compos…
…ites solution
f88ff56
Tiago Cardoso added comment concerning a 5 line snippet which seems to be copy-past…
…ed in 2 locations (how to best solve this?)
ab5d11c
Owner

albus522 commented Nov 16, 2012

You can also tell bundler to use your repo in your Gemfile

gem "delayed_job", :git => "https://github.com/TiagoCardoso1983/delayed_job.git", :branch => "composite_keys"

yup, that's what I'm doing already. I will just have to patch delayed jobs each time you update the gem.

@HoneyryderChuck HoneyryderChuck referenced this pull request in composite-primary-keys/composite_primary_keys Nov 22, 2012

Merged

Concerning issue 129 #131

@albus522 albus522 closed this Sep 24, 2014

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