Skip to content

Improve support for SQL functions that return tables #1973

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jrperritt
Copy link
Contributor

@jrperritt jrperritt commented Nov 30, 2022

This change adds an IsArray field for function parameters and creates structs for PosgreSQL functions that return tables.

There are a couple of examples in internal/endtoend/testdata/func_return_table/, in addition to the existing examples in func_return that didn't regress.

closes #1322

@jrperritt jrperritt force-pushed the func-table-return-type branch from 4e58b26 to 1b847a3 Compare December 8, 2022 14:01
@jrperritt
Copy link
Contributor Author

@kyleconroy Is this change out of scope for this project? If so, I can go ahead and close this to clear up the queue. If not, I'll try to find some time to rebase and resolve conflicts.

@andrewmbenton
Copy link
Collaborator

I don't think it's out of scope necessarily. When @kyleconroy and I last discussed I think we had settled on support for queries using the RETURNS TABLE(columns) syntax. He may correct me though.

I haven't had a chance to look closely through your PR to determine whether it meets that requirement though.

@jrperritt
Copy link
Contributor Author

I don't think it's out of scope necessarily. When @kyleconroy and I last discussed I think we had settled on support for queries using the RETURNS TABLE(columns) syntax. He may correct me though.

I haven't had a chance to look closely through your PR to determine whether it meets that requirement though.

Great. Yes, that's exactly what it's supposed to do. You can see a couple of examples here.

@jrperritt jrperritt force-pushed the func-table-return-type branch 2 times, most recently from 8563994 to 88ab842 Compare July 13, 2023 20:19
@jrperritt
Copy link
Contributor Author

jrperritt commented Jul 13, 2023

The 2 force-pushes are for

  1. Rebasing on main and,
  2. Changing the enum names that were flagged during linting.

@kyleconroy
Copy link
Collaborator

A few pieces of feedback on these changes. Instead of generating structs inside models.go for these functions, I'd like to use per-query row structs. The Go codegen package already handles this for queries that don't cleanly map to an existing table.

This change simplifies the code as well, as we no longer need to pass function information to the codegen plugin.

const booksByTagsViaFunc = `-- name: BooksByTagsViaFunc :many
SELECT book_id, title, name, isbn, tags FROM sqlc_book_schema.books_by_tags_func($1::VARCHAR[])
`

type BooksByTagsViaFuncRow struct {
	BookID sql.NullInt32
	Title  sql.NullString
	Name   sql.NullString
	Isbn   sql.NullString
	Tags   sql.NullString
}

func (q *Queries) BooksByTagsViaFunc(ctx context.Context, dollar_1 []string) ([]BooksByTagsViaFuncRow, error) {
	...
}

As for the tests you've added, can you move all the changes for the examples directory to individual testcases into the end-to-end directory? The examples lean towards simplicity and most users of sqlc don't use stored procedures.

@jrperritt jrperritt force-pushed the func-table-return-type branch from 88ab842 to 9d1a68d Compare July 29, 2023 18:37
@jrperritt
Copy link
Contributor Author

A few pieces of feedback on these changes. Instead of generating structs inside models.go for these functions, I'd like to use per-query row structs. The Go codegen package already handles this for queries that don't cleanly map to an existing table.

This change simplifies the code as well, as we no longer need to pass function information to the codegen plugin.

Indeed, that turned out to be a much better approach.

As for the tests you've added, can you move all the changes for the examples directory to individual testcases into the end-to-end directory? The examples lean towards simplicity and most users of sqlc don't use stored procedures.

Sure, I've added an internal/endtoend/testdata/func_return_table/ directory.

@jrperritt
Copy link
Contributor Author

@kyleconroy This is ready for another look.

@jrperritt jrperritt force-pushed the func-table-return-type branch from 9d1a68d to 34df824 Compare November 7, 2023 01:34
@jrperritt jrperritt force-pushed the func-table-return-type branch from 34df824 to 66f537c Compare November 7, 2023 02:09
@jrperritt
Copy link
Contributor Author

I rebased this on main and added support for multidimensional array function parameters, which was implemented some time in the last year without me realizing 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.

Functions returning TABLE types do not work
3 participants