-
Notifications
You must be signed in to change notification settings - Fork 834
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
Ability to return an iterator on rows #3631
base: main
Are you sure you want to change the base?
Conversation
Thank you so much for the first pass on iterator support. Could you update your PR description with an example of how to use the |
Fixes #720 |
@kyleconroy |
@kyleconroy |
I found some example tests here https://github.com/sqlc-dev/sqlc/tree/main/examples and added iterator tests there |
@kyleconroy, hi |
I see that the stdlib sometimes uses the suffix Seq() to indicate methods that returns iterators. I suggest to think a bit more if Iterate() is the appropriate name for this functionality. |
|
I do not think the |
@gbarr, thanks for your comment
|
Instead of having an IterCities(ctx context.Context) (seq iter.Seq2[City, error], stop func()) Usage would be: func example(ctx context.Context, q *Queries) error {
rows, stop := q.IterCities(ctx)
defer stop()
for city, err := range rows {
if err != nil {
return err
}
// do something with city
}
return nil
} I think this is a bit cleaner than the above generated code, saving a whole new type for the iterator. |
I thought about such an interface, but I'm afraid that if the user uses range with one variable, the behavior will not match the expected one |
I found these in the release notes for Go 1.24. The bytes package adds several functions that work with iterators:
I don't like the |
Following this example, it seems more appropriate to rename Iterate() to Rows() / Items() / Values(). Because the Seq suffix is used when there was already a Foo() or FooBar() function, and we want a similar FooSec() function that returns a sequence |
I agree that the My preference would be for |
I would prefer Items(). |
It's great to see iterators coming to sqlc. Adding memory pooling as an option on top of this would make this truly a game-changer for large result sets. I would imagine saving the pool in a public variable, and let the callside have the responsibility to put the object back. Thoughts? Maybe out of scope for this one? |
I'm up for this one. The only thing that blocks us from migrating from pure SQLx is the absence of an iterator. |
Not to blow up our existing design, but I just read this post (https://blog.thibaut-rousseau.com/blog/writing-testing-a-paginated-api-iterator/) which makes me think we may want to consider the following: func example(ctx context.Context, q *Queries) {
for city, err := range rows.IterCities(ctx) {
// do something with city
}
} The upside to this design is that it's difficult to use incorrectly. We don't have to decide on a name for the |
The problem with using Seq2 that returns err is that if the code |
Just to add my 2cts to the discussion: it seems weird to me to have the error, and have to check it, in every loop iteration. That’s an advantage of the initial proposal. Since we can start the query in the first iteration, and break inside the loop, we don’t need Close. We can use Err() to determine if the query or any of the row fetches failed. I personally prefer:
Over:
Simply because of the lack of err clutter inside the loop. This does assume that:
|
Example of generated query:
Example of use:
UPD: updated the code according to the latest changes