perf(ado.net): fetch multiple rows at a time#779
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces batching and prefetching support for streaming rows in the Spanner ADO.NET provider, updating connection and streaming methods to pass a batch size and buffering fetched rows. However, several critical issues were identified in the review. First, clearing the buffered rows at the start of NextResultSet and NextResultSetAsync can lead to silent data loss, and the provider needs to correctly track and defer the HasMoreResults signal when rows are buffered. Second, the removal of the DrainStream helper and its calls in Server.cs must be reverted, as failing to drain the redirected standard output and error streams of the child process will cause it to block and hang indefinitely when the OS pipe buffer fills up.
Fetch multiple rows at a time when reading data from a query.
216fc83 to
2901edf
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces row prefetching and batching capabilities to the streaming rows implementation in the Spanner ADO.NET gRPC client. It adds a new ContinueStreamingRequest protobuf message to specify batch sizes and implements a buffered queue (_bufferedRows) in StreamingRows to cache pre-fetched rows. Feedback on these changes highlights an opportunity to optimize the row enqueuing logic by returning the first row directly and only enqueuing the remainder. Additionally, a bug was identified in NextResultSetAsync where a synchronous call to ContinueStreaming is used instead of its asynchronous counterpart, which would block the async thread.
Fetch multiple rows at a time when reading data from a query.