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

Include selects in group query with having clause #28183

Merged
merged 1 commit into from
Feb 26, 2017

Conversation

eugeneius
Copy link
Member

When a grouped calculation contains a having clause that references a selected value, we need to include that selected value in the query.

Postgres doesn't support referencing a selected value in a having clause, but other databases do; we can skip the test on the pg adapter but run it for the others.

This was fixed before in #1969, but the test coverage was lost in 5a05207. The fix regressed in 6311975 and was removed in #28179.

r? @pixeltrix

/fyi @kamipo

When a grouped calculation contains a having clause that references a
selected value, we need to include that selected value in the query.

Postgres doesn't support referencing a selected value in a having
clause, but other databases do; we can skip the test on the pg adapter
but run it for the others.

This was fixed before in 9a298a1, but
the test coverage was lost in 5a05207.
The fix regressed in 6311975 and was
removed in 97d46c1.
@pixeltrix
Copy link
Contributor

@eugeneius good catch - I didn't read the intent of the query correctly

@pixeltrix pixeltrix merged commit 39449ef into rails:master Feb 26, 2017
pixeltrix added a commit that referenced this pull request Feb 26, 2017
@eugeneius
Copy link
Member Author

Thanks for the quick review! 👍

I assumed the same thing as you initially -- that the test proved that some other code was now covering the same functionality -- but I was curious enough to try running a query...

@pixeltrix
Copy link
Contributor

Backported to 5-0-stable and and 4-2-stable - thanks again @eugeneius

yahonda added a commit to yahonda/rails that referenced this pull request Feb 27, 2017
pixeltrix pushed a commit that referenced this pull request Feb 28, 2017
pixeltrix pushed a commit that referenced this pull request Feb 28, 2017
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.

3 participants