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

Having aggregate alias bug #2174

Merged
merged 8 commits into from
Dec 5, 2023
Merged

Having aggregate alias bug #2174

merged 8 commits into from
Dec 5, 2023

Conversation

max-hoffman
Copy link
Contributor

@nicktobey
Copy link
Contributor

nicktobey commented Dec 4, 2023

I added some additional tests to the branch that I wrote while trying to convince myself this is correct.

The fundamental issue is that aggregate expressions should always prefer binding names to columns in the FROM expression over aliases in the SELECT expression. But if there isn't a match in the underlying tables, we're allowed to match to an alias. This can be correct behavior if the alias is uniquely determined by the aggregated column.

This can be seen in one of the examples I added:

select y as z from xy group by (y) having AVG(z) > 0;

z does not bind to a column in xy, so it binds to the alias. The alias is uniquely determined by the aggregate column, so the behavior is well-defined.

With this PR, that statement is an error. But it should be allowed.

@nicktobey
Copy link
Contributor

On the other hand, it looks like the added test doesn't work before this PR either, so this PR may still be making things strictly more correct.

@max-hoffman
Copy link
Contributor Author

max-hoffman commented Dec 4, 2023

This one is a good test, but I'd separate it as an additive change to what I have so far

select y as z from xy group by (y) having AVG(z) > 0;

We can only express one group by per scope, so the z is going to have to be variable replaced and normalized into the format of other aggregations.

@max-hoffman max-hoffman merged commit 6c70cd8 into main Dec 5, 2023
7 checks passed
@max-hoffman max-hoffman deleted the max/having-agg-alias-bug branch December 5, 2023 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HAVING clause is handled incorrectly when it references a name that matches both a column and an alias.
2 participants