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(resolve): fix resolving reference to CTEs #3220

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

simonklee
Copy link
Contributor

Fixed resolving refs to CTEs by adding CTEs to the aliasMap and indexing its columns when resolving catalog references.

Fix #3219


This is may not be the fix you want. I'm sure there is a better way to resolve this.

@ConradKurth
Copy link

+1 on getting this fixed!

@msbatarce
Copy link

Also waiting for this fix!

@Jille
Copy link
Contributor

Jille commented Apr 4, 2024

@simonklee Can I get write access to this branch so I can rebase and push some test fixes?

Jille added a commit to Jille/sqlc that referenced this pull request Apr 4, 2024
We should check whether these changes are correct before merging.
Fixed resolving refs to CTEs by adding CTEs to the aliasMap and indexing
its columns when resolving catalog references.

Fix sqlc-dev#3219
@simonklee
Copy link
Contributor Author

@simonklee Can I get write access to this branch so I can rebase and push some test fixes?

I invited you. I also rebased the branch onto main and you can now see the tests that fail. It's the managed database tests and I haven't been able to reproduce this locally. Not 100% sure how to setup the managed test env to run from local machine.

CGO_ENABLED=1 go test -tags=example ./...

Runs without failure for me.

@simonklee
Copy link
Contributor Author

simonklee commented Apr 5, 2024

I was able to reproduce it locally with this test setup:

extern/sqlc git diff
diff --git a/Makefile b/Makefile
index b4b7e80bc..869a6935f 100644
--- a/Makefile
+++ b/Makefile
@@ -18,6 +18,19 @@ test-examples:
 build-endtoend:
        cd ./internal/endtoend/testdata && go build ./...

+test-managed:
+       @echo "Starting required services..."
+       docker-compose up -d --remove-orphans
+
+       @echo "Exporting connection strings..."
+       export MYSQL_SERVER_URI="root:mysecretpassword@tcp(localhost:3306)/dinotest?multiStatements=true&parseTime=true"; \
+       export POSTGRESQL_SERVER_URI="postgres://postgres:mysecretpassword@localhost:5432/postgres?sslmode=disable"; \
+       echo "Running tests..."; \
+       go test -tags=example ./...
+
+       @echo "Stopping services..."
+       docker-compose down
+
 test-ci: test-examples build-endtoend vet

 sqlc-dev:

I've checked each of them, and it seems correct those fields lose their nullability
@simonklee
Copy link
Contributor Author

simonklee commented Apr 5, 2024

I think the updated tests are correct. For instance:

-- name: UpdateAttribute :one
with updated_attribute as (UPDATE attribute_value
    SET
        val = CASE WHEN @filter_value::bool THEN @value ELSE val END
    WHERE attribute_value.id = @id
    RETURNING id,attribute,val)
select updated_attribute.id, val, name
from updated_attribute
         left join attribute on updated_attribute.attribute = attribute.id;

had created this struct:

type UpdateAttributeParams struct {
	FilterValue pgtype.Bool
	Value       pgtype.Text
	ID          pgtype.Int8
}

pgtype.Bool, pgtype.Text, and pgtype.Int8 are nullable types and not required in this context. So, this change seems correct to me:

type UpdateAttributeParams struct {
-	FilterValue pgtype.Bool
-	Value       pgtype.Text
-	ID          pgtype.Int8
+	FilterValue bool
+	Value       string
+	ID          int64
}

The other changes are similar and I've also checked them. To me, Jille's update seems correct.

@Dmdv
Copy link

Dmdv commented May 15, 2024

When is the merge?

@dkrieger
Copy link

dkrieger commented Jul 1, 2024

@simonklee are you still driving this PR?

@simonklee
Copy link
Contributor Author

@simonklee are you still driving this PR?

I don't exactly know what you mean by "driving" the PR. I don't feel like nudging maintainers to merge pull requests. It was ready in April and I'm using a fork with the changes since February. AFAIK only @kyleconroy and @andrewmbenton have commit access and before the that changes I doubt this PR (or the PR and issue queue) will move much.
Andrew and Kyle have done a tremendous job building this project and personally I don't mind maintaining a few patches on top of it.

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.

Resolving reference to CTE column fails
6 participants