Refactor StreamWriter to use temp files for constant memory usage#43
Conversation
Rewrite the StreamWriter to use temp file-based streaming instead of accumulating all rows in memory. Each write_row() call now serializes the row as XML and appends it to a NamedTempFile, keeping memory usage constant (O(1)) regardless of the number of rows. Key changes: - Use inline strings (t="inlineStr") instead of SST references, eliminating the need for SST index remapping - StreamedSheetData holds the temp file reference and pre-built XML fragments (sheetViews, cols, mergeCells) - During save, worksheet XML is composed by streaming header + temp file rows + footer directly into the ZIP entry - Cell values in streamed sheets cannot be read before save (save and reopen to read data) - Add new NAPI methods: setFreezePanes, setColStyle, setColVisible, setColOutlineLevel, writeRowWithStyle - Update English and Korean documentation https://claude.ai/code/session_012QobdGuj2a6P915GgXArXN
…ehensive tests - Fix build_row_xml to emit <v> element when Formula has a cached result, instead of silently discarding it - Remove duplicate tempfile dependency from sheetkit-core Cargo.toml - Add 10 unit tests in stream.rs: Date, RichString, Formula with result (numeric/string/bool/none), error XML escaping, empty rows - Add 6 integration tests in sheet_ops.rs: freeze panes roundtrip, merge cells roundtrip, column widths roundtrip, multiple streamed sheets, delete streamed sheet, combined features roundtrip - Add 10 Node.js tests for new StreamWriter methods: freeze panes, merge cells, column widths, writeRowWithStyle, setColStyle, setColVisible, setColOutlineLevel, multiple sheets, combined features https://claude.ai/code/session_012QobdGuj2a6P915GgXArXN
Code reviewFound 1 issue that should be addressed: CLAUDE.md compliance issue in
|
Nebu1eto
left a comment
There was a problem hiding this comment.
Thanks for the substantial StreamWriter rewrite and test expansion. I rebuilt and reran both Rust and Node test suites before reviewing (cargo build --workspace, cargo test --workspace, pnpm build, pnpm test) and found three blocking behavioral issues.
-
[P1] Formula cached result types are not serialized correctly
- File:
crates/sheetkit-core/src/stream.rs - In
CellValue::Formula { result: ... }, cachedString/Bool/Errorresults are written as<v>...</v>without setting the corresponding cell type (t="str",t="b",t="e"). - On reopen,
xml_cell_to_valueusestto decode formula results. This causes:- bool result -> decoded as
Number(1.0) - string/error result -> dropped (
result: None)
- bool result -> decoded as
- File:
-
[P1] Edits after
apply_stream_writerare silently ignored on save- File:
crates/sheetkit-core/src/workbook/io.rs - Save path always prioritizes
streamed_sheetsfor that index and writes temp-file payload directly. - Any later mutation on the same sheet (e.g.
set_cell_value) updates the placeholderWorksheetXml, but that data is never serialized because streamed data wins. - Repro: write
oldvia stream, callset_cell_value("S", "A1", "new"), save+reopen => value is stillold.
- File:
-
[P1]
copy_sheetdoes not copy streamed payload- File:
crates/sheetkit-core/src/workbook/sheet_ops.rs copy_sheetclones worksheet metadata/parallel vectors, but does not clone source entry fromstreamed_sheets.- For streamed source sheets, copied target sheet saves as empty (placeholder only).
- Repro: stream-write
Src!A1 = "x",copy_sheet("Src", "Dst"), save+reopen =>Src!A1 = "x",Dst!A1 = Empty.
- File:
Given these data integrity issues, this should be fixed before merge.
[P1] Formula cached result type attributes: - Set t="str", t="b", t="e" on <c> element in build_row_xml when Formula has a cached String/Bool/Error result. Without these, xml_cell_to_value could not decode the result on reopen (bool -> Number(1.0), string/error -> result: None). - Add FormulaString handling in xml_cell_to_value formula branch so t="str" formulas decode their cached string result correctly. [P1] Edits after apply_stream_writer silently ignored: - Add invalidate_streamed() helper that removes a sheet's streamed entry. - Call it from worksheet_mut(), set_cell_value(), and set_cell_values() so that any mutation to a streamed sheet invalidates the temp file data and uses the normal WorksheetXml serialization path on save. [P1] copy_sheet does not copy streamed payload: - Add StreamedSheetData::try_clone() that creates a new temp file with identical contents plus cloned XML fragments. - Call it from copy_sheet() when the source sheet has streamed data. Also update CLAUDE.md Tech Stack: tempfile is now a runtime dependency. https://claude.ai/code/session_012QobdGuj2a6P915GgXArXN
Summary
Refactor
StreamWriterto write row data directly to temporary files on disk instead of accumulating rows in memory. This enables writing sheets with millions of rows while maintaining constant memory usage, regardless of the number of rows written.Key Changes
Streaming to disk: Replace in-memory
Vec<Row>with aBufWriterwriting to atempfile::NamedTempFile. Each row is serialized to XML and written immediately.Inline strings instead of SST: Change from shared string table references to inline strings (
<is><t>...</t></is>). This eliminates the need for SST index remapping when applying the stream writer to a workbook and allows each row to be serialized independently.New
StreamedSheetDatastruct: Introduced to hold the temporary file and pre-built XML fragments (sheetViews, cols, mergeCells) for later composition during save.API changes:
finish()→into_streamed_data()returns(sheet_name, StreamedSheetData)instead of XML bytesinto_parts()andinto_worksheet_parts()methodssst()getter since streamed sheets no longer use the shared string tableNew helper functions:
build_worksheet_header()andbuild_worksheet_footer()to compose the full worksheet XMLwrite_streamed_sheet()to write streamed data directly to the ZIP archive during savebuild_row_xml()to serialize individual rows with inline stringsxml_escape_into()for efficient XML escapingWorkbook integration:
streamed_sheets: HashMap<usize, StreamedSheetData>to store streamed dataapply_stream_writer()to store streamed data instead of merging SST entriesDocumentation updates: Updated API docs and architecture docs to reflect that streamed sheets use inline strings and that cell values cannot be read before save/reopen.
Test updates: Updated tests to verify inline string usage and added tests for save/reopen roundtrips.
Implementation Details
StreamWriternow implementsSendviaunsafe implsince the temp file is exclusively ownedStreamedSheetDatais droppedinto_streamed_data()and reused during savehttps://claude.ai/code/session_012QobdGuj2a6P915GgXArXN