sqlc icon indicating copy to clipboard operation
sqlc copied to clipboard

fix(resolve): fix resolving reference to CTEs

Open simonklee opened this issue 1 year ago • 7 comments

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.

simonklee avatar Feb 25 '24 00:02 simonklee

+1 on getting this fixed!

ConradKurth avatar Mar 09 '24 02:03 ConradKurth

Also waiting for this fix!

msbatarce avatar Apr 01 '24 16:04 msbatarce

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

Jille avatar Apr 04 '24 22:04 Jille

@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 avatar Apr 05 '24 08:04 simonklee

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:

simonklee avatar Apr 05 '24 08:04 simonklee

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.

simonklee avatar Apr 05 '24 19:04 simonklee

When is the merge?

Dmdv avatar May 15 '24 04:05 Dmdv

@simonklee are you still driving this PR?

dkrieger avatar Jul 01 '24 19:07 dkrieger

@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.

simonklee avatar Jul 01 '24 21:07 simonklee