Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Make `ActiveRecord::Batches#find_each` to not return `self`. #1300

Closed
wants to merge 4 commits into from

4 participants

@knapo

This caused that find_each may produce extra db call taking all the records from db, return array with all records, and was less efficient than ActiveRecord::Base#all. There is no sense to keep scope with this method, as it expects block and having further scopes after find_each is not possible.

eg. (notice last query)

ruby-1.9.2-p180 :016 > User.find_each{|u| }
  User Load (93.8ms)  SELECT `users`.* FROM `users` WHERE (`users`.`id` >= 0) ORDER BY `users`.`id` ASC LIMIT 1000
  User Load (175.0ms)  SELECT `users`.* FROM `users` WHERE (`users`.`id` > 1000) ORDER BY `users`.`id` ASC LIMIT 1000
  User Load (0.5ms)  SELECT `users`.* FROM `users` WHERE (`users`.`id` > 2000) ORDER BY `users`.`id` ASC LIMIT 1000
  User Load (308.2ms)  SELECT `users`.* FROM `users` 
knapo added some commits
@knapo knapo Make `ActiveRecord::Batches#find_each` to not return `self`.
This caused that `find_each` was producing extra db call taking all the records from db, and was less efficient than `ActiveRecord::Base#all`.
9ddf699
@knapo knapo Merge remote branch 'upstream/master'
9561094
@knapo

Sorry for merge commit - did merge instead of pull --rebase :/

@dmathieu
Collaborator

Test ?

@knapo

If you're not able to combine commits (dunno if it's possible with pull request), I can provide a patch with both of them.

activerecord/test/cases/batches_test.rb
@@ -18,6 +18,13 @@ class EachTest < ActiveRecord::TestCase
end
end
+ def test_each_should_not_return_query_chain_and_excecute_only_one_query
@dasch
dasch added a note

Typo: should be "execute", not "excecute".

@knapo
knapo added a note

thanks dasch.

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

ping. can you merge this?

@spastorino
Owner

Can you first squash all the commits into one?, thanks

@knapo

@spastorino - I created new pull request with squashed commits: #1997

@knapo knapo closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 25, 2011
  1. @knapo

    Make `ActiveRecord::Batches#find_each` to not return `self`.

    knapo authored
    This caused that `find_each` was producing extra db call taking all the records from db, and was less efficient than `ActiveRecord::Base#all`.
  2. @knapo
  3. @knapo
Commits on May 26, 2011
  1. @knapo

    Fix typo in test case name

    knapo authored
This page is out of date. Refresh to see the latest.
View
2  activerecord/lib/active_record/relation/batches.rb
@@ -20,8 +20,6 @@ def find_each(options = {})
find_in_batches(options) do |records|
records.each { |record| yield record }
end
-
- self
end
# Yields each batch of records that was found by the find +options+ as
View
7 activerecord/test/cases/batches_test.rb
@@ -18,6 +18,13 @@ def test_each_should_excecute_one_query_per_batch
end
end
+ def test_each_should_not_return_query_chain_and_execute_only_one_query
+ assert_queries(1) do
+ result = Post.find_each(:batch_size => 100000){ }
+ assert_nil result
+ end
+ end
+
def test_each_should_raise_if_select_is_set_without_id
assert_raise(RuntimeError) do
Post.find_each(:select => :title, :batch_size => 1) { |post| post }
Something went wrong with that request. Please try again.