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

Refactor go codegen to plugin types #1460

Merged
merged 7 commits into from
Mar 2, 2022
Merged

Refactor go codegen to plugin types #1460

merged 7 commits into from
Mar 2, 2022

Conversation

stephen
Copy link
Contributor

@stephen stephen commented Mar 1, 2022

See #288, #923, #1416 .

(hi @kyleconroy!)

I got excited about the plugin api and wanted to try a few things out with the golang code generation...

There are a couple questionable things in here that I wasn't sure about - will add some comments about them. If you think this work is worth continuing with, I'd be happy to try to clean it up and put it in an upstreamable state.

The tests currently pass.

Copy link
Contributor Author

@stephen stephen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added some comments on things i was not sure about in the port

@@ -28,6 +28,8 @@ message Override {
string column_name = 8 [json_name="column_name"];

PythonType python_type = 9;

ParsedGoType go_type = 10;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh boy nice indent

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol. I need to set up some tooling to auto format the proto files. For now, proto files should use spaces instead of tabs. It's not a big deal though.

internal/cmd/shim.go Show resolved Hide resolved
internal/cmd/shim.go Show resolved Hide resolved
Comment: typ.Comment,
Vals: typ.Vals,
})
case *catalog.CompositeType:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i assume the proto has Enums instead of Types because it it hard to represent a Enum | CompositeType in proto. this could also be a oneof maybe

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're correct that oneof is the correct way to handle this type of polymorphism. You can see this in the Python AST proto file. For now I'm happy to keep these as two separate arrays.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah thanks, i missed the existing usage of oneof. i am happy to keep this as is or take a suggested refactor

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine to keep this for now

internal/cmd/shim.go Show resolved Hide resolved
internal/codegen/golang/gen.go Show resolved Hide resolved
internal/codegen/golang/go_type.go Show resolved Hide resolved
}

for _, ct := range schema.CompositeTypes {
// XXX: old code had a bug?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think the old code... did not check that the name/schema actually matched? but this is conjecture and i did not try to write a test

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think the current composite type support is broken at best, so this feels like a step in the right direction.

@kyleconroy
Copy link
Collaborator

This is awesome! Hoping to review this later tonight

Copy link
Collaborator

@kyleconroy kyleconroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few small changes, but overall this is really close.

internal/cmd/shim.go Show resolved Hide resolved
internal/cmd/shim.go Show resolved Hide resolved
Comment: typ.Comment,
Vals: typ.Vals,
})
case *catalog.CompositeType:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're correct that oneof is the correct way to handle this type of polymorphism. You can see this in the Python AST proto file. For now I'm happy to keep these as two separate arrays.

internal/cmd/shim.go Show resolved Hide resolved
)

func sameTableName(n *ast.TableName, f core.FQN, defaultSchema string) bool {
func sameTableName(n *plugin.Identifier, f core.FQN, defaultSchema string) bool {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should refactor this to match the signature from the codegen/python so that it doesn't depend on the core package.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!

SQLPackage: sqlpkg,
}
} else if len(query.Params) > 1 {
var cols []goColumn
for _, p := range query.Params {
cols = append(cols, goColumn{
id: p.Number,
id: int(p.Number), // XXX: ??
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Go can be weird. Casting a int32 to an int is fine.

}

for _, ct := range schema.CompositeTypes {
// XXX: old code had a bug?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think the current composite type support is broken at best, so this feels like a step in the right direction.

@@ -28,6 +28,8 @@ message Override {
string column_name = 8 [json_name="column_name"];

PythonType python_type = 9;

ParsedGoType go_type = 10;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol. I need to set up some tooling to auto format the proto files. For now, proto files should use spaces instead of tabs. It's not a big deal though.

protos/plugin/codegen.proto Outdated Show resolved Hide resolved
@@ -131,6 +171,7 @@ message Query
repeated Parameter params = 5 [json_name="parameters"];
repeated string comments = 6 [json_name="comments"];
string filename = 7 [json_name="filename"];
Identifier InsertIntoTable = 8;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be insert_into_table.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed, thanks

@stephen
Copy link
Contributor Author

stephen commented Mar 1, 2022

thanks for looking! i addressed/responded to comments here.

func goInnerType(r *compiler.Result, col *compiler.Column, settings config.CombinedSettings) string {
columnType := col.DataType
func goInnerType(req *plugin.CodeGenRequest, col *plugin.Column) string {
columnType := dataType(col.Type)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, one more thought: it might be worth removing DataType from the proto. I fell into the pit where it's not actually filled but available on the struct.

I suspect that https://github.com/stephen/sqlc/blame/32f4c968857273582f71d01b72fdf85589548f1d/internal/codegen/python/gen.go#L199 might be wrong? probably wants to also dataType(col.Type)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having both is confusing and error-prone. Let's remove it in another PR

@kyleconroy kyleconroy marked this pull request as ready for review March 2, 2022 03:07
@kyleconroy kyleconroy merged commit aade087 into sqlc-dev:main Mar 2, 2022
stephen added a commit to stephen/sqlc that referenced this pull request Mar 2, 2022
See sqlc-dev#1460 (comment)

DataType is never filled in so this comparison is buggy. This change mirrors
what the go gen.go does instead.
stephen added a commit to stephen/sqlc that referenced this pull request Mar 2, 2022
See sqlc-dev#1460 (comment)

DataType is never filled in so this comparison is buggy. This change mirrors
what the go gen.go does instead.

Deleting a field from proto can be not safe for compat, so we could use
https://developers.google.com/protocol-buffers/docs/proto3#reserved,
but I think these protos are transient so it should be ok.
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