fix(js): stream one record batch per result-iterator next() - #42
Open
fornwall wants to merge 1 commit into
Open
Conversation
`AdbcResultIteratorCore::next` previously drained the entire `RecordBatchReader` into a single contiguous IPC buffer on its first call and marked itself exhausted. For large result sets this materialized the whole result in memory at once (OOM risk) and defeated the streaming abstraction the iterator name implies. `next()` now yields the bytes for a single record batch per call and returns `Ok(None)` once the reader is exhausted. A persistent `StreamWriter<Vec<u8>>` is created lazily on the first call so the schema message is written exactly once at the front of the first buffer; each subsequent buffer contains only its record batch, and the underlying `Vec` is drained (`std::mem::take` on `get_mut`) after each write. An empty result set still emits a schema-only stream (finished with the end-of-stream marker) so the consumer observes the result schema, exactly as before. Errors keep the existing conversion path: an `arrow_schema::ArrowError` propagates through `?` as `ClientError::Arrow`. JS-consumer compatibility verified: `iteratorToReader` in lib/index.ts feeds every per-`next()` chunk into a single `RecordBatchReader.from(asyncIterable)`, which concatenates the chunks into one logical byte stream. Because the concatenation of the drained buffers is byte-for-byte a single valid IPC stream (schema header followed by each batch), the existing decoder reads every batch with no JS changes. This was confirmed empirically against apache-arrow 21: the naive "self-contained stream per batch" variant would have stopped at the first stream's end-of-stream marker (reading only the first batch), whereas the continuous-stream approach used here reads all batches, and the empty-result stream still decodes to a schema with zero rows. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNCrC87g9MkppGpL4MDgh5
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.
Problem
AdbcResultIteratorCore::next(javascript/src/client.rs) buffered the entire result set into a single contiguous IPC buffer on its first call and then marked itself exhausted:For large result sets this materializes the whole result in memory at once — an OOM risk — and defeats the streaming abstraction the iterator name implies.
Fix
next()now yields the bytes for one record batch per call and returnsOk(None)once the reader is exhausted.A persistent
StreamWriter<Vec<u8>>is created lazily on the first call so the schema message is written exactly once at the front of the first buffer. Each subsequent buffer contains only its record batch; the underlyingVecis drained after each write. An empty result set still emits a schema-only stream (finished with the end-of-stream marker) so the consumer observes the result schema, exactly as before. Error conversion is unchanged: anarrow_schema::ArrowErrorpropagates through?asClientError::Arrow.JS-consumer compatibility (verified)
iteratorToReaderinlib/index.tsfeeds every per-next()chunk into a singleRecordBatchReader.from(asyncIterable), which concatenates the chunks into one logical byte stream. The concatenation of the drained buffers here is byte-for-byte a single valid IPC stream (schema header followed by each batch), so the existing decoder reads every batch — no JS changes required.This was confirmed empirically against
apache-arrow21:Testing
cd javascript && cargo buildpasses.🤖 Generated with Claude Code
https://claude.ai/code/session_01XNCrC87g9MkppGpL4MDgh5