Fixes bug in ActiveRecord::QueryMethods, #1697 #2775

Merged
merged 1 commit into from Sep 1, 2011

3 participants

@jaw6

Replace split on comma with a regexp that will reverse all ASC/DESC specifically

#1697 says it was fixed, but that fix was reverted in 971a74b. It seems likely 18d307e was failing on Travis because MAX (the function used in the test scenario) does not actually accept multiple arguments on MySQL.

@jaw6 jaw6 Fixes bug in ActiveRecord::QueryMethods, #1697
Replace split on comma with a regexp that will reverse all ASC/DESC specifically
0df27c9
@titanous

It looks like this patch is also ~37% faster than the previous implementation on Ruby 1.9:

ruby 1.9.3dev (2011-07-31 revision 32789) [x86_64-darwin11.0.0]
Rehearsal ---------------------------------------
old   0.080000   0.000000   0.080000 (  0.079311)
new   0.050000   0.000000   0.050000 (  0.047826)
------------------------------ total: 0.130000sec

          user     system      total        real
old   0.070000   0.000000   0.070000 (  0.077106)
new   0.040000   0.000000   0.040000 (  0.046636)
ruby 1.9.2p290 (2011-07-09 revision 32553) [x86_64-darwin11.0.0]
Rehearsal ---------------------------------------
old   0.080000   0.000000   0.080000 (  0.080773)
new   0.050000   0.000000   0.050000 (  0.049190)
------------------------------ total: 0.130000sec

          user     system      total        real
old   0.080000   0.000000   0.080000 (  0.080600)
new   0.040000   0.000000   0.040000 (  0.048524)
ruby 1.8.7 (2011-02-18 patchlevel 334) [i686-darwin11.0.0], MBARI 0x6770, Ruby Enterprise Edition 2011.03
Rehearsal ---------------------------------------
old   0.100000   0.000000   0.100000 (  0.100959)
new   0.090000   0.000000   0.090000 (  0.096180)
------------------------------ total: 0.190000sec

          user     system      total        real
old   0.100000   0.000000   0.100000 (  0.099736)
new   0.100000   0.000000   0.100000 (  0.093022)

Benchmark source

@tenderlove tenderlove merged commit 90248d2 into rails:master Sep 1, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment