Forum: Ruby on Rails Reduce Number of Queries When Using ActiveRecord

Announcement (2017-05-07): www.ruby-forum.com is now read-only since I unfortunately do not have the time to support and maintain the forum any more. Please see rubyonrails.org/community and ruby-lang.org/en/community for other Rails- und Ruby-related community platforms.
61b70c2f195b0e669a8e25000148d9dd?d=identicon&s=25 Eden Brandeis (Guest)
on 2006-04-11 20:14
(Received via mailing list)
I have a controller method called update_all that grabs parameters for
about
30 form fields and saves them to the database.  My current code looks to
see
if there is a string in params[] for each form field (two fields for
each
SelfEvaluationItem) and saves the updated self_evaluation_answer if a
string
is available.  Since this is iterated code, I am almost certain that
this
generates 30 or so SQL queries.

Is there a way to build up all of the self_evaluation_answers into a
larger
unit (model array?) and then save them all at once?  The goal of course
would be to reduce the number of queries to just 1.  I guess the
alternate
would be to build a SQL query to do this and execute it, but I prefer to
work in Ruby and Rails if possible.

Any help is very much appreciated!

I have included the source for reference below:

  def update_all
    for item in SelfEvaluationItem.find_all
      id = params[:self_evaluation_answer][item.id.to_s][:id]
      comment = params[:self_evaluation_answer][item.id.to_s][:comment]
      response =
params[:self_evaluation_answer][item.id.to_s][:response]
      if !comment.empty? or !response.empty?
        @self_evaluation_answer = SelfEvaluationAnswer.find(id)
        @self_evaluation_answer.comment = comment unless comment.empty?
        @self_evaluation_answer.response = response unless
response.empty?
        @self_evaluation_answer.save
      end
    end
    flash[:notice] = "Save Completed"
  end
59ea1b450935b9d70abfec4186b7a4d5?d=identicon&s=25 Jeff Coleman (progressions)
on 2006-04-11 20:41
Eden Brandeis wrote:
> I have a controller method called update_all that grabs parameters for
> about
> 30 form fields and saves them to the database.  My current code looks to
> see
> if there is a string in params[] for each form field (two fields for
> each
> SelfEvaluationItem) and saves the updated self_evaluation_answer if a
> string
> is available.  Since this is iterated code, I am almost certain that
> this
> generates 30 or so SQL queries.
>
> Is there a way to build up all of the self_evaluation_answers into a
> larger
> unit (model array?) and then save them all at once?  The goal of course
> would be to reduce the number of queries to just 1.  I guess the
> alternate
> would be to build a SQL query to do this and execute it, but I prefer to
> work in Ruby and Rails if possible.
>
> Any help is very much appreciated!
>
> I have included the source for reference below:
>
>   def update_all
>     for item in SelfEvaluationItem.find_all
>       id = params[:self_evaluation_answer][item.id.to_s][:id]
>       comment = params[:self_evaluation_answer][item.id.to_s][:comment]
>       response =
> params[:self_evaluation_answer][item.id.to_s][:response]
>       if !comment.empty? or !response.empty?
>         @self_evaluation_answer = SelfEvaluationAnswer.find(id)
>         @self_evaluation_answer.comment = comment unless comment.empty?
>         @self_evaluation_answer.response = response unless
> response.empty?
>         @self_evaluation_answer.save
>       end
>     end
>     flash[:notice] = "Save Completed"
>   end

If I'm reading this correctly, what you're doing is cycling through
every single item in your SelfEvaluationItem table to compare each item
against the entries in params[:self_evaluation_answer].  Wouldn't it be
more efficient to cycle through params[:self_evaluation_answer] and
refer to evaluation items based on the results?

For example:

def update_all
  params[:self_evaluation_answer].each do |answer|
    answer.each_key do |id_from_answer|
      item = SelfEvaluationAnswer.find(id_from_answer.to_i)
      new_comment = answer[id_from_answer][:comment]
      new_response = answer[id_from_answer][:response]
      unless new_comment.empty? and new_response.empty?
        @self_evaluation_answer = item
        @self_evaluation_answer.comment = new_comment unless
new_comment.empty?
        @self_evaluation_answer.response = new_response unless
new_response.empty?
        @self_evaluation_answer.save
      end
    end
  end
  flash[:notice] = "Save Completed"
end

Again I'm hoping I have your read your code correctly, but what I
attemped to do here is to loop through the
params[:self_evaluation_answer] hash, which seems to have keys that are
String versions of your SelfEvaluationItem primary key ids.

So we take each key and use it to find the SelfEvaluationItemAnswer
record which matches that id.  Then if there's a new comment or response
we add them to the record, and save it.

I added descriptive flash messages indicating when an error prevented
the record from saving or when the record didn't need to be updated.

I'm not sure this really addresses the gist of your question, but I
thought I'd offer it as an alternative that might be slightly more
efficient.  Feel free to disregard if this doesn't suit your needs for
whatever reason.

Jeff Coleman
61b70c2f195b0e669a8e25000148d9dd?d=identicon&s=25 Eden Brandeis (Guest)
on 2006-04-11 21:42
(Received via mailing list)
Jeff,

Thank you for the suggestions.  I really like your approach better than
mine
for aesthetic reasons, but it seems that you have introduced an extra
SQL
query with each iteration:

item = SelfEvaluationAnswer.find(id_from_answer.to_i)

Since the form will return a valid key for every field (even when left
blank), I think this is guaranteed to more than double the number of
queries
versus the code I have now.  Currently I only generate a query before
entering the loop and once for each form item that has content (blank
form
items don't generate and Update query).

To be honest, it is a little early in the development of this
application to
worry much about optimization, but I realized that using SQL I could
probably do all the UPDATE queries in my loop in a single statement and
thought I could nip this problem in the bud while still using
ActiveRecord.

Thanks again for your contribution.  I will probably at least reorganize
some of my code based on your suggestions.

Eden
59ea1b450935b9d70abfec4186b7a4d5?d=identicon&s=25 Jeff Coleman (progressions)
on 2006-04-11 22:33
Eden Brandeis wrote:
> Jeff,
>
> Thank you for the suggestions.  I really like your approach better than
> mine
> for aesthetic reasons, but it seems that you have introduced an extra
> SQL
> query with each iteration:
>
> item = SelfEvaluationAnswer.find(id_from_answer.to_i)
>
> Since the form will return a valid key for every field (even when left
> blank), I think this is guaranteed to more than double the number of
> queries
> versus the code I have now.  Currently I only generate a query before
> entering the loop and once for each form item that has content (blank
> form
> items don't generate and Update query).
>
> To be honest, it is a little early in the development of this
> application to
> worry much about optimization, but I realized that using SQL I could
> probably do all the UPDATE queries in my loop in a single statement and
> thought I could nip this problem in the bud while still using
> ActiveRecord.
>
> Thanks again for your contribution.  I will probably at least reorganize
> some of my code based on your suggestions.
>
> Eden

Eden,

I see what you mean abou the SQL query--it was my mistake, but the line

item = SelfEvaluationAnswer.find(id_from_answer.to_i)

could be placed at the top of the conditional block, the way you had it
in your original.  That would at least make it equal to your original
version in SQL calls, if we exclude your original find_all.

I'm not a SQL expert but I've been thinking about how to optimize your
overall number of SQL queries.  I'm guessing with enough thought, that a
sufficient :condition statement could be amalgamated from your
parameters that would do what you're looking for.

I'll let you know if anything comes up.

Jeff Coleman
59ea1b450935b9d70abfec4186b7a4d5?d=identicon&s=25 Jeff Coleman (progressions)
on 2006-04-11 23:02
Eden Brandeis wrote:

> To be honest, it is a little early in the development of this
> application to
> worry much about optimization, but I realized that using SQL I could
> probably do all the UPDATE queries in my loop in a single statement and
> thought I could nip this problem in the bud while still using
> ActiveRecord.

Eden,

Try this.  What I've done is basically make a new updated_answers Hash
using only the answers which have had comment or response updated.  I've
converted the index from a string to an integer so we can use it to find
all the related SelfEvaluationItem records.

I believe you'll still need to cycle through the array and save each
record individually, but at least the query can be reduced to a single
statement.


def update_all
  updated_answers = {}
  params[:self_evaluation_answer].each do |answer|
    answer.each_key do |id_from_answer|
      new_comment = answer[id_from_answer][:comment]
      new_response = answer[id_from_answer][:response]

      updated_answers[id_from_answer.to_i] = answer[id_from_answer]
unless (new_comment.empty? and new_response.empty?)
    end
  end
  updated_items = SelfEvaluationAnswer.find(updated_answers.keys)
  updated_items.each do |item|
    new_comment = updated_answers[item.id][:comment]
    new_response = updated_answers[item.id][:response]

    item.comment = new_comment unless new_comment.empty?
    item.response = new_response unless new_response.empty?
    item.save
  end
  flash[:notice] = "Save completed."
end

There may be a more efficient way of doing this, but this is what I've
come up with.  Hope it helps you some,

Jeff Coleman
61b70c2f195b0e669a8e25000148d9dd?d=identicon&s=25 Eden Brandeis (Guest)
on 2006-04-12 22:34
(Received via mailing list)
Thank you for your help Jeff.  I will look at reducing the number of
queries
again when I start optimizing the application.
61b70c2f195b0e669a8e25000148d9dd?d=identicon&s=25 Eden Brandeis (Guest)
on 2006-04-16 17:09
(Received via mailing list)
Jeff and others following this thread,

I found what looks like a good solution to this problem over at Rails
Weenie:

http://rails.techno-weenie.net/tip/2006/3/15/speed...

I think if I wrap my updates in a transaction, then I should get a
single
query to the db.  I will need to do some testing to make sure that I get
the
expected results if the transaction fails, but I think this is a
possible
solution to my original question (reduce queries from 32->2).

Thanks again,

Eden
This topic is locked and can not be replied to.