Fixes Issue 98 for me #99

Merged
merged 1 commit into from May 5, 2012

Conversation

Projects
None yet
2 participants
@codeodor
Contributor

codeodor commented Apr 16, 2012

After some further research on issue 98 (#98), it looks like it's just doing the same thing Rails' version is doing, but adding some special cases for CPK.

However, I didn't find out how/why Rails was not using the subquery.

Instead, I just created a case like "if we're only selecting 1 column, there's no need for the subquery, so don't use it"

Not sure if you're interested in including that or not, but in case you are, I've submitted this pull request.

I don't like to hack around this kind of thing -- it makes the code uglier and harder to follow -- but if everyone else is seeing 5-10 seconds to count records using the subquery vs. almost instantaneous results without the subquery, then I guess it's useful.

If it's just me, well then I guess I'll have to monkey patch it in my project. =)

@codeodor

This comment has been minimized.

Show comment
Hide comment
@codeodor

codeodor Apr 16, 2012

Contributor

I should also note that I only ran the tests for mysql, since I didn't have the other DBs handy.

Contributor

codeodor commented Apr 16, 2012

I should also note that I only ran the tests for mysql, since I didn't have the other DBs handy.

@cfis

This comment has been minimized.

Show comment
Hide comment
@cfis

cfis Apr 20, 2012

Thanks for the patch. Do tests pass with this? Conceptually I think this makes sense, the subquery solution was meant to deal with more than 1 field (see my comment to your github issue). Seems like you could special case the 1 field. I'll give it a try on postgresql.

Charlie

cfis commented on 49d399d Apr 20, 2012

Thanks for the patch. Do tests pass with this? Conceptually I think this makes sense, the subquery solution was meant to deal with more than 1 field (see my comment to your github issue). Seems like you could special case the 1 field. I'll give it a try on postgresql.

Charlie

This comment has been minimized.

Show comment
Hide comment
@codeodor

codeodor Apr 20, 2012

Owner

It passes the tests on mysql. I have postgresql and sqlite3 handy now, and the tests pass on those as well.

I did see an error that didn't affect the test output though:

Unable to load restaurants_suburb, underlying cause No such file to load -- restaurants_suburb
Owner

codeodor replied Apr 20, 2012

It passes the tests on mysql. I have postgresql and sqlite3 handy now, and the tests pass on those as well.

I did see an error that didn't affect the test output though:

Unable to load restaurants_suburb, underlying cause No such file to load -- restaurants_suburb

This comment has been minimized.

Show comment
Hide comment
@codeodor

codeodor Apr 20, 2012

Owner

BTW, the tests were run on Ruby 1.9.2p290 with Rails 3.2.1

Owner

codeodor replied Apr 20, 2012

BTW, the tests were run on Ruby 1.9.2p290 with Rails 3.2.1

This comment has been minimized.

Show comment
Hide comment
@cfis

cfis Apr 21, 2012

The error above is fine, its because there is no restaurants_suburb class but that's a hatbm join table iirc.

cfis replied Apr 21, 2012

The error above is fine, its because there is no restaurants_suburb class but that's a hatbm join table iirc.

cfis added a commit that referenced this pull request May 5, 2012

@cfis cfis merged commit 63854f2 into composite-primary-keys:master May 5, 2012

@cfis

This comment has been minimized.

Show comment
Hide comment
@cfis

cfis May 5, 2012

Contributor

Ok, merged in, will see if it works with postgresql or not.

Contributor

cfis commented May 5, 2012

Ok, merged in, will see if it works with postgresql or not.

@codeodor

This comment has been minimized.

Show comment
Hide comment
@codeodor

codeodor May 7, 2012

Contributor

Cool. I'm back from traveling as of today, so I plan to do a little more of the research we discussed on the comments of issue #98 when I'm caught up on email.

Contributor

codeodor commented May 7, 2012

Cool. I'm back from traveling as of today, so I plan to do a little more of the research we discussed on the comments of issue #98 when I'm caught up on email.

cfis added a commit that referenced this pull request May 20, 2012

Merge pull request #107 from codeodor/master
Pull Request #99 had a bug when selecting distinct on a column
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment