perf: batch the raw data export instead of holding it in memory - #1504
Merged
Conversation
The export built the entire result before writing anything: every answer id, every AnswerValue for those answers, one dictionary per row, and then a SheetData DOM holding every cell. That is what forced the row cap down to 25 000 in the first place, and the cap was a stopgap rather than a fix. Answers are now fetched ExportBatchSize (2000) at a time and streamed straight into the sheet, so peak memory is one batch regardless of how large the export is. - IRawDataExcelWriter replaces WriteRawDataToExcelFile. It opens the workbook, writes the header, takes rows one at a time via OpenXmlWriter, and finalises on Complete. Nothing accumulates. - RawDataService.ExportToFile owns the loop and keeps the SDK context open across batches, which it has to: every batch reads from the same query. Splitting the old Build into ResolveScope (offset-independent work: item, schema, answer query) and BuildRows (one window) lets the paged endpoint and the export share the pivot without sharing a lifetime. - The controller no longer creates files. It calls ExportToFile, streams the result and deletes it. - Export batches order by FinishedAt then Id. The tie-breaker added earlier is what makes batching safe: without it, answers sharing a timestamp could be duplicated into one batch and skipped from another. - ExportRowLimit rises 25 000 -> 250 000. Memory is no longer the binding constraint, so the cap now bounds file size and request duration. - Free-text answers are stripped of XML-illegal control characters, which previously produced a workbook Excel refuses to open. RawDataExportUTests covers the writer without needing a database: 5000 rows land in the file, a missing value leaves a gap at its own column reference rather than shifting later columns, a vertical tab is removed while tabs and newlines survive, an empty result still yields a valid header-only workbook, and abandoning the writer without completing does not throw. Verified locally, and checked non-vacuous by bypassing the sanitiser and watching the control-character test fail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EnP42zmHAgo2NZQsa6zdhG
Code review found the batching traded one scaling problem for a worse one. Each batch was Skip(offset).Take(2000) against a query whose WHERE is an IN subquery - a Union over AnswerValues for compared items - so MySQL re-evaluated that subquery, re-sorted the whole matching set and discarded offset rows on every batch. Quadratic where the original was linear, and the cap had just been raised tenfold, multiplying it by a hundred. The comment claiming the cap now bounded request duration had it exactly backwards. RawDataPaging replaces offsets with keyset paging on (FinishedAt, Id), the total order the tie-breaker already guaranteed. That makes the export linear and index-seekable, and fixes a correctness problem in the same move: with OFFSET, an answer finished mid-export sorts to the front and shifts every later window, duplicating rows across batch boundaries and dropping others. A cursor is anchored to a value, so new answers sort above it and are simply not seen - a consistent view as of the export's start. Also from review: - ExportRowLimit drops 250 000 -> 50 000. The honest reason is in the comment: the whole file is still generated before the response starts, so wall-clock against a proxy timeout is the real constraint, and this number is not derived from measuring a cap-sized export. Streaming to the response body would remove the ceiling properly. - A truncated export no longer passes as complete: rows written are compared against the expected total and a mismatch is logged. - Column headers are sanitised. They come from question text and option translations - the same user-entered tables as the answers - so an unsanitised header would corrupt every export of that survey rather than one unlucky row. - Restored the null guard on value.ToString(), wrapped the document dispose so a close failure cannot replace the exception being unwound, and corrected two comments that still justified the old cap. - ExportBatchSize becomes a settable property so tests can cross batch boundaries without seeding thousands of answers. The batching itself was untested, which was the review's main point about coverage. Two tests now page through the seeded answers in batches of three and assert the stitched result equals a single ordered read exactly once, including through a timestamp collision - the case that has no defined order without the Id tie-breaker. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EnP42zmHAgo2NZQsa6zdhG
CI reported 18 passed, 1 skipped, and the skip was KeysetPaging_HandlesAnswersSharingATimestamp: the seeded database holds no two answers sharing a FinishedAt. That left the timestamp-collision case unverified - the exact situation the Id tie-breaker exists for, and the one where paging silently duplicates and drops rows if it is wrong. Two in-memory tests pin the predicate where the collision can actually be constructed: three answers on one timestamp plus one on another, walked a single row at a time so every step crosses the collision, and a no-cursor case asserting the first batch starts at the newest answer. These complement rather than replace the database test. That one proves the expression translates to SQL; these prove it is correct when timestamps tie. Both run without a database, so both were verified locally, and the tie-break test was checked non-vacuous by removing the Id comparison and watching it fail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EnP42zmHAgo2NZQsa6zdhG
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
1 participant
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.
The last follow-up from the raw data table spec. #1502 shipped the export with a 25 000-row cap, which was a stopgap for a memory problem rather than a fix for it.
The export built the entire result before writing anything: every answer id, every
AnswerValuefor those answers, one dictionary per row, then aSheetDataDOM holding every cell.Answers are now fetched 2000 at a time and streamed straight into the sheet, so peak memory is one batch no matter how large the export is.
Changes
IRawDataExcelWriterreplacesWriteRawDataToExcelFile. It opens the workbook, writes the header, takes rows one at a time viaOpenXmlWriter, and finalises onComplete. Nothing accumulates.RawDataService.ExportToFileowns the loop and keeps the SDK context open across batches — it has to, since every batch reads from the same query. The oldBuildsplits intoResolveScope(offset-independent work) andBuildRows(one window), so the paged endpoint and the export share the pivot without sharing a lifetime.ExportToFile, streams the result, deletes it.ExportRowLimit25 000 → 250 000. Memory is no longer the binding constraint; the cap now bounds file size and request duration.Batching correctness rests on the
FinishedAt, thenIdordering. That tie-breaker went in with #1502: without it, answers sharing a timestamp could be duplicated into one batch and skipped from another.Testing
RawDataExportUTestscovers the writer and needs no database, so unlike the rest of this suite it was run locally before pushing — 6/6 passing:Checked non-vacuous by bypassing the sanitiser and confirming the control-character test fails.
dotnet buildclean. The database-backed suites run in CI as usual.🤖 Generated with Claude Code
https://claude.ai/code/session_01EnP42zmHAgo2NZQsa6zdhG