Repository navigation
fix(FB-5242): read every result set a response carries - #557
Merged
Merged
Conversation
simonzelenskifbio
approved these changes
Oct 26, 2026
kop
reviewed
Oct 26, 2026
kop
left a comment
kop
left a comment
There was a problem hiding this comment.
Two bugs and a few smaller items. Both bugs come from nextset() now reading past the end of the current result set to look for the next START. Details are inline; the sync and async streaming row sets are affected the same way.
Bugs
rowcountis wrong afternextset()skips rows.- A transport error after the last record escapes
nextset()as a raw httpx exception and leaves the responses open.
Smaller items
- A blank line after the final FINISH now makes
nextset()raise. nextset()can now raiseFireboltStructuredError, and a FINISH_WITH_ERRORS without START overwrites the previous set's statistics.- In-memory: separator handling and non-object documents.
Testing gap
The feature itself, one request carrying several statements via skip_parsing=True, has no integration test against a real engine, and the integration workflows don't run on PRs. So these are unverified: how the engine separates JSON documents, whether it pads the stream with blank lines, whether a failed statement comes back as HTTP 200 with an error document/record, and whether DDL/DML statements each produce their own result set. A dispatch-run integration test of execute and execute_stream with skip_parsing=True would settle them. (Multi-response execute_stream was broken on main by the _lines_ite typo, and no integration test caught it.)
Unit tests are missing for: rowcount after skipping rows, a blank line after FINISH, nextset() running into FINISH_WITH_ERRORS, and a transport error after FINISH. The four adjusted nextset tests preset _response_consumed = True, so they never go through the new skip loop.
Every finding was reproduced with scripted httpx responses and compared against origin/main. The unit suite passes (822 passed, 1 skipped).
A response holds one result document per statement it ran. The in-memory row set parsed the body as a single JSON document and failed with "Extra data" on the second one, and the streaming row set stopped at the first FINISH record, so later result sets were never reachable through nextset(). Both row sets now walk the result sets inside a response before moving on to the next response. On the streaming path nextset() reads and discards the rest of the current result set, counting the skipped rows into rowcount, skips blank lines, turns a transport error into an OperationalError, and reports an error record met where the next START was expected against a reset result set. The in-memory path only keeps result sets from a body that parsed completely, accepts JSON whitespace only between documents, and reports a non-object document or trailing text after a complete document as a DataError that carries that text. Along the way, _reset() assigned a misspelled attribute, so the line iterator of a finished response was reused for the next one; the error message for an unexpected record crashed on the raw message_type string; and a DATA record without rows raised IndexError on fetch.
gm42
force-pushed
the
fb-5242-multiple-result-sets-per-response
branch
from
October 26, 2026 16:03
04d8b48 to
43b75af
Compare
|
pscls
added a commit
that referenced
this pull request
Oct 28, 2026
## Summary Since #557, `execute_stream` can return several result sets in one response, and `nextset()` walks through them. To find out whether another set follows, `nextset()` has to read past the remaining rows of the current set. It threw those rows away as it went. On the last set this meant `nextset()` returned `False` with the rows already gone, so the common "skip to the last result set, then fetch" loop got nothing back. `execute` was not affected: buffered results keep the current set when `nextset()` returns `False`. ```python cursor.execute("SET enable_multi_query_requests = true") cursor.execute_stream( "INSERT INTO t VALUES (1); SELECT 1 UNION ALL SELECT 2;", skip_parsing=True, ) while cursor.nextset(): pass cursor.fetchall() # before: [] after: [[1], [2]] # cursor.execute(...) with the same loop already returned [[1], [2]] ``` ## Fix - `nextset()` now keeps the rows it reads past. If no next set follows, they stay readable from where reading stopped. Once a next set starts, they are dropped as before. - If a statement fails while its rows are being skipped, those rows are dropped as well, so a failed statement's partial rows can't be fetched afterwards. - `row_count` is unchanged: it still covers every row of the set, fetched or skipped. - Sync and async row sets get the same change. The `nextset()` docstrings on both cursors now describe what happens when there is no next set. **Trade-off:** skipping a result set that hasn't been read now holds its remaining rows in memory until the next set starts. There is no way around this: the only way to know whether another set follows is to read past every remaining row, so `nextset()` must either keep them or lose them. ## Test plan Unit tests for the sync and async streaming row sets: - the last set stays readable after `nextset()` returns `False`, with 0 to 3 rows already fetched across several DATA records - repeated `nextset()` at the end neither loses nor counts rows twice - walking past empty result sets to the last one - skipped rows are dropped when the next set is in the next response - rows of a statement that errors during `nextset()` cannot be fetched Cursor tests (sync and async) for `execute_stream` + `while nextset()` + `fetchall()`. Two existing mock-based `test_recovery_after_error` tests had their setup adjusted to a state the real stream can reach; none were removed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes core streaming cursor iteration and multi-result-set navigation; behavior shift is intentional but could affect callers relying on rows being discarded after nextset. > > **Overview** > Fixes **`execute_stream` + `while nextset()` + `fetchall()`** on the final result set: streaming **`nextset()`** used to read ahead to see if another set exists and **threw away** those rows, so the last set often came back empty even though buffered **`execute`** kept them. > > **Streaming row sets** (sync and async) now buffer rows read during **`nextset()`** in **`_skipped_records`**. If no next set exists, iteration continues from that buffer; if a next set starts (or the statement errors), those rows are cleared as before. **`row_count`** still counts skipped rows. Cursor **`nextset()`** docstrings describe the new behavior. > > **Trade-off:** skipping an unread result set may hold its remaining rows in memory until the next set begins. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit f331710. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The engine can answer one request with several result sets, one document per statement. The SDK read only the first one:
executeparsed the body as a single JSON document and failed on the second withExtra data, andexecute_streamstopped at the firstFINISH_SUCCESSFULLYrecord, sonextset()never reached the rest.Both row sets now walk the result sets inside a response before moving on to the next response, so
nextset()behaves the same whether the result sets came from one response or from several. Together with the existingskip_parsing=True, a cursor can send a text as one request and read everything that comes back.Two latent bugs on the same path go with it:
_reset()assigned a misspelled attribute, so the exhausted line iterator of a finished response was reused for the next one. Moving across two streamed responses failed withUnexpected end of response stream while reading columns; the existing tests missed it because they patch the column fetch.record.message_type.value, but parsed records hold the raw string, so the message itself raisedAttributeError.Test plan
Unit coverage for both formats, sync and async: several result sets in one response, an empty result set in between, result sets spanning several responses, an error in a later result set, truncated streams and unexpected records, plus cursor-level
executeandexecute_streamwithskip_parsing=True. Four existing mock-basednextsettests had their setup adjusted to the new semantics; none were removed.Note
Medium Risk
Changes core query result parsing and
nextset()behavior for all sync/async buffered and streaming paths; behavior change for multi-statement single-request responses but aligned with engine output.Overview
Fixes reading every result set returned in a single HTTP response (FB-5242). The engine can attach multiple statement results to one body; the SDK previously kept only the first (
Extra dataon buffered JSON, or stopping at the firstFINISH_SUCCESSFULLYin JSONLines).Buffered (
InMemoryRowSet) now decodes the body as a sequence of JSON objects (RFC whitespace between documents), builds one in-memory result set per document, and surfaces errors or trailing garbage after a complete document.Streaming (
StreamingRowSet/StreamingAsyncRowSet+ sharedstreaming_common) advancesnextset()by draining the current result set on the same response (_next_result_set_in_response) before closing the HTTP response and moving to the next one. Row counts include rows skipped bynextset, blank JSONLines are ignored, and stream/parse failures set_response_failedso the response is not reused. State reset is split so the line iterator survives within a response but is cleared when switching responses (fixes the_lines_itetypo that broke multi-response streaming).Cursors document the new
nextsetsemantics and possibleOperationalError/FireboltStructuredError. Unexpected message-type errors use_message_type_nameinstead of assuming an enum.value.Unit and cursor tests cover multi-result-set
execute/execute_streamwithskip_parsing=True, plus error and edge cases.Reviewed by Cursor Bugbot for commit 43b75af. Bugbot is set up for automated code reviews on this repo. Configure here.