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
feat: add sqlc.embed
to allow model re-use
#1615
Conversation
Just voicing support for this ❤️ |
@kyleconroy this would help us out dramatically! Does this approach seem reasonable? |
e206aec
to
b8ca9f5
Compare
@kyleconroy this PR could add a lot of value to the way that we use sqlc in our project. We're thinking of forking the project to merge this PR into the fork, but there is some risk that we take on by forking when it comes to merging future changes into our fork if there aren't plans to merge this PR into the project at some point. So, I'm wondering if there are plans to merge this into a future release? If there aren't, is there any specific issue with the PR that we can address so that it is more likely to be included in a future release? |
@kyleconroy is there an issue with this PR that I can address? The feature looks popular, and I'm willing to make changes. |
b8ca9f5
to
97205bd
Compare
I put this on my Christmas list, hoping Santa pulls through! |
@kyleconroy |
de11a54
to
39b0fb8
Compare
@nickjackson I know I'm late to the party here, so apologies in advance. First off, this is a very cool piece of functionality, and I am in particular interested if this could be implemented in a way that is more harmonious with SQL itself. (and ideally with less special handling) by using a generic tuple type from the database. I am referring to specifically "row constructors": https://www.postgresql.org/docs/current/sql-expressions.html#SQL-SYNTAX-ROW-CONSTRUCTORS In your example: SELECT sqlc.embed(users), (SELECT count(*) FROM users) AS total_count FROM users;
-- could be written in "raw" SQL as
SELECT row(users.*) as user, (SELECT count(*) FROM users) AS total_count FROM users;
-- and to stay consistent with sqlc's "star-expansion" it would expand to a static query like
SELECT row(users.id, users.name) as user, (SELECT count(*) FROM users) AS total_count FROM users; I think this stays a lot closer to the raw SQL, and could possibly enable some crazier queries like: -- name: UserWithPosts :many
select
row(users.*) as user,
count(posts.id) as post_count,
array_agg(row(posts.*)) as posts
from users left join posts on posts.user_id = user.id
group by users.id (I'm sorry if your functionality already allows for this, I couldn't tell for certain). Which should generate a resulting struct like: type UserWitihPostsRow struct {
User User
PostCount int
Posts []Post
} I believe this would require not even require an additional Anyway, I was wondering if you considered this at all? [I am not a maintainer but...] this feels like a smaller change (no additional APIs needed, and therefore easier to merge?) |
Hi @skabbes thanks for message - I've done some research into it but I'd like to just preface that I'm no expert, I just took #984 and ran with it because it was something my team desired. I do like the possibilities with what you are suggesting, getting data out like that would be super convenient. It could reduce the number of round trips and subsequent mappings. In my opinion, I think your suggestion should be an additional feature, not a replacement and I'll explain why. I personally don't mind I think the major issue you'll find with There may be a case for supporting Another more selfish reason that I'd like to keep I hope this answers your questions. 😄 |
With you explanation (and the fact that sqlc wants to be multi-database), I'm on board with that.
I think this feature is important for sqlc to implement, but still trying to find the sqlc-way for it to work. I had a suggestion for a If this feature existed, SELECT
-- pseudo-code for "output column tagging"
sqlc.output(row_to_jsonb(users), 'type', 'model.User'),
(SELECT count(*) FROM users) AS total_count
FROM users; Regardless you convinced me :) but also I think sending enough metadata through to code generators for my above example to have worked would also be aweseme. Thanks for the contribution! looking forward to seeing it land in 1.17! |
39b0fb8
to
14923ff
Compare
Not to pile on, but I'm playing with the branch and it works just as I'd expect it to. Haven't attempted anything particularly complex, but the workflow for simple joins like: -- name: ListUsers :many
select sqlc.embed(u), sqlc.embed(t) from users u
join things t on u.thing_id = t.id
where u.deleted_at is null
and u.email > $1
order by u.email asc
limit $2;
-- type ListUsersRow struct {
-- User User
-- Thing Thing
-- } is sensible and easy. Nice work. |
Hey guys! any update for this feature? |
@kyleconroy I see this in the milestone https://github.com/kyleconroy/sqlc/milestone/18. Is Feb 6 an accurate timeline for this? |
@nickjackson I don't know if this is helpful, but I rebased the upstream onto your branch in nickjackson#2 in case it saves you any grey hairs. I'm reasonably certain I got it right (tests are still passing, at least!) |
For the record, this did not make into the v1.17.0 release, did it? |
Great feature |
@kyleconroy This is going to be super useful feature for adopting sqlc in our org. Is there anyway I can help get this PR merged? |
14923ff
to
6bcbcbf
Compare
@kyleconroy @nickjackson thanks so much for your patience in getting this across the line! I can finally stop using a fork 😆 |
🔥 |
The best merge ever 😆 |
Finally! |
@skabbes Did you open another issue for sqlc to support the |
In a case I have a table cart as example, and a table cart_products that contains all the products of the cart. Is there any way I can retrieve the cart and a slice of cart produtcs? This is just a little schema, so I can try to explain better
At the query Ive wrote the following:
So lets say that at cart 1 I have 2 products. When I run that GetCartProducts I get the following: But I wanted to get:
Yours, Saulo Lauesr |
@SauloLauers I put your example into a playground and can see the output structure is as I'd expect it to be. https://play.sqlc.dev/p/4b5a7f132ce1e0d13fffa509f666fe41ba255b568ba25ab27ca93f77052713ef You need to create your own types for |
Thank you for the quick answer. Yes, I have the structs. I just wanted to see it it was possible somehow. I tried even with a group by. But I did my work arround already mapping it at controller. |
@nickjackson Thanks for this PR, excited to try it out in 1.18. I'm curious, does this work with left joins? If I have an Order struct which sometimes contain a Receipt for example. If I do a left join and use sqlc.embed, will the Order struct returned contain a nullable Receipt? Thanks! |
@azrshana If you use a LEFT JOIN, the embedded struct will not be nullable. |
Intro
This PR allows
sqlc.embed
to be used in a query. This is based on the discussion at #984.You can now write a query like
and sqlc will produce a struct like
Why?
In our code base we have many
List
queries which require additional pagination fields, and I'm unhappy about how much extra mapping we need to do because of it. I could also see it being used to save mapping on some sql joins.How it Works
There is now a
sql/rewrite.Embeds()
function which takes the ast at compile time and looks for thesqlc.embed(param)
function, swapping it out for anast.ColumnRef
ofparam.*
. This allows the compiler to re-use all of the existingast.ColumnRef
logic to build and expand the columns.Columns now have a flag on the proto to indicate they are embedded, allowing the codgen to be adjusted accordingly. I can go into more detail on this side if you wish.
Finally
It's currently in Draft because I need to implement the GoDo I need to do anything to disable this feature for non-go codegen?stdlib
templating and add tests for it.On #984 there was discussion about maybe disabling this for Left joins, I'd be happy to do this but could do with some pointers about how best to do that.
I'm more than happy to receive feedback, especially around the topic of naming things and where to put the various functions. I'm also happy to add more testing if necessary.
Thanks!