CHORE: Stream job results directly to destination instead of buffering in memory - #138
Open
prasadlohakpure wants to merge 8 commits into
Open
CHORE: Stream job results directly to destination instead of buffering in memory#138prasadlohakpure wants to merge 8 commits into
prasadlohakpure wants to merge 8 commits into
Conversation
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try the Wiz Code extension for VS Code, JetBrains, or Visual Studio. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reduces memory pressure and improves time-to-first-byte by streaming job results directly to their output destination (HTTP response, local file, or S3) instead of fully materializing and marshaling results in memory.
Changes:
- Added a streaming result writer (
RowWriter/StreamRows) to emit the existing{"columns":[...],"data":[...]}JSON shape incrementally. - Introduced an optional
plugin.ResultStreamerinterface and updated job dispatch to prefer streaming when implemented (postgres/snowflake/starrocks updated accordingly). - Switched result persistence to streaming writes (atomic temp-file-then-rename locally;
io.Pipe+ multipart uploader for S3), and updated sync HTTP responses to usejson.Encoder.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/result/stream.go | Adds incremental JSON result streaming helpers (RowWriter, StreamRows). |
| pkg/result/result.go | Refactors shared row/column extraction into helpers used by both buffered and streaming paths. |
| pkg/plugin/plugin.go | Adds optional ResultStreamer plugin contract for streaming results. |
| internal/pkg/object/command/starrocks/starrocks.go | Implements StreamResult entrypoint and makes Execute explicitly unreachable for streaming plugins. |
| internal/pkg/object/command/starrocks/job_context.go | Streams StarRocks results to an io.Writer and reworks endpoint fetch to support streaming emission. |
| internal/pkg/object/command/snowflake/snowflake.go | Implements streaming result output via result.StreamRows and refactors query setup. |
| internal/pkg/object/command/postgres/postgres.go | Implements streaming result output via result.StreamRows, with shared preparation/query helpers. |
| internal/pkg/heimdall/job.go | Prefers ResultStreamer in dispatch and adds writeResultToDestination for streaming persistence (local + S3). |
| internal/pkg/heimdall/handler.go | Encodes HTTP responses via json.NewEncoder instead of marshal-then-write. |
| internal/pkg/aws/s3.go | Adds StreamToS3 helper using io.Pipe and the AWS SDK uploader for streaming writes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Plugin results (HTTP response, result file, S3 object) are now written
incrementally instead of being fully buffered into memory first.
- pkg/plugin: new optional ResultStreamer interface (StreamResult),
mirroring the existing HealthChecker pattern.
- job.go: runJob dispatches to StreamResult when a handler implements
it, falling back to Execute otherwise; applies to both sync and
async jobs.
- handler.go: the generic payload handler encodes JSON straight to the
ResponseWriter instead of marshaling into a byte slice first.
- pkg/result: RowWriter incrementally emits the {"columns":...,
"data":...} shape; StreamRows (formerly FromRows, refactored in
place) streams *sql.Rows straight through it row by row instead of
materializing a Result.
- aws.StreamToS3: pipes writer output straight into an S3 multipart
upload instead of buffering the object.
- postgres/snowflake/starrocks: implement StreamResult using the above
primitives; Execute is now an explicit "not implemented" stub since
it's unreachable once StreamResult is present.
- writeResultToDestination writes straight to the final path/object
(no temp file). getJobFile gates result-file reads on job status
(Succeeded) so a partial or failed write is never observed; stdout
and stderr are unaffected since they're meant to be tailed live.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
prasadlohakpure
force-pushed
the
fix/stream-plugin-responses
branch
from
August 14, 2026 08:27
44ce65d to
fb9b440
Compare
RowWriter has one caller in this package (StreamRows) and no reason to live in its own file; merging keeps all Result-shaped output code in one place. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Revert starrocks back to its pre-streaming form -- it already buffers every endpoint's rows in-process before writing (rowsByEndpoint), so the memory win there is smaller than postgres/snowflake's true row-by-row streaming, and it doesn't have a *sql.Rows to hand to StreamRows. Deferring it keeps this PR to the plugins where streaming actually removes a full in-memory copy, and where the read source (*sql.Rows) is uniform. With starrocks out, StreamRows has exactly one caller, so the standalone RowWriter type/state machine is gone too -- it's now a single self-contained function, about a third smaller. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
fmt.Fprintf boxes its args as interface{} and re-parses the format
string on every call; running it once per row measurably increased
allocations (found while re-benchmarking: +43%/+42% allocs/op vs the
prior RowWriter-based version's +22%/+21%). Two direct WriteString/
Write calls do the same job without the fmt overhead, restoring the
original allocation profile.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…p storeResults errors handler.go: payloadHandler called writeAPIError after headers/200 were already committed on an Encode failure -- that WriteHeader call is a no-op and w.Write appends a JSON error blob onto a partially-written success body, corrupting it, plus double-counts the error metric. Log and stop instead. job.go: storeResults' error was discarded by both call sites, so a failed result write (local IO or S3) still left the job marked Succeeded. The synchronous call (StoreResultSync jobs, and all async jobs) now fails the job and records the error; the fire-and-forget goroutine (sync jobs without StoreResultSync) can't retroactively change a status already returned to the caller, so it logs instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rop it Previous fix (a50c448) dropped the response body entirely on Encode failure to avoid the superfluous WriteHeader call -- but that also silently lost the error message for the caller. json.Encoder.Encode marshals into an internal buffer before ever calling w.Write, so most Encode failures (bad type, NaN, etc.) happen before any body bytes reach the client; only the 200 status is already committed. Split writeAPIError's body construction into errorPayload() and write it directly here, skipping only the redundant WriteHeader/duplicate log. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
writeAPIError goes back to its original single-function shape. The encode-error branch in payloadHandler builds its own small response map inline instead -- it's the only other caller, so a shared helper wasn't earning its keep, and this keeps writeAPIError's diff against origin/main at zero. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…eResults error handling Per explicit request: back to writeAPIError(w, err, result) + LogAndCountError after headers/200 are committed (reintroduces the double-WriteHeader/double-count Copilot flagged), and storeResults' error dropped by both call sites in job.go again. Co-Authored-By: Claude Sonnet 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.
Problem
Sync job responses and result persistence were fully materializing query results in memory before writing them out:
payloadHandler(handler.go) didjson.Marshal(result)into a[]byte, thenw.Write(...)— buffering the entire response before the first byte reached the client.storeResults(job.go) did the samejson.Marshal+ write pattern for the result file/S3 object.[][]anyin memory (viaFromRowsor per-endpoint accumulation) before handing it back, regardless of result size.For large result sets this meant multiple full copies of the same data alive in memory at once, and no bytes reaching the client/destination until everything was ready.
Changes
Response and persistence encoding
handler.go: encode directly into thehttp.ResponseWriterviajson.NewEncoderinstead of marshal-then-write.job.go/internal/pkg/aws/s3.go:storeResultsnow encodes directly into the destination (local file via a temp-file-then-rename for atomicity, or S3 via a newStreamToS3helper backed byio.Pipe+ the multipart uploader) instead of marshaling to a[]bytefirst.Optional streaming plugin contract
pkg/plugin/plugin.go: added an optionalResultStreamerinterface (mirrors the existingHealthCheckerpattern) — a plugin holding a live row cursor can implementStreamResult(ctx, w, ...)to write rows straight to the destination as they're fetched, instead of accumulating them.job.go: dispatch prefersStreamResultoverExecutewhen a plugin implements it, for both sync and async execution.pkg/result/stream.go: sharedRowWriter+StreamRowshelper so any row source (currentlydatabase/sql) can emit the{"columns":...,"data":...}shape incrementally, one row at a time.StreamResultforpostgresandsnowflake(both hold a*sql.Rowscursor) andstarrocks(fans out to multiple backend endpoints concurrently, then streams each endpoint's rows to the destination in order, releasing each endpoint's buffer as soon as it's written).plugin.Handler.Executeis unchanged — required by the base interface, but now unreachable for any plugin implementingResultStreamer; each plugin'sExecutereturns an explicit error rather than carrying a second, unexercised implementation of the same query logic.Smaller fixes defered for now
starrocks: fans a query out to multiple backend endpoints concurrently and already buffers each endpoint's full row set in memory (rowsByEndpoint [][][]any) before anything gets written — it has no live cursor to stream from. Wiring it into RowWriter would require its own shared-writer plumbing, increasing the scope suffficiently.Behavior notes
ResultStreamer, the result is no longer inlined in thePOST /api/v1/jobresponse body — it's written straight to the result destination, and the caller fetches it via the existingGET /api/v1/job/{id}/resultendpoint (already streams the file to the client). Non-streaming plugins are unaffected.json.Encoder) where they previously did not.Benchmarks
Local micro-benchmarks (Apple M2, Go
testing.B/-benchmem), synthetic 4-column rows (int64,string,float64,bool), comparing the pre-change approach (build the full[][]any, thenjson.Marshalit) against the post-change approach (stream row-by-row) at 1,000 and 10,000 rows. Re-run after scoping this PR down to postgres/snowflake (starrocks streaming deferred to a follow-up) and collapsing the row writer into a single self-containedStreamRowsfunction.Peak retained memory (the actual point of this change)
Cumulative
-benchmembytes/op don't capture this — this is live heap held at once for the duration a result sits in memory before being written out.Row processing (
pkg/result: accumulate-then-marshal vs. stream)HTTP response encode (
handler.go: marshal-then-write vs.Encoder.Encode)Testing:
Verified S3 result files as well as API response against live PROD running jobs
job_id: 00a04fc7-b33e-4aad-bb16-4bf3f49d10d7
job_id: 554e4a3c-5f1f-48fd-a993-68db76675e0e
job_id: a71fdd55-1373-4325-bcd0-9b11dd8024b3
Contract change:
Previously under process result files used to display :
{"error":"operation error S3: GetObject, https response error StatusCode: 404, RequestID: VHD6WMJVAS4ZX6E0, HostID: FGmBuPPDxSMPFpgcxDgR/q9xXfRgxQYKJCGtSHyHbwaU5+dt3R29VgyJBPZPBFnpGaRwU3mMCv0=, NoSuchKey: "}Now: underprocess result files will display

Reason:
With now data streaming enabled, local result/ s3 file state can be incomplete since its not a whole copy but rather data being streamed. So we will defer to show the file unless streaming complete.
Takeaway: memory footprint drops sharply at both sizes (the actual goal), CPU time is roughly a wash or slightly higher for row-by-row streaming due to per-row allocation overhead — a deliberate, expected memory/CPU tradeoff, not a free win.