-
-
Notifications
You must be signed in to change notification settings - Fork 200
Use QueryUnbufferedAsync for streaming IAsyncEnumerable results #671
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: sebastienros <[email protected]>
| { | ||
| // TODO: [IAsyncEnumerable] Once Dapper supports IAsyncEnumerable we can replace this call by a non-buffered one | ||
| foreach (var item in await ListImpl(cancellationToken)) | ||
| await foreach (var item in ListImplAsync(cancellationToken)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Is the foreach necessary? Can't the inner result be returned directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right! I've simplified it to directly return ListImpl(cancellationToken) in commit ea9a596.
|
|
||
| #pragma warning disable CS8425 // Async-iterator member has one or more parameters of type 'CancellationToken' but none of them is decorated with the 'EnumeratorCancellation' attribute, so the cancellation token parameter from the generated 'IAsyncEnumerable<>.GetAsyncEnumerator' will be unconsumed | ||
| async IAsyncEnumerable<T> IQuery<T>.ToAsyncEnumerable(CancellationToken cancellationToken) | ||
| #pragma warning restore CS8425 // Async-iterator member has one or more parameters of type 'CancellationToken' but none of them is decorated with the 'EnumeratorCancellation' attribute, so the cancellation token parameter from the generated 'IAsyncEnumerable<>.GetAsyncEnumerator' will be unconsumed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Can this pragma be removed an fixed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already fixed in commit ea9a596! The pragma was removed by simplifying the implementation - ToAsyncEnumerable now directly returns ListImpl instead of being an async iterator.
| } | ||
| } | ||
|
|
||
| internal async IAsyncEnumerable<T> ListImplAsync([EnumeratorCancellation] CancellationToken cancellationToken) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot why does it need to be a new method? What about replacing the existing one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion! I've replaced ListImpl to return IAsyncEnumerable<T> and updated ListAsync to buffer the results. This eliminates the duplicate method and follows the TODO comment's guidance. See commit ea9a596.
|
|
||
| #pragma warning disable CS8425 // Async-iterator member has one or more parameters of type 'CancellationToken' but none of them is decorated with the 'EnumeratorCancellation' attribute, so the cancellation token parameter from the generated 'IAsyncEnumerable<>.GetAsyncEnumerator' will be unconsumed | ||
| async IAsyncEnumerable<T> IQuery<T>.ToAsyncEnumerable(CancellationToken cancellationToken) | ||
| #pragma warning restore CS8425 // Async-iterator member has one or more parameters of type 'CancellationToken' but none of them is decorated with the 'EnumeratorCancellation' attribute, so the cancellation token parameter from the generated 'IAsyncEnumerable<>.GetAsyncEnumerator' will be unconsumed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Can this pragma be removed an fixed?
- ToAsyncEnumerable now directly returns ListImpl result - Removed pragma warning by eliminating unnecessary wrapper - ListImpl now returns IAsyncEnumerable<T> instead of separate ListImplAsync method - ListAsync buffers results from the async enumerable - Applied same pattern to QueryIndex class Co-authored-by: sebastienros <[email protected]>
Dapper added
QueryUnbufferedAsyncreturningIAsyncEnumerable<T>in version 2.1+, eliminating the need to buffer results when usingToAsyncEnumerable().Changes
ListImpl: Changed to returnIAsyncEnumerable<T>using Dapper'sQueryUnbufferedAsyncto stream results directly from the databaseToAsyncEnumerable: Now directly returnsListImplfor true streaming (no wrapper needed)ListAsync: Buffers results from the async enumerable into aList<T>Query<T>andQueryIndex<T>: Same pattern applied consistently across both implementationsBefore/After
Before:
After:
The new implementation streams results row-by-row using
connection.QueryUnbufferedAsync<T>().WithCancellation(cancellationToken), reducing memory footprint for large result sets. This eliminates code duplication and follows the original TODO guidance to "return it by default, and buffer it in ListAsync instead."Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.