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

incorrect ambiguous reference check #2398

Closed
Stumble opened this issue Jun 30, 2023 · 2 comments · Fixed by #2537
Closed

incorrect ambiguous reference check #2398

Stumble opened this issue Jun 30, 2023 · 2 comments · Fixed by #2537

Comments

@Stumble
Copy link

Stumble commented Jun 30, 2023

Version

1.18.0

What happened?

There seems to be a minor bug in checking ambiguous references when joning tables while using alias.

To reproduce, change the sqlc/internal/endtoend/testdata/order_by_non_existing_column/postgresql to

-- Example queries for sqlc
CREATE TABLE authors (
  id   INT
);

CREATE TABLE books (
  id   INT,
  author_id INT,
  price INT
);

-- name: ListAuthors :many
SELECT id FROM authors
ORDER BY adfadsf;

-- name: ListAuthorsByCheapestBook :many
SELECT
author_id, min(b.price) as min_price
From books b inner join authors a on a.id = b.author_id
GROUP BY b.author_id
ORDER BY min_price;

The column of min_price is not ambiguous but sqlc reports:

# package 
query.sql:13:1: column reference "adfadsf" not found: if you want to skip this validation, set 'strict_order_by' to false
query.sql:17:1: column reference "min_price" is ambiguous: if you want to skip this validation, set 'strict_order_by' to false

Relevant log output

query.sql:13:1: column reference "adfadsf" not found: if you want to skip this validation, set 'strict_order_by' to false
query.sql:17:1: column reference "min_price" is ambiguous: if you want to skip this validation, set 'strict_order_by' to false

Database schema

CREATE TABLE authors (
  id   INT
);

CREATE TABLE books (
  id   INT,
  author_id INT,
  price INT
);

SQL queries

-- name: ListAuthorsByCheapestBook :many
SELECT
author_id, min(b.price) as min_price
From books b inner join authors a on a.id = b.author_id
GROUP BY b.author_id
ORDER BY min_price;

Configuration

No response

Playground URL

No response

What operating system are you using?

Linux

What database engines are you using?

PostgreSQL

What type of code are you generating?

Go

@Stumble Stumble added bug Something isn't working triage New issues that hasn't been reviewed labels Jun 30, 2023
akutschera added a commit to akutschera/sqlc that referenced this issue Jul 1, 2023
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
akutschera added a commit to akutschera/sqlc that referenced this issue Jul 1, 2023
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
@akutschera
Copy link
Contributor

akutschera commented Jul 1, 2023

That's an interesting one. I believe this has been in the code for a while.
What happens seems to be this:
For each column name in a GROUP BY or ORDER BY clause, we check, if we have it in a table definition.
When we cannot find it, we check the select statement. When we find it there (e.g. in an alias) we say we found it.
Because we iterate over the number of tables in the statement, we will find it twice. That causes this problem.

The PR above should fix this (and also #1886 which seems to be very similar to this one here).

@kyleconroy kyleconroy added 📚 postgresql 🔧 golang 💻 linux and removed triage New issues that hasn't been reviewed labels Jul 6, 2023
andrewmbenton added a commit that referenced this issue Jul 27, 2023
kyleconroy pushed a commit that referenced this issue 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
@Stumble
Copy link
Author

Stumble commented Aug 29, 2023

Sorry for late reply, thanks for the fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants