Allow find_in_batches to use :order, :limit, and :offset

Seems a lot like this one:

http://rails.lighthouseapp.com/p...

Even with the order, offset, and limit stuff set up you still have the problem of 'missing' records that are being inserted while find_in_batches is doing it's thing.

Not to sound like a jerk, but even though this sounds nice to have, I can't think of a place where I would need the batch done in a certain order.

Ryan, you could need to do some report, most recent first, ordered by surname, whatever.

In my view this method should allow the user to do that. It is the user responsability to know whether it is safe, or to decide whether a missing record is a big deal.

In addition, AR is a standalone ORM, you could even be certain that no other program is touching the database.

I guess when I think of batch processing I think of sending notifications and carrying out async processes. Things that inherently do not require ordering. If you're doing a report that requires ordering, sort the report when you're done.

I really have an issue with the second line. find returns all records matching the query. find_in_batches shouldn't have different behavior.

I dont understand part three. You could be certain that no other program is touching the database, but with cronjobs and multi-mongrel setups it's pretty unlikely that would ever be the case.

Data cleanup or munging is an example where record order might make sense. You may need to process all records, but some records are more active than others. Some may be months old and no one cares when they are fixed. Allowing the order to be set means you can handle this case.

By selecting all the ids first, you can at least be certain the records you are iterating over. The other method will result in an indeterminate number of records processed if inserts are happening. If the iteration loop updates other records of the same class, it could even change the results since updates to other records may affect whether or not they match the conditions.

To clarify just a little bit, I think the functionality here should be equivalent:

@@@ruby Post.all(options).each do |post| # do something end

and

@@@ruby
Post.each(options) do |post|
  # do something
end

By allowing find_in_batches to select new records after it starts, the two statements iterate over different sets of data. With the patch, they can take the same options and will always iterate over the same set.

Sorry, that should have been (my formatting skills are off)

Post.all(options).each do |post|
  # do something end
and

Post.each(options) do |post|
  # do something
end

Ryan, I don't think telling the user to order his report makes any sense. Just give him the API to let SQL do his job.

cronjobs have ways to control they run only once at a time, and in the end is the responsability of the user to decide whether it is safe for him or not.

It is certainly not the case that it is always nonsense or always dangerous. You just have no idea a priori, the user has.

Well it is clear that I am +1 here.

One other thing I'd like to add is that this code makes the functionality more consistent with the behavior of database cursors where once a cursor is opened (at least in the implementations I'm familiar with), the data set is static and won't change as you fetch the rows. Cursors don't prohibit you from specifying an ORDER BY clause.

If we can make the functionality closer to a concept people already understand, I thinking that's a good thing.

If I understand what you just said, then I didn't know that, and I may now agree with you. I'll change my -1 to a 0 while I mull over it :-)

(from [f4d174b211effd4cd510c1578bdc391852f4d375]) making rake:migrate VERSION=0 a noop called in succession. [#2137 Allow find_in_batches to use :order, :limit, and :offset state:resolved]

Signed-off-by: José Valim jose.valim@gmail.com
http://github.com/rails/rails/commit/f4d174b211effd4cd510c1578bdc39...

Had to rework the functionality a bit, ported test provided in the previous patch and added some new ones. Also made a new example in the doc comments for usage of :order and :limit.
This all was done against the current master (patch should apply).