Skip to content

fix: count column aliases only once #2399

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

Closed
wants to merge 1 commit into from

Conversation

akutschera
Copy link
Contributor

Alias column names don't exist in the table definition. When we take it from the select target list, we only need to do this once (and not once for every table in a join statement).

Refs: #2398 #1886

Alias column names don't exist in the table definition. When we take
it from the select target list, we only need to do this once (and not
once for every table in a join statement).

Refs: sqlc-dev#2398 sqlc-dev#1886
@andrewmbenton
Copy link
Collaborator

Thanks for this and sorry for the delay. There is definitely a bug, but I don't know if restricting the restarget counting is the right fix (although your change does seem to have "fixed" the bug). The issue seems to be that sqlc doesn't understand what list of refs the aliases in ORDER BY and GROUP BY clauses should be checked against.

I'm going to dig in a little bit and see if I can come up with a recommendation for a different approach.

@andrewmbenton
Copy link
Collaborator

After poking around a little, I think your solution is basically correct. I opted to pull the loop checking for aliases in the target list out of the table iteration loop, which I think is the right thing to do. The PR I have is here: #2537. If you see an issue with that please leave a comment over there.

If my PR gets merged then I'll close this PR.

kyleconroy pushed a commit that referenced this pull request Jul 28, 2023
… joins (#2537)

* fix(compiler): correctly validate alias in order/group by clauses for joins

Resolves #1886
Resolves #2398
Resolves #2399

* remove dead code and split up test
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.

2 participants