Repository navigation
Fixes from three audits: safe path writes, XLSX metadata parsing, exact enums, and wide-header, pipeline and shared-string performance - #157
Open
GabrielMarquezMatte wants to merge 20 commits into
Open
GabrielMarquezMatte wants to merge 20 commits into
GabrielMarquezMatte wants to merge 20 commits into
Conversation
Excel.EncryptPackage(path), xl_write_typed and xl_open_write_handle opened the destination with FileMode.Create, so a rejected package, a rejected cell (NaN) or a cancellation left an empty or partial file where the caller's previous one had been. They now write a temporary file beside the destination through the new StagedFile and move it over the destination only on success; on failure or cancellation the temporary file is deleted and the old file is left as it was. A path write handle commits only when xl_close_write_handle returns XL_OK. Stream destinations are unchanged.
…tput convert had five problems: - A failed conversion had already truncated --output. It now writes through StagedFile, like the library's path writers. - Row.Cells skips empty cells, but each cell went to the next output column, so A,<empty>,C came out as A,C. Gaps are now skipped. - A boolean was true unless its text began with '0', so "false" became true. It now goes through ExcelCellReaders.Bool and fails on anything else. - Every number went through double, rounding integers above 2^53. Text numbers now keep their exact long or decimal value when it is at least as faithful as the double. - A CSV source has no sheet name, which every spreadsheet writer rejects. The output sheet is now named Sheet1.
Valid XML that the metadata scanners misread: - Whitespace around '=' (r = "C1"): the cell scanner dropped the coordinate and shifted the cell left, and Attr matched only the exact ' name=' spelling. Both now share XlsxXml.NextAttribute, which allows XML whitespace around '=' and matches whole attribute names. - A newline or tab after the element name (<xf\n ...): the style scan searched for '<xf ' with a literal space and lost date styles. Tags are now matched by name with a boundary check. - A '>' inside a quoted value (formatCode="[>1]yyyy-mm-dd") ended the tag early, and a commented-out <sheet> or <numFmt> was read as real. TagSpanEnumerable now skips comments and quoted '>'. - A relationship target with ./ or ../ segments was looked up verbatim. NormalizePart now resolves dot segments and rejects a target that climbs above the package root. The canonical cell fast path is unchanged.
EnumCache converted every member to Int64 while initializing, so an enum with a ulong member above long.MaxValue threw TypeInitializationException even for a plain name. Its name map came from ToString() over GetValues, which drops alias names. And a numeric cell went through double, so 9007199254740993 resolved to the member declared as 9007199254740992. Values are now keyed by Int128, read in the underlying type's own signed or unsigned domain. Names come from Enum.GetNames, and integer text is parsed exactly before any double fallback. Fractions, non-finite and undeclared values still fail.
The hint name flattened '.' into '_', so A.B_C and a nested A.B.C both became A_B_C.ExcelRowMap.g.cs and the second AddSource failed with CS8785. The fully qualified name now keeps its dots.
DateTime.FromOADate rounds to the millisecond, so a serial just under the 2958466 bound, such as 2958465.9999999995, rounds past DateTime.MaxValue and throws ArgumentException out of a Try method. It now returns false; the last representable millisecond still reads.
…bytes _write_all took a write() returning None as everything written, the convention of old file-like sinks. For io.RawIOBase, None means the write would block and nothing was taken, so write_workbook, encrypt_package and the writer handle silently lost the output. A raw stream now raises BlockingIOError; other sinks keep the old reading.
…opens ToCsvReaderOptions always set the CSV override fields, filling them from the CSV defaults when csv_max_cell_bytes and csv_intern_strings were left at 0. The overrides then beat ExcelReaderOptions, so a native open with max_cell_bytes = 16 still accepted a 200,000-byte CSV cell, and the global intern_strings setting was ignored. The overrides are now set only when the native CSV fields are, and xl_open_stream's CSV path, which called OpenCsv directly, now builds its options through Excel.CsvOf like every managed entry point.
Close set _closed before finishing the workbook, so after a failed finish (an XLSX with no sheet) a second xl_write_handle_bytes skipped the work, cleared the error and returned a 22-byte empty ZIP as success. The handle now records the failure, and every later xl_write_handle_bytes or xl_close_write_handle returns it again; it is marked closed only after the finish succeeds.
With ParseText, a column mixing booleans and the numbers 0 and 1 is inferred as BoolColumn. That test looked only at the cell's text, and XLSB and XLS numbers carry none, so the same values inferred as BoolColumn from XLSX but StringColumn from XLSB or XLS. A number without text is now checked by its stored value.
The temporary file was named after the destination plus a GUID, so a valid destination name near the file system's 255-character limit made the temporary name too long and every staged write failed. It is now .excelreader-<guid>.tmp in the destination's directory.
xl_write_typed and xl_write_typed_to_memory kept their own copy of the handle's format switch (writer per format, option mapping, parallel writes) and a separate workbook lifecycle helper around the same WriteSheetBody. They now open a NativeWriterHandle over the stream, call WriteSheet and Close, and dispose it. A handle disposed without a successful Close still held the sheet that failed, and ending that sheet could throw over the original error. Its row and sheet now go through FailureCleanup: that output is discarded anyway (the staged file is deleted, the memory buffer never returned). WritePathBenchmark gains BulkTyped_* (100k rows x 4 string columns, serial and parallel XLSX/XLSB). Before and after are equal within noise with identical allocations, e.g. XLSB serial 18.06 +/- 0.84 ms against 18.61 +/- 0.97 ms over six launches.
SetLastError already encodes the message into a pinned thread-local buffer for xl_last_error_ptr, but LastError counted and encoded the string again on every call. It now copies the stored bytes, keeping the zero-length and BufferTooSmall behavior, with no partial copy.
TryResolveColumns searched the header row from its first cell for every candidate name, decoding and trimming each visited cell again, so binding a wide header by name was quadratic: 4,096 named columns took 356 ms and allocated 388 MB before the first data row. The header is now read once into an OrdinalIgnoreCase dictionary where the first column with a name wins, and each spec's candidates are looked up in order through its ReadOnlySpan<char> lookup, trimmed on the span. Index-only specs and header_row 0 never build it. NativeWideHeaderBenchmark, a header-only CSV bound by name: 512 columns 4.86 ms / 6.44 MB -> 0.25 ms / 0.50 MB, 4,096 columns 356 ms / 388 MB -> 2.79 ms / 4.07 MB.
…early ChunkProducer detached each chunk from its own cleanup before awaiting emit, so a chunk whose hand-off failed under cancellation belonged to nobody. The runner also left chunks still queued for workers when the results were abandoned. Those pooled arrays were never returned: disposing the 65K-row XLSX pipeline after its first result lost one 256 KiB rental every time. A chunk whose emit throws now goes back to the pool in the producer, for both regular and final chunks, and after every producer and worker task has stopped the runner returns the chunks no worker took. A chunk a worker accepted is still returned only by that worker.
ToXlsxBytes and ToXlsbBytes serialized every shared string into one growing buffer before compressing it, so finishing a workbook with many distinct texts held the whole part at once: 96 MB of pooled buffer for 400,000 128-character strings in XLSX, 192 MB in XLSB. The part is now written to its ZIP entry in pieces of about 64 KiB through one reused buffer, sync and async. The bytes, indices, counts, whitespace preservation, escaping and XLSB framing are unchanged. 16 KiB and 256 KiB pieces timed the same within noise. Buffer peak while ending the workbook, 400k strings: XLSX 96 MB -> 512 KiB, XLSB 192 MB -> 512 KiB, flat from 1k to 400k. SharedStringWriteBenchmark (100k distinct texts): XLSX 30.35 -> 30.17 ms, XLSB 37.99 -> 33.02 ms, allocations unchanged.
…h copy StringColumnAppender formatted every cell into a scratch array sized to the cell, then the Arrow builder copied it again. Text cells already hold their UTF-8 bytes, so they are now appended directly; the scratch is a fixed 32 bytes, used only for numbers stored without text (XLS, XLSB). ArrowConversionBenchmark.CsvLongText (500 x 64 KiB cells, six launches): 27.14 -> 24.46 ms, 64.01 -> 63.95 MB. CsvAllString and XlsbTyped are unchanged within noise.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #157 +/- ##
==========================================
+ Coverage 91.46% 91.50% +0.03%
==========================================
Files 284 285 +1
Lines 22904 23015 +111
Branches 3697 3718 +21
==========================================
+ Hits 20950 21059 +109
- Misses 1375 1378 +3
+ Partials 579 578 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
added 2 commits
October 26, 2026 18:06
…to include shared string and wide header benchmarks
…ark output messages feat(tests): add comprehensive tests for arrow conversion handling various data types and nulls refactor(writer): optimize column handling and validity bitmap processing in Arrow integration
This branch has not been deployed
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
Seventeen commits since v7.3.0, from three audits (two bug hunts, one code-reduction, one performance). Bug fixes and internal changes only: the public API files, the C ABI and the exports are unchanged; the header gains one comment.
Writes no longer destroy the existing file on failure
Excel.EncryptPackage(path, …),xl_write_typed,xl_open_write_handleand the CLI's--outputopened the destination withFileMode.Create. A rejected package, a rejected cell (NaN) or a cancellation left an empty or partial file where the previous one had been.A path write handle replaces its file only when
xl_close_write_handlereturnsXL_OK. A handle whose finish fails keeps returning that error; a retry used to return a 22-byte empty ZIP as success.Reader fixes
=(r = "C1") dropped the cell coordinate and shifted the cell left. A newline after an element name (<xf\n …) lost date styles.>inside a quoted value (formatCode="[>1]yyyy-mm-dd") ended the tag early, and a commented-out<sheet>or<numFmt>was read as real../or../are now resolved. A target that climbs above the package root throwsInvalidDataException.TryGetDateTimethrewArgumentExceptionfor serials just under the upper bound, becauseFromOADaterounds to the millisecond. It now returns false.BoolColumnfor booleans mixed with 0/1 in XLSX butStringColumnin XLSB and XLS, whose numbers carry no text. All three formats now agree.max_cell_bytesandintern_strings. Thecsv_*fields now override them only when set.Enum parsing
CLI
convertA,<empty>,Cno longer becomesA,C.falsebooleans stay false. Text that isn't a boolean is now an error; it used to becometrue.Sheet1.Other fixes
A.B_Cand a nestedA.B.Cproduced the same file name (CS8785). The full type name now keeps its dots.io.RawIOBase) whosewrite()returns None meant would-block, and the bytes were silently dropped. It now raisesBlockingIOError.Performance
xl_parse_typedsetup)Cleanups, no behavior change:
xl_write_typedwrites throughNativeWriterHandleinstead of its own copy of the format switch (about −45 lines). Bulk-write benchmarks are equal within noise, with the same allocations.xl_last_errorcopies the bytesSetLastErroralready encoded.New benchmarks:
NativeWideHeaderBenchmark,SharedStringWriteBenchmark,WritePathBenchmark.BulkTyped_*,ArrowConversionBenchmark.CsvLongText.Behavior changes worth noting
BoolColumnfor XLSB and XLS columns that previously inferred asStringColumn.max_cell_bytescan now reject cells it accepted before.convertfails on a boolean cell it cannot parse, and its number text can differ from before because it is now exact.Verification