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

fetch ids without instantiating records using pluck #584

Merged
merged 1 commit into from
Aug 29, 2014

Conversation

ashanbrown
Copy link
Contributor

Not only is pluck more efficient for fetching ids, but instantiating a record with only an id can cause problems isfthere are after_find hooks that expect to have all the columns populated. However, pluck was a rails 3.2 addition, so we have to test for it since this gem supports back to rails 3.0.0.

@seuros
Copy link
Collaborator

seuros commented Aug 29, 2014

Nice! 👍 we don't need the test, the gem support 3.2+

Can you add also a note in the change log under 3.4.0 ?
while you are on it, please fix the markdown.

@ashanbrown ashanbrown force-pushed the fix_mysql_record_instantiation branch 2 times, most recently from bfd4123 to 518ed6f Compare August 29, 2014 18:02
@ashanbrown ashanbrown changed the title fetch ids without instantiating records when pluck is available fetch ids without instantiating records using pluck Aug 29, 2014
@ashanbrown
Copy link
Contributor Author

I've amended the commit to always use pluck (for mysql only ... not sure about the other path). I've also updated the changelog and fixed the markdown. The gemspec requires active record >= 3. Maybe it should be updated to >= 3.2?

@@ -12,13 +12,15 @@ As such, a _Feature_ would map to either major or minor. A _bug fix_ to a patch.
* Performance
* Misc

### [3.4.0 / 2014-08-29](Master [changes](https://github.com/mbleigh/acts-as-taggable-on/compare/v3.3.0...v3.4.0)
### [3.4.0 / 2014-08-29](https://github.com/mbleigh/acts-as-taggable-on/compare/v3.3.0...v3.4.0)

* Features
* [@ProGM Support for custom parsers for tags](https://github.com/mbleigh/acts-as-taggable-on/pull/579)
* [@damzcodes #577 Popular feature](https://github.com/mbleigh/acts-as-taggable-on/pull/577)
* Fixes
* [@twalpole Update for rails edge (4.2)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

* [@twalpole Update for rails edge (4.2)](https://github.com/mbleigh/acts-as-taggable-on/pull/583)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you correct this one too ?

@seuros
Copy link
Collaborator

seuros commented Aug 29, 2014

I already fixed the gemspec
plucks works for all databases. Let see how the tests go.

@ashanbrown ashanbrown force-pushed the fix_mysql_record_instantiation branch from 518ed6f to 9616b3f Compare August 29, 2014 18:28
@ashanbrown
Copy link
Contributor Author

I've updated the request to just replace select with pluck. As I read the code, mysql doesn't enjoy the benefit of making a single query, which is unfortunate. I know rails can automatically generated nested queries, but I'm having trouble getting that to work and I don't know what version that started with.

@seuros
Copy link
Collaborator

seuros commented Aug 29, 2014

Look good to me. Thank you. We can optimize/fix later.

@ashanbrown ashanbrown force-pushed the fix_mysql_record_instantiation branch 2 times, most recently from 6c4cb3b to af95d94 Compare August 29, 2014 18:48
@ashanbrown
Copy link
Contributor Author

Got it. Put the link in the changelog.

Regarding the the separate queries, it looks like that's why the code was refactored a bit (#457) and it fixes a problem with pagination. I wonder if we could just check if the query has a limit_value or offset_value, before deciding to make two queries on mysql, although I don't know how confident we can be that a paginator would build up the query using .limit and .offset. Probably ok though. Looks like kaminari and will_paginate both use .limit and .offset

@seuros
Copy link
Collaborator

seuros commented Aug 29, 2014

Core need a full rewrite to use the rails way.

seuros added a commit that referenced this pull request Aug 29, 2014
fetch ids without instantiating records using pluck
@seuros seuros merged commit 19826a3 into mbleigh:master Aug 29, 2014
tekniklr pushed a commit to tekniklr/acts-as-taggable-on that referenced this pull request Mar 19, 2021
…ntiation

fetch ids without instantiating records using pluck
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants