Calculations break with multi-column :select argument #624

Closed
lighthouse-import opened this Issue May 16, 2011 · 9 comments

Projects

None yet

1 participant

@lighthouse-import

Imported from Lighthouse. Original ticket at: http://rails.lighthouseapp.com/projects/8994/tickets/2823
Created by Joshua Krall - 2009-06-22 07:43:09 UTC

I ran into a problem with geokit and will_paginate... that I tracked down to a bad interaction between the :select option and calculation methods.

It boils down to a simple case that looks like this:

Account.scoped(:select => "credit_limit, COS(credit_limit) as cosine_of_credit_limit").count

This case demonstrates what geokit adds to the :select argument, but it also fails if you do something as simple as :select=>'a,b'

I'm uploading a new patch to fix this, by splitting the column on commas and taking the first column for the COUNT() directive. So, :select=>'a,b' turns into "SELECT COUNT(a) as count_a ..."

All tests pass, but take a look and see if you find anything wrong here. Thanks!

Imported from Lighthouse.
Comment by Michael Koziarski - 2009-08-08 01:54:43 UTC

We can't just take the first column specified, while it works for your example it's not 'good enough' in general. Instead we need a way to fix it permanently

I'd question why we're even taking the :select key from find scopes. I've attached a patch that takes it from :count scopes instead.

I've attached a patch for this and would appreciate your feedback. Your fix would look like:

    Account.with_scope(:find=>{:select=>"COS(id), SIN(id)"}, :count=>{:select=>"id"}) do

Imported from Lighthouse.
Comment by Jeremy Kemper - 2009-08-08 23:35:20 UTC

This is messy indeed. I'd almost see an error raised than let this slip through or get hacked around.

Michael, that fix works for count, but not the other calculations.

Imported from Lighthouse.
Comment by Michael Koziarski - 2009-08-08 23:37:19 UTC

OK, I agree on the error case, let's just raise an exception in the
event that the :select scope doesn't map onto the column_name

Imported from Lighthouse.
Comment by Jeremy Kemper - 2010-08-30 03:10:30 UTC

[bulk edit]

Imported from Lighthouse.
Comment by Jeremy Kemper - 2010-10-15 22:01:33 UTC

[bulk edit]

Imported from Lighthouse.
Comment by Santiago Pastorino - 2010-11-15 21:15:11 UTC

[bulk edit]

Imported from Lighthouse.
Comment by Santiago Pastorino - 2011-02-12 21:58:20 UTC

[bulk edit]

Imported from Lighthouse.
Comment by Santiago Pastorino - 2011-02-27 03:15:37 UTC

[bulk edit]

Attachments saved to Gist: http://gist.github.com/971638

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment