[PATCH] Dont use find in AR::Base#exists?

I'm definitely for this idea. As a suggestion, I've submitted your path without the use of the instance variable. This is largely a stylistic choice, but there's no real sense in creating a variable that only used once.

Either way, I've verified and give you my +1.

Same as the original patch, but without the 'sql' variable.

I'm also a +1 on this - I can't replicate Josh's behaviour of find() being faster than going straight to the connection.

And... I'm also submitting an alternate patch - there's no point in doing select_all (which hashifies the resulting row) when select_value is pretty much exactly what you want.

Again, same as original, bu also eliminates sql variable and uses select_value connection call

I'm going to specifically direct my +1 to protocool's patch. I think this one hits the nail on the head.

Replying to protocool:

I'm also a +1 on this - I can't replicate Josh's behaviour of find() being faster than going straight to the connection. And... I'm also submitting an alternate patch - there's no point in doing select_all (which hashifies the resulting row) when select_value is pretty much exactly what you want.

D'oh! Should have thought of that :-)

Don't pay too much mind to my claims about those measurements. It was a while ago, I don't remember exactly what I measured, and I can't find the data anymore.

#select_value calls #select_all, then extracts the value from the hash. It doesn't save you any work, but actually adds a tiny bit more effort.

Josh is right... I stupidly assumed that select_value would delegate to select_values - but it does not.

IMHO it should... so rather than change the patch to be:

!select_values(construct_finder_sql(...)).empty? 

I would argue that select_value should be changed at some point in the not-to-distant future :-)

I've tried your recommendation, but found that there's a failure on the very first test for exists?. I.e. Object.exists?(1) fails (line 25, finder_test.rb).

Replying to protocool:

Josh is right... I stupidly assumed that select_value would delegate to select_values - but it does not. IMHO it should... so rather than change the patch to be: {{{ !select_values(construct_finder_sql(...)).empty? }}} I would argue that select_value should be changed at some point in the not-to-distant future :-)

Do we have some sort of consensus then ?

Replying to fcheung:

Do we have some sort of consensus then ?

In light of hasmanyjosh's response, I'd like to nominate my patch as the consensus. It's merely fcheung's patch without the variable. It's a little cleaner than his original submission without the introduced variable.

There's definitely a difference between select_all and select_values though, although I don't know enough to say which one is preferable (and I expect that it doesn't make any difference in this case)

Fred,

(perhaps it's an oversight but) select_values is not query-cached, while select_all (and subsequently select_value) is.

Personally I prefer seeing something explicit like select_value but either that or select_all will be consistent with query caching behavior seen with the current exists? implementation.

In that case absolutely, jamesh's patch all the way! Now to find a missing +1 ...

+1 Seems reasonable to me

+1

(In [8531]) Optimize ActiveRecord::Base#exists? to use #select_all instead of #find. Closes #10605 [jamesh, fcheung, protocool]