Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix AR::Relation#cache_key to remove select scope added by user #23051

Merged

Conversation

prathamesh-sonpatki
Copy link
Member

  • We don't need the select scope added by user as we only want to max
    timestamp and size of the collection. So we already know which columns
    to select.
  • Additionally having user defined columns in select scope blows the cache_key
    method with PostGreSQL because it needs all selected columns in the group_by
    clause or aggregate function.
  • Fixes Collection cache_key raises PG::GroupingError with select in query #23038.

r? @matthewd

@prathamesh-sonpatki
Copy link
Member Author

@matthewd I tried investigating group_by and includes queries, but they worked without unscoping select. Can you help with those test cases?

@matthewd
Copy link
Member

@prathamesh-sonpatki group(:something) won't cause an error.. it'll just mean we get the count/max for one arbitrary group, rather than the whole relation.

And includes+references / preload will vary the count because of the join.. which may or may not be a thing we want.

I don't have any particular opinion on whether we want/need to address either of those, though.

Maybe we should raise instead of potentially giving misleading answers? ¯_(ツ)_/¯

(but that's all incidental observation, not directly relevant to your fixing select here.)

@prathamesh-sonpatki
Copy link
Member Author

I got it now what you meant. Thanks for the clarification :)

@rafaelfranca
Copy link
Member

👍 for raising on group. But this one LGTM. @matthewd :shipit:!

- We don't need the select scope added by user as we only want to max
  timestamp and size of the collection. So we already know which columns
  to select.
- Additionally having user defined columns in select scope blows the cache_key
  method with PostGreSQL because it needs all `selected` columns in the group_by
  clause or aggregate function.
- Fixes rails#23038.
@prathamesh-sonpatki
Copy link
Member Author

Rebased with master.

rafaelfranca added a commit that referenced this pull request Jan 24, 2016
…he-key

Fix AR::Relation#cache_key to remove select scope added by user
@rafaelfranca rafaelfranca merged commit 6b89c4a into rails:master Jan 24, 2016
@prathamesh-sonpatki prathamesh-sonpatki deleted the fix-collection-cache-key branch January 25, 2016 01:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants