I can submit a patch, but wanted to confirm I''m looking at this
right...
The docs indicate that if you specify a has_many association with
:finder_sql, but no :counter_sql, it constructs the appropriate counter
sql by substituting the SELECT clause.
But has_many_association.rb doesn''t seem to do that -- it just passes
Base#count_by_sql the finder_sql, which doesn''t work so good.
def count(runtime_conditions = nil)
if @reflection.options[:counter_sql]
@reflection.klass.count_by_sql(@counter_sql)
elsif @reflection.options[:finder_sql]
@reflection.klass.count_by_sql(@finder_sql)
Am I missing something, or should I submit a patch?
Michael, construct_sql is executed in the initialize method of the association and it handles the substitution of the sql. Cody On 1/19/06, Michael Schoen <schoenm@earthlink.net> wrote:> I can submit a patch, but wanted to confirm I''m looking at this right... > > The docs indicate that if you specify a has_many association with > :finder_sql, but no :counter_sql, it constructs the appropriate counter > sql by substituting the SELECT clause. > > But has_many_association.rb doesn''t seem to do that -- it just passes > Base#count_by_sql the finder_sql, which doesn''t work so good. > > def count(runtime_conditions = nil) > if @reflection.options[:counter_sql] > @reflection.klass.count_by_sql(@counter_sql) > elsif @reflection.options[:finder_sql] > @reflection.klass.count_by_sql(@finder_sql) > > Am I missing something, or should I submit a patch? > > _______________________________________________ > Rails-core mailing list > Rails-core@lists.rubyonrails.org > http://lists.rubyonrails.org/mailman/listinfo/rails-core >
> construct_sql is executed in the initialize method of the association > and it handles the substitution of the sql.Ah, thanks. So the bug is that it doesn''t do the right thing if the finder_sql has newlines (the regex needs to be modified). I''ll submit a patch.
>> construct_sql is executed in the initialize method of the association >> and it handles the substitution of the sql. > > Ah, thanks. So the bug is that it doesn''t do the right thing if the > finder_sql has newlines (the regex needs to be modified). I''ll submit a > patch.patch submitted as: http://dev.rubyonrails.org/attachment/ticket/3540
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Jan 19, 2006, at 9:30 AM, Michael Schoen wrote:>> construct_sql is executed in the initialize method of the association >> and it handles the substitution of the sql. > > Ah, thanks. So the bug is that it doesn''t do the right thing if the > finder_sql has newlines (the regex needs to be modified). I''ll > submit a patch.Nice catch. jeremy -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.2 (Darwin) iD8DBQFDz/bVAQHALep9HFYRAvjVAJ4ldM8k7scs38737DrKkKtdEejOsACcC8uY g6V+LO2Q/8/+j7B1cMDmYrU=7unY -----END PGP SIGNATURE-----
>>> construct_sql is executed in the initialize method of the association >>> and it handles the substitution of the sql. >> >> Ah, thanks. So the bug is that it doesn''t do the right thing if the >> finder_sql has newlines (the regex needs to be modified). I''ll submit >> a patch. > > Nice catch.Um, so even with this patch I''m not totally happy, but the fix is Oracle-specific, so not sure if it belongs in the core. When I''m using finder_sql, it''s often because I''ve needed to supply a sql hint to get the right performance (isn''t Oracle grand). So my finder_sql is something like... "SELECT /*+ INDEX(FOO$I) */ * FROM ..." Unfortunately, when this gets auto-converted into counter_sql, the hint gets wiped away as well. And if I needed a hint for the find, I also need a hint for the count. Would you be open to a patch that made the regexp uglier but didn''t kill the sql hint? Or should I just specify both finder_sql and counter_sql?
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Jan 19, 2006, at 12:59 PM, Michael Schoen wrote:>>>> construct_sql is executed in the initialize method of the >>>> association >>>> and it handles the substitution of the sql. >>> >>> Ah, thanks. So the bug is that it doesn''t do the right thing if >>> the finder_sql has newlines (the regex needs to be modified). >>> I''ll submit a patch. >> Nice catch. > > Um, so even with this patch I''m not totally happy, but the fix is > Oracle-specific, so not sure if it belongs in the core. > > When I''m using finder_sql, it''s often because I''ve needed to supply > a sql hint to get the right performance (isn''t Oracle grand). So my > finder_sql is something like... > > "SELECT /*+ INDEX(FOO$I) */ * FROM ..." > > Unfortunately, when this gets auto-converted into counter_sql, the > hint gets wiped away as well. And if I needed a hint for the find, > I also need a hint for the count. > > Would you be open to a patch that made the regexp uglier but didn''t > kill the sql hint? Or should I just specify both finder_sql and > counter_sql?Sure. I think we should try to preserve SQL hints if only for the next programmer to come along with the notion that Rails will eat his hairy custom SELECT and grin right back. Plus, MySQL has a similar hint syntax, and every patch lends a welcome bit of polish.. Thanks, jeremy -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.2 (Darwin) iD8DBQFD0A1ZAQHALep9HFYRAr9OAKCnngbZ+3TA6n3oa9GgYOuS4b5GHgCcDsSw Sqgj4NzHCNMd3zeXlr+pTpE=O++C -----END PGP SIGNATURE-----
>> Would you be open to a patch that made the regexp uglier but didn''t >> kill the sql hint? Or should I just specify both finder_sql and >> counter_sql? > > Sure. I think we should try to preserve SQL hints if only for the next > programmer to come along with the notion that Rails will eat his hairy > custom SELECT and grin right back. Plus, MySQL has a similar hint > syntax, and every patch lends a welcome bit of polish..Ok, I''ve updated the ticket w/ a revised patch that handles this situation.