From f0303d2b9b4d873f090aeafec8536db466f2fdc2 Mon Sep 17 00:00:00 2001 From: GabrielMarquezMatte Date: Mon, 26 Oct 2026 13:17:11 -0300 Subject: [PATCH 01/20] fix(writer): replace a path destination only after the write succeeds 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. --- src/ExcelReader.Core/Reader/Excel.Encrypt.cs | 12 ++-- .../Writer/Internal/StagedFile.cs | 58 +++++++++++++++++++ .../Writer/NativeWriterHandle.cs | 4 ++ src/ExcelReader.Native/Writer/WriteApi.cs | 16 +++-- src/ExcelReader.Native/include/excelreader.h | 2 + .../Crypto/EncryptedPackageWriteTests.cs | 46 +++++++++++++++ .../Native/NativeApiTests.Writer.cs | 25 ++++++++ .../Native/NativeWriterHandleTests.cs | 51 ++++++++++++++++ 8 files changed, 203 insertions(+), 11 deletions(-) create mode 100644 src/ExcelReader.Core/Writer/Internal/StagedFile.cs diff --git a/src/ExcelReader.Core/Reader/Excel.Encrypt.cs b/src/ExcelReader.Core/Reader/Excel.Encrypt.cs index c1d4968f..cb1e80c4 100644 --- a/src/ExcelReader.Core/Reader/Excel.Encrypt.cs +++ b/src/ExcelReader.Core/Reader/Excel.Encrypt.cs @@ -1,4 +1,5 @@ using ExcelReader.Core.Crypto; +using ExcelReader.Core.Writer.Internal; namespace ExcelReader.Core.Reader { @@ -43,8 +44,9 @@ public static void EncryptPackage(string packagePath, string destinationPath, Ex ArgumentException.ThrowIfNullOrEmpty(packagePath); ArgumentException.ThrowIfNullOrEmpty(destinationPath); using FileStream package = File.OpenRead(packagePath); - using FileStream destination = new(destinationPath, FileMode.Create, FileAccess.Write, FileShare.None); - PackageEncryptor.Encrypt(package, destination, password); + using StagedFile destination = new(destinationPath); + PackageEncryptor.Encrypt(package, destination.Stream, password); + destination.Commit(); } /// Asynchronous counterpart to . @@ -83,11 +85,11 @@ public static async ValueTask EncryptPackageAsync(string packagePath, string des bufferSize: 4096, useAsync: true); await using (package.ConfigureAwait(false)) { - FileStream destination = new(destinationPath, FileMode.Create, FileAccess.Write, - FileShare.None, bufferSize: 4096, useAsync: true); + StagedFile destination = new(destinationPath, useAsync: true); await using (destination.ConfigureAwait(false)) { - await PackageEncryptor.EncryptAsync(package, destination, password, ct).ConfigureAwait(false); + await PackageEncryptor.EncryptAsync(package, destination.Stream, password, ct).ConfigureAwait(false); + await destination.CommitAsync().ConfigureAwait(false); } } } diff --git a/src/ExcelReader.Core/Writer/Internal/StagedFile.cs b/src/ExcelReader.Core/Writer/Internal/StagedFile.cs new file mode 100644 index 00000000..a0781f64 --- /dev/null +++ b/src/ExcelReader.Core/Writer/Internal/StagedFile.cs @@ -0,0 +1,58 @@ +namespace ExcelReader.Core.Writer.Internal +{ + /// + /// A file written beside its destination and moved over it only by , so a failed, + /// canceled or abandoned write leaves an existing destination untouched and removes the partial file. + /// + internal sealed class StagedFile : IDisposable, IAsyncDisposable + { + private readonly string _destination; + private readonly string _temporary; + private bool _committed; + + internal FileStream Stream { get; } + + internal StagedFile(string destination, bool useAsync = false) + { + _destination = Path.GetFullPath(destination); + _temporary = Path.Combine(Path.GetDirectoryName(_destination) ?? string.Empty, + $".{Path.GetFileName(_destination)}.{Guid.NewGuid():N}.tmp"); + Stream = new FileStream(_temporary, FileMode.CreateNew, FileAccess.Write, FileShare.None, + bufferSize: 4096, useAsync); + } + + internal void Commit() + { + Stream.Dispose(); + File.Move(_temporary, _destination, overwrite: true); + _committed = true; + } + + internal async ValueTask CommitAsync() + { + await Stream.DisposeAsync().ConfigureAwait(false); + File.Move(_temporary, _destination, overwrite: true); + _committed = true; + } + + public void Dispose() + { + if (_committed) + { + return; + } + FailureCleanup.Dispose(Stream); + File.Delete(_temporary); + } + + public async ValueTask DisposeAsync() + { + if (_committed) + { + return; + } + await FailureCleanup.DisposeAsync(Stream).ConfigureAwait(false); + File.Delete(_temporary); + } + } +} diff --git a/src/ExcelReader.Native/Writer/NativeWriterHandle.cs b/src/ExcelReader.Native/Writer/NativeWriterHandle.cs index 4056e0f6..b017512b 100644 --- a/src/ExcelReader.Native/Writer/NativeWriterHandle.cs +++ b/src/ExcelReader.Native/Writer/NativeWriterHandle.cs @@ -1,5 +1,6 @@ using ExcelReader.Core.Writer; using ExcelReader.Core.Writer.Csv; +using ExcelReader.Core.Writer.Internal; using ExcelReader.Core.Writer.Xls; using ExcelReader.Core.Writer.Xlsb; using ExcelReader.Core.Writer.Xlsx; @@ -36,9 +37,12 @@ internal abstract class NativeWriterHandle : IDisposable internal MemoryStream? MemoryBuffer { get; set; } + internal StagedFile? StagedFile { get; set; } + public void Dispose() { Dispose(true); + StagedFile?.Dispose(); GC.SuppressFinalize(this); } diff --git a/src/ExcelReader.Native/Writer/WriteApi.cs b/src/ExcelReader.Native/Writer/WriteApi.cs index 31c68663..5063de78 100644 --- a/src/ExcelReader.Native/Writer/WriteApi.cs +++ b/src/ExcelReader.Native/Writer/WriteApi.cs @@ -2,6 +2,7 @@ using System.Text; using ExcelReader.Core.Writer; using ExcelReader.Core.Writer.Csv; +using ExcelReader.Core.Writer.Internal; using ExcelReader.Core.Writer.Xls; using ExcelReader.Core.Writer.Xlsb; using ExcelReader.Core.Writer.Xlsx; @@ -38,8 +39,9 @@ internal static int WriteTyped(ReadOnlySpan path, int format, NativeColumn { string filePath = Encoding.UTF8.GetString(path); string sheetName = options.SheetName ?? "Sheet1"; - using FileStream stream = new(filePath, FileMode.Create, FileAccess.Write, FileShare.None); - WriteToStream(stream, format, specs, table, options, sheetName, hasHeader); + using StagedFile staged = new(filePath); + WriteToStream(staged.Stream, format, specs, table, options, sheetName, hasHeader); + staged.Commit(); return NativeStatus.Ok; } catch (Exception exception) @@ -94,16 +96,17 @@ internal static int OpenWriteHandle(ReadOnlySpan path, int format, NativeW } NativeApi.ClearLastError(); string filePath = Encoding.UTF8.GetString(path); - FileStream? stream = null; + StagedFile? staged = null; try { - stream = new FileStream(filePath, FileMode.Create, FileAccess.Write, FileShare.None); - handle = NativeWriterHandle.Create(stream, format, options); + staged = new StagedFile(filePath); + handle = NativeWriterHandle.Create(staged.Stream, format, options); + handle.StagedFile = staged; return NativeStatus.Ok; } catch (Exception exception) { - stream?.Dispose(); + staged?.Dispose(); NativeApi.SetLastError(exception.Message); return NativeStatus.Error; } @@ -142,6 +145,7 @@ internal static int CloseWriteHandle(NativeWriterHandle? handle) try { handle.Close(); + handle.StagedFile?.Commit(); NativeApi.ClearLastError(); return NativeStatus.Ok; } diff --git a/src/ExcelReader.Native/include/excelreader.h b/src/ExcelReader.Native/include/excelreader.h index 384ec470..19c5cfbe 100644 --- a/src/ExcelReader.Native/include/excelreader.h +++ b/src/ExcelReader.Native/include/excelreader.h @@ -333,6 +333,8 @@ typedef struct xl_sheet_options { int32_t xl_write_sheet(xl_writer_handle* handle, const uint8_t* name, int32_t name_len, const xl_column_spec* specs, const xl_table* table, const xl_sheet_options* options); +/* A path handle writes beside its path and replaces the file only when this returns XL_OK; on failure + any existing file is left as it was. */ int32_t xl_close_write_handle(xl_writer_handle* handle); int32_t xl_write_handle_bytes(xl_writer_handle* handle, xl_buffer* out_buffer); diff --git a/tests/ExcelReader.Tests/Crypto/EncryptedPackageWriteTests.cs b/tests/ExcelReader.Tests/Crypto/EncryptedPackageWriteTests.cs index fc3da566..7ca96883 100644 --- a/tests/ExcelReader.Tests/Crypto/EncryptedPackageWriteTests.cs +++ b/tests/ExcelReader.Tests/Crypto/EncryptedPackageWriteTests.cs @@ -130,6 +130,52 @@ public void EncryptPackage_PathOverload_RoundTrips() } } + [Fact] + public void EncryptPackage_PathOverload_Keeps_The_Existing_Output_When_The_Package_Is_Invalid() + { + string directory = Path.Combine(Path.GetTempPath(), $"excelreader-encrypt-{Guid.NewGuid():N}"); + Directory.CreateDirectory(directory); + string packagePath = Path.Combine(directory, "plain.xlsx"); + string outputPath = Path.Combine(directory, "out.xlsx"); + File.WriteAllText(packagePath, "BAD"); + File.WriteAllText(outputPath, "ORIGINAL"); + try + { + Assert.Throws(() => Excel.EncryptPackage(packagePath, outputPath, Password)); + + Assert.Equal("ORIGINAL", File.ReadAllText(outputPath)); + Assert.Equal(2, Directory.GetFiles(directory).Length); + } + finally + { + Directory.Delete(directory, recursive: true); + } + } + + [Fact] + public async Task EncryptPackageAsync_PathOverload_Keeps_The_Existing_Output_When_Canceled() + { + string directory = Path.Combine(Path.GetTempPath(), $"excelreader-encrypt-{Guid.NewGuid():N}"); + Directory.CreateDirectory(directory); + string outputPath = Path.Combine(directory, "out.xlsx"); + File.WriteAllText(outputPath, "ORIGINAL"); + try + { + using CancellationTokenSource canceled = new(); + await canceled.CancelAsync(); + + await Assert.ThrowsAnyAsync(async () => + await Excel.EncryptPackageAsync(EncryptedFixtures.PlainPath("agile-aes256-sha512.xlsx"), outputPath, Password, canceled.Token)); + + Assert.Equal("ORIGINAL", File.ReadAllText(outputPath)); + Assert.Equal([outputPath], Directory.GetFiles(directory)); + } + finally + { + Directory.Delete(directory, recursive: true); + } + } + [Fact] public async Task EncryptPackageAsync_StreamOverload_RoundTrips() { diff --git a/tests/ExcelReader.Tests/Native/NativeApiTests.Writer.cs b/tests/ExcelReader.Tests/Native/NativeApiTests.Writer.cs index 70926917..decef2a2 100644 --- a/tests/ExcelReader.Tests/Native/NativeApiTests.Writer.cs +++ b/tests/ExcelReader.Tests/Native/NativeApiTests.Writer.cs @@ -1244,6 +1244,31 @@ public void WriteTyped_Should_Reject_A_Spec_With_More_Than_One_Name() } } + [Fact] + public void WriteTyped_Should_Keep_The_Existing_File_When_A_Cell_Is_Rejected() + { + string directory = Path.Combine(Path.GetTempPath(), $"excelreader-native-{Guid.NewGuid():N}"); + Directory.CreateDirectory(directory); + string path = Path.Combine(directory, "out.xlsx"); + File.WriteAllText(path, "ORIGINAL"); + NativeTable table = BuildFloat64Table([double.NaN]); + try + { + NativeColumnSpec[] specs = [new() { Names = ["x"], Type = NativeColumnType.Float64 }]; + + Assert.Equal(NativeStatus.Error, WriteApi.WriteTyped( + Encoding.UTF8.GetBytes(path), NativeFormat.Xlsx, specs, table, DefaultWriteOptions())); + + Assert.Equal("ORIGINAL", File.ReadAllText(path)); + Assert.Equal([path], Directory.GetFiles(directory)); + } + finally + { + FreeBuiltTable(ref table); + Directory.Delete(directory, recursive: true); + } + } + [Fact] public void WriteTyped_Should_Reject_Auto_Format_And_Create_No_File() { diff --git a/tests/ExcelReader.Tests/Native/NativeWriterHandleTests.cs b/tests/ExcelReader.Tests/Native/NativeWriterHandleTests.cs index ca327f11..15e4fc2b 100644 --- a/tests/ExcelReader.Tests/Native/NativeWriterHandleTests.cs +++ b/tests/ExcelReader.Tests/Native/NativeWriterHandleTests.cs @@ -80,6 +80,57 @@ public void OpenWriteHandle_Should_Round_Trip_A_Streamed_Row_Through_OpenFile(in } } + [Fact] + public void OpenWriteHandle_Should_Replace_The_Existing_File_Only_When_Closed_Successfully() + { + string directory = Path.Combine(Path.GetTempPath(), $"excelreader-writer-handle-{Guid.NewGuid():N}"); + Directory.CreateDirectory(directory); + string path = Path.Combine(directory, "out.csv"); + File.WriteAllText(path, "ORIGINAL"); + try + { + Assert.Equal(NativeStatus.Ok, OpenWriteHandle(path, NativeFormat.Csv, out NativeWriterHandle? handle)); + Assert.NotNull(handle); + handle.StartSheet("Dados"); + handle.StartRow(); + handle.WriteString("new"); + handle.EndRow(); + + Assert.Equal("ORIGINAL", File.ReadAllText(path)); + Assert.Equal(NativeStatus.Ok, WriteApi.CloseWriteHandle(handle)); + + Assert.Equal("new", File.ReadAllText(path).TrimEnd()); + Assert.Equal([path], Directory.GetFiles(directory)); + } + finally + { + Directory.Delete(directory, recursive: true); + } + } + + [Fact] + public void OpenWriteHandle_Should_Keep_The_Existing_File_When_Close_Fails() + { + string directory = Path.Combine(Path.GetTempPath(), $"excelreader-writer-handle-{Guid.NewGuid():N}"); + Directory.CreateDirectory(directory); + string path = Path.Combine(directory, "out.xlsx"); + File.WriteAllText(path, "ORIGINAL"); + try + { + Assert.Equal(NativeStatus.Ok, OpenWriteHandle(path, NativeFormat.Xlsx, out NativeWriterHandle? handle)); + Assert.NotNull(handle); + + Assert.Equal(NativeStatus.Error, WriteApi.CloseWriteHandle(handle)); + + Assert.Equal("ORIGINAL", File.ReadAllText(path)); + Assert.Equal([path], Directory.GetFiles(directory)); + } + finally + { + Directory.Delete(directory, recursive: true); + } + } + [Fact] public void OpenWriteHandle_Should_Reject_An_Empty_Path() { From b66224531f90157f0464ea1754d4522c2b0630ec Mon Sep 17 00:00:00 2001 From: GabrielMarquezMatte Date: Mon, 26 Oct 2026 13:17:11 -0300 Subject: [PATCH 02/20] fix(cli): keep cell positions, booleans, exact numbers and the old output 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,,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. --- src/ExcelReader.Cli/CliCommands.cs | 99 +++++---- src/ExcelReader.Core/ExcelReader.Core.csproj | 2 + tests/ExcelReader.Tests/Cli/CliTests.cs | 204 +++++++++++++++++++ 3 files changed, 266 insertions(+), 39 deletions(-) diff --git a/src/ExcelReader.Cli/CliCommands.cs b/src/ExcelReader.Cli/CliCommands.cs index 8f1833d8..12c1ed94 100644 --- a/src/ExcelReader.Cli/CliCommands.cs +++ b/src/ExcelReader.Cli/CliCommands.cs @@ -4,7 +4,9 @@ using ExcelReader.Core.Reader.Csv; using ExcelReader.Core.Reader.Schema; using ExcelReader.Core.Writer; +using ExcelReader.Core.Parser; using ExcelReader.Core.Writer.Csv; +using ExcelReader.Core.Writer.Internal; using ExcelReader.Core.Writer.Xls; using ExcelReader.Core.Writer.Xlsb; using ExcelReader.Core.Writer.Xlsx; @@ -38,41 +40,39 @@ internal static int Convert(string path, string? sheet, string? output, string? ThrowIfOutputIsADirectory(output); using IExcelWorkbook workbook = Open(path, sheet, out IExcelSheet selected, password, inputDelimiter); - bool leaveOpen = output is null; - Stream target = leaveOpen - ? stdout - : new FileStream(output!, FileMode.Create, FileAccess.Write, FileShare.None); - try + if (output is null) { - switch (resolvedFormat) - { - case "csv": - WriteCsv(selected, target, leaveOpen, delimiter, onProgress); - break; - case "xlsx": - WriteXlsx(selected, target, leaveOpen, onProgress); - break; - case "xlsb": - WriteXlsb(selected, target, leaveOpen, onProgress); - break; - case "xls": - WriteXls(selected, target, leaveOpen, onProgress); - break; - default: - throw new UnreachableException($"unresolved format '{resolvedFormat}'."); - } - } - finally - { - if (!leaveOpen) - { - target.Dispose(); - } + WriteFormat(resolvedFormat, selected, stdout, leaveOpen: true, delimiter, onProgress); + return 0; } + using StagedFile staged = new(output); + WriteFormat(resolvedFormat, selected, staged.Stream, leaveOpen: true, delimiter, onProgress); + staged.Commit(); return 0; }, stderr); } + private static void WriteFormat(string format, IExcelSheet selected, Stream target, bool leaveOpen, char delimiter, Action? onProgress) + { + switch (format) + { + case "csv": + WriteCsv(selected, target, leaveOpen, delimiter, onProgress); + return; + case "xlsx": + WriteXlsx(selected, target, leaveOpen, onProgress); + return; + case "xlsb": + WriteXlsb(selected, target, leaveOpen, onProgress); + return; + case "xls": + WriteXls(selected, target, leaveOpen, onProgress); + return; + default: + throw new UnreachableException($"unresolved format '{format}'."); + } + } + internal static string ResolveFormat(string? format, string? output) { if (format is not null) @@ -143,7 +143,7 @@ private static void WriteRows(TWorkbook workbook, IExce where TSheet : ISheetWriter where TRow : IRowWriter { - using TSheet sheetWriter = workbook.AddSheet(sheet.Name); + using TSheet sheetWriter = workbook.AddSheet(sheet.Name.Length == 0 ? "Sheet1" : sheet.Name); int rowCount = 0; using IExcelRowEnumerator rows = sheet.GetEnumerator(); @@ -151,23 +151,20 @@ private static void WriteRows(TWorkbook workbook, IExce { using TRow row = sheetWriter.StartRow(); Row current = rows.Current; + int nextColumn = 0; foreach (var cell in current.Cells) { + row.Skip(cell.ColumnIndex - nextColumn); + nextColumn = cell.ColumnIndex + 1; var cellValue = cell.Value; switch (cellValue.Type) { case CellType.Boolean: - row.Write(!cellValue.Value.IsEmpty && cellValue.Value[0] != (byte)'0'); + var boolValue = ExcelCellReaders.Bool(in cellValue, default, out bool flag) ? flag : throw new InvalidOperationException($"cell {cell.ColumnIndex} is a boolean but holds '{cellValue.GetString()}'."); + row.Write(boolValue); break; case CellType.Number: - if (cellValue.TryGetDouble(out double number)) - { - row.Write(number); - } - else - { - row.Write(cellValue.GetString()); - } + WriteNumber(row, in cellValue); break; case CellType.Date: var dateValue = cellValue.TryGetDateTime(sheet.IsDate1904, out var date) ? date : throw new InvalidOperationException($"cell {cell.ColumnIndex} is a date but TryGetDateTime failed to parse it."); @@ -189,6 +186,30 @@ private static void WriteRows(TWorkbook workbook, IExce workbook.End(); } + // Text-backed numbers keep their exact integer or decimal value; a double cannot tell apart + // integers above 2^53. + private static void WriteNumber(TRow row, in Cell cell) where TRow : IRowWriter + { + if (!cell.Value.IsEmpty && cell.TryParse(CultureInfo.InvariantCulture, out long integer)) + { + row.Write(integer); + return; + } + bool hasDouble = cell.TryGetDouble(out double number); + if (!cell.Value.IsEmpty && hasDouble && cell.TryParse(CultureInfo.InvariantCulture, out decimal exact) + && double.Parse(exact.ToString(CultureInfo.InvariantCulture), CultureInfo.InvariantCulture) == number) + { + row.Write(exact); + return; + } + if (hasDouble) + { + row.Write(number); + return; + } + row.Write(cell.GetString()); + } + internal static int Schema(string path, string? sheet, int headerRow, int sampleSize, TextWriter stdout, TextWriter stderr, string? password = null, char? inputDelimiter = null) { return Execute(() => diff --git a/src/ExcelReader.Core/ExcelReader.Core.csproj b/src/ExcelReader.Core/ExcelReader.Core.csproj index 82943fb7..074aa053 100644 --- a/src/ExcelReader.Core/ExcelReader.Core.csproj +++ b/src/ExcelReader.Core/ExcelReader.Core.csproj @@ -53,6 +53,8 @@ + + diff --git a/tests/ExcelReader.Tests/Cli/CliTests.cs b/tests/ExcelReader.Tests/Cli/CliTests.cs index 0643d294..daa61446 100644 --- a/tests/ExcelReader.Tests/Cli/CliTests.cs +++ b/tests/ExcelReader.Tests/Cli/CliTests.cs @@ -1,4 +1,5 @@ using ExcelReader.Cli; +using ExcelReader.Core.Parser; using ExcelReader.Core.Reader; using ExcelReader.Core.Writer.Xlsx; using ExcelReader.Tests.Crypto; @@ -356,6 +357,209 @@ public void Should_ReturnOne_When_TheFormatNameIsUnknown() Assert.Contains("pdf", error, StringComparison.Ordinal); } + private string TemporaryPath(string extension) + { + string path = Path.Combine(Path.GetTempPath(), $"excelreader-cli-{Guid.NewGuid():N}.{extension}"); + _temporaryFiles.Add(path); + return path; + } + + private string WriteXlsx(Action writeRow) + { + string path = TemporaryPath("xlsx"); + using XlsxWorkbookWriter workbook = XlsxWorkbookWriter.Create(File.Create(path)); + using XlsxSheetWriter sheet = workbook.AddSheet("Data"); + using (XlsxRowWriter row = sheet.StartRow()) + { + writeRow(row); + } + sheet.End(); + workbook.End(); + return path; + } + + private string WriteRawXlsx(string rows) + { + string path = TemporaryPath("xlsx"); + using MemoryStream built = WorkbookBuilder.Build(rows); + File.WriteAllBytes(path, built.ToArray()); + return path; + } + + private static string FirstCsvLine(string csv) + { + return csv.Split('\n')[0].TrimEnd('\r'); + } + + [Fact] + public void Should_KeepTheExistingOutput_When_ConversionFails() + { + string directory = Path.Combine(Path.GetTempPath(), $"excelreader-cli-{Guid.NewGuid():N}"); + Directory.CreateDirectory(directory); + try + { + string target = Path.Combine(directory, "out.xlsx"); + File.WriteAllText(target, "ORIGINAL"); + string source = WriteRawXlsx("""1maybe"""); + + (int code, _, string error) = Convert(source, output: target); + + Assert.Equal(1, code); + Assert.NotEmpty(error); + Assert.Equal("ORIGINAL", File.ReadAllText(target)); + Assert.Equal([target], Directory.GetFiles(directory)); + } + finally + { + Directory.Delete(directory, recursive: true); + } + } + + [Fact] + public void Should_ReplaceTheExistingOutput_When_ConversionSucceeds() + { + string target = TemporaryPath("csv"); + File.WriteAllText(target, "ORIGINAL"); + + (int code, _, string error) = Convert(WriteXlsx(row => row.Write("new")), output: target); + + Assert.Equal(0, code); + Assert.Empty(error); + Assert.Equal("new", FirstCsvLine(File.ReadAllText(target))); + } + + [Fact] + public void Should_KeepColumnGaps_When_ConvertingSparseRowsToCsv() + { + string source = WriteXlsx(row => + { + row.Skip(); + row.Write("B"); + row.Skip(); + row.Write("D"); + }); + + (int code, string output, string error) = Convert(source); + + Assert.Equal(0, code); + Assert.Empty(error); + Assert.Equal(",B,,D", FirstCsvLine(output)); + } + + [Theory] + [InlineData("xlsx")] + [InlineData("xlsb")] + [InlineData("xls")] + public void Should_KeepColumnGaps_When_ConvertingSparseRowsToASpreadsheet(string extension) + { + string source = WriteXlsx(row => + { + row.Write("A"); + row.Skip(); + row.Write("C"); + }); + string target = TemporaryPath(extension); + + (int code, _, string error) = Convert(source, output: target); + + Assert.Equal(0, code); + Assert.Empty(error); + using IExcelWorkbook written = Excel.Open(target); + using IExcelRowEnumerator rows = written.FirstSheet.GetEnumerator(); + Assert.True(rows.MoveNext()); + Assert.Equal("A", rows.Current[0].GetString()); + Assert.Equal(string.Empty, rows.Current[1].GetString()); + Assert.Equal("C", rows.Current[2].GetString()); + } + + [Fact] + public void Should_ConvertEveryBooleanSpelling_When_WritingCsv() + { + string source = WriteRawXlsx("""falsetrue01"""); + + (int code, string output, string error) = Convert(source); + + Assert.Equal(0, code); + Assert.Empty(error); + Assert.Equal("false,true,false,true", FirstCsvLine(output)); + } + + [Theory] + [InlineData("xlsx")] + [InlineData("xlsb")] + [InlineData("xls")] + public void Should_ConvertEveryBooleanSpelling_When_WritingASpreadsheet(string extension) + { + string source = WriteRawXlsx("""falsetrue01"""); + string target = TemporaryPath(extension); + + (int code, _, string error) = Convert(source, output: target); + + Assert.Equal(0, code); + Assert.Empty(error); + using IExcelWorkbook written = Excel.Open(target); + using IExcelRowEnumerator rows = written.FirstSheet.GetEnumerator(); + Assert.True(rows.MoveNext()); + bool[] expected = [false, true, false, true]; + for (int column = 0; column < expected.Length; column++) + { + Core.Reader.Cell cell = rows.Current[column]; + Assert.True(ExcelCellReaders.Bool(in cell, default, out bool value)); + Assert.Equal(expected[column], value); + } + } + + [Fact] + public void Should_KeepExactIntegers_When_ConvertingNumbersAbove2Pow53() + { + string source = WriteXlsx(row => + { + row.Write(9007199254740993L); + row.Write(12345678901234567890.5m); + row.Write(0.1); + row.Write(1e-30); + }); + string target = TemporaryPath("xlsx"); + + (int code, string output, string error) = Convert(source); + (int xlsxCode, _, _) = Convert(source, output: target); + + Assert.Equal(0, code); + Assert.Empty(error); + Assert.Equal("9007199254740993,12345678901234567890.5,0.1,1E-30", FirstCsvLine(output)); + Assert.Equal(0, xlsxCode); + using IExcelWorkbook written = Excel.Open(target); + using IExcelRowEnumerator rows = written.FirstSheet.GetEnumerator(); + Assert.True(rows.MoveNext()); + Assert.True(rows.Current[0].TryParse(null, out long integer)); + Assert.Equal(9007199254740993L, integer); + } + + [Theory] + [InlineData("xlsx")] + [InlineData("xlsb")] + [InlineData("xls")] + public void Should_ConvertCsvToASpreadsheet_When_TheSourceSheetHasNoName(string extension) + { + string source = TemporaryPath("csv"); + File.WriteAllText(source, "a,b\n1,2\n"); + string target = TemporaryPath(extension); + + (int code, _, string error) = Convert(source, output: target); + + Assert.Equal(0, code); + Assert.Empty(error); + using IExcelWorkbook written = Excel.Open(target); + Assert.Equal("Sheet1", written.FirstSheet.Name); + using IExcelRowEnumerator rows = written.FirstSheet.GetEnumerator(); + Assert.True(rows.MoveNext()); + Assert.Equal("a", rows.Current[0].GetString()); + Assert.Equal("b", rows.Current[1].GetString()); + Assert.True(rows.MoveNext()); + Assert.Equal("1", rows.Current[0].GetString()); + Assert.Equal("2", rows.Current[1].GetString()); + } + private static (int Code, string Out, string Err) Schema( string path, string? sheet = null, int headerRow = 1, int sampleSize = 100) { From b68e67ac243bb20870ab48422ceea5cea5f33a91 Mon Sep 17 00:00:00 2001 From: GabrielMarquezMatte Date: Mon, 26 Oct 2026 13:17:52 -0300 Subject: [PATCH 03/20] fix(reader): read XLSX metadata the way XML spells it 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 (' inside a quoted value (formatCode="[>1]yyyy-mm-dd") ended the tag early, and a commented-out or 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. --- .../Reader/Xlsx/TagSpanEnumerable.cs | 13 +- .../Reader/Xlsx/XlsxTables.cs | 14 +- .../Reader/Xlsx/XlsxWorkbook.Enumerator.cs | 52 +------ .../Reader/Xlsx/XlsxWorkbook.Loading.cs | 16 +- .../Reader/Xlsx/XlsxWorkbook.Styles.cs | 14 +- src/ExcelReader.Core/Reader/Xlsx/XlsxXml.cs | 146 ++++++++++++++++-- .../Reader/DialectRobustnessTests.cs | 59 ++++++- .../Reader/Xlsx/XlsxLexicalTests.cs | 139 +++++++++++++++++ 8 files changed, 363 insertions(+), 90 deletions(-) create mode 100644 tests/ExcelReader.Tests/Reader/Xlsx/XlsxLexicalTests.cs diff --git a/src/ExcelReader.Core/Reader/Xlsx/TagSpanEnumerable.cs b/src/ExcelReader.Core/Reader/Xlsx/TagSpanEnumerable.cs index 6ee09b8b..dc0791fe 100644 --- a/src/ExcelReader.Core/Reader/Xlsx/TagSpanEnumerable.cs +++ b/src/ExcelReader.Core/Reader/Xlsx/TagSpanEnumerable.cs @@ -29,7 +29,18 @@ public bool MoveNext() { return false; } - int end = _remaining[start..].IndexOf((byte)'>'); + int comment = _remaining[..start].IndexOf(""u8); + if (commentEnd < 0) + { + return false; + } + _remaining = _remaining[(comment + 4 + commentEnd + 3)..]; + continue; + } + int end = XlsxXml.TagEnd(_remaining[start..]); if (end < 0) { return false; diff --git a/src/ExcelReader.Core/Reader/Xlsx/XlsxTables.cs b/src/ExcelReader.Core/Reader/Xlsx/XlsxTables.cs index 0e906ad5..c82b6fbd 100644 --- a/src/ExcelReader.Core/Reader/Xlsx/XlsxTables.cs +++ b/src/ExcelReader.Core/Reader/Xlsx/XlsxTables.cs @@ -13,14 +13,14 @@ internal static TablePart ParseTable(int sheetIndex, ReadOnlySpan xml) { throw new InvalidDataException("A table part has no element."); } - string name = XlsxXml.DecodeToString(XlsxXml.Attr(table, " displayName="u8)); - if (!TablePart.TryParseRange(XlsxXml.Attr(table, " ref="u8), out int firstRow, out int firstColumn, out int lastRow, out int lastColumn)) + string name = XlsxXml.DecodeToString(XlsxXml.Attr(table, "displayName"u8)); + if (!TablePart.TryParseRange(XlsxXml.Attr(table, "ref"u8), out int firstRow, out int firstColumn, out int lastRow, out int lastColumn)) { throw new InvalidDataException($"Table '{name}' has an unreadable ref."); } - int id = XlsxXml.ParseIntOr(XlsxXml.Attr(table, " id="u8), 0); - int headerRowCount = ParseRowCount(table, " headerRowCount="u8, 1, name); - int totalsRowCount = ParseRowCount(table, " totalsRowCount="u8, 0, name); + int id = XlsxXml.ParseIntOr(XlsxXml.Attr(table, "id"u8), 0); + int headerRowCount = ParseRowCount(table, "headerRowCount"u8, 1, name); + int totalsRowCount = ParseRowCount(table, "totalsRowCount"u8, 0, name); return TablePart.Create(sheetIndex, id, name, firstRow, firstColumn, lastRow, lastColumn, headerRowCount, totalsRowCount, ColumnNames(xml, prefix), StyleName(xml, prefix)); } @@ -44,14 +44,14 @@ private static string[] ColumnNames(ReadOnlySpan xml, ReadOnlySpan p List names = []; foreach (ReadOnlySpan column in new TagSpanEnumerable(xml, XlsxXml.Token("<"u8, prefix, "tableColumn"u8))) { - names.Add(XlsxXml.DecodeXStringToString(XlsxXml.Attr(column, " name="u8))); + names.Add(XlsxXml.DecodeXStringToString(XlsxXml.Attr(column, "name"u8))); } return [.. names]; } private static string? StyleName(ReadOnlySpan xml, ReadOnlySpan prefix) { - ReadOnlySpan style = XlsxXml.Attr(FirstTag(xml, XlsxXml.Token("<"u8, prefix, "tableStyleInfo"u8)), " name="u8); + ReadOnlySpan style = XlsxXml.Attr(FirstTag(xml, XlsxXml.Token("<"u8, prefix, "tableStyleInfo"u8)), "name"u8); if (style.IsEmpty) { return null; diff --git a/src/ExcelReader.Core/Reader/Xlsx/XlsxWorkbook.Enumerator.cs b/src/ExcelReader.Core/Reader/Xlsx/XlsxWorkbook.Enumerator.cs index 7d00d310..45cb3544 100644 --- a/src/ExcelReader.Core/Reader/Xlsx/XlsxWorkbook.Enumerator.cs +++ b/src/ExcelReader.Core/Reader/Xlsx/XlsxWorkbook.Enumerator.cs @@ -824,7 +824,7 @@ private bool MissingRowOpenTag() [MethodImpl(MethodImplOptions.NoInlining)] private void AdvanceRowIndex(ReadOnlySpan rowTag) { - int rowNumber = XlsxXml.ParseIntOr(XlsxXml.Attr(rowTag, " r="u8), 0); + int rowNumber = XlsxXml.ParseIntOr(XlsxXml.Attr(rowTag, "r"u8), 0); if (rowNumber > 0) { _rowIndex = rowNumber - 1; @@ -931,57 +931,19 @@ private static void ScanCellAttributes( out ReadOnlySpan tVal) { rRef = sVal = tVal = default; - int i = 2; - while (i < open.Length && open[i] is not ((byte)'>' or (byte)'/')) + int pos = XlsxXml.ElementNameEnd(open); + while (XlsxXml.NextAttribute(open, ref pos, out ReadOnlySpan name, out ReadOnlySpan value)) { - if (!IsXmlSpace(open[i])) + if (name.Length != 1) { - i++; continue; } - i++; - - int nameStart = i; - while (i < open.Length && open[i] is not ((byte)'=' or (byte)'>' or (byte)' ')) - { - i++; - } - if (i >= open.Length || open[i] != (byte)'=') - { - continue; - } - int nameLen = i - nameStart; - - i++; - if (i >= open.Length || open[i] is not ((byte)'"' or (byte)'\'')) - { - continue; - } - byte quote = open[i++]; - int valueStart = i; - while (i < open.Length && open[i] != quote) - { - i++; - } - ReadOnlySpan value = open[valueStart..i]; - i++; - - if (nameLen != 1) - { - continue; - } - if (open[nameStart] == (byte)'r') { rRef = value; } - else if (open[nameStart] == (byte)'s') { sVal = value; } - else if (open[nameStart] == (byte)'t') { tVal = value; } + if (name[0] == (byte)'r') { rRef = value; } + else if (name[0] == (byte)'s') { sVal = value; } + else if (name[0] == (byte)'t') { tVal = value; } } } - [MethodImpl(MethodImplOptions.AggressiveInlining)] - private static bool IsXmlSpace(byte b) - { - return b is (byte)' ' or (byte)'\t' or (byte)'\r' or (byte)'\n'; - } - private enum Kind { Number, Shared, Inline, Bool, Error, Formula, IsoDate } [MethodImpl(MethodImplOptions.AggressiveInlining)] diff --git a/src/ExcelReader.Core/Reader/Xlsx/XlsxWorkbook.Loading.cs b/src/ExcelReader.Core/Reader/Xlsx/XlsxWorkbook.Loading.cs index f3ab2ffe..0856a76c 100644 --- a/src/ExcelReader.Core/Reader/Xlsx/XlsxWorkbook.Loading.cs +++ b/src/ExcelReader.Core/Reader/Xlsx/XlsxWorkbook.Loading.cs @@ -16,20 +16,20 @@ private static (string Name, string Path, ExcelSheetVisibility Visibility)[] Par Dictionary rels = XlsxXml.ParseRelationships(relsBytes); var sheets = new List<(string, string, ExcelSheetVisibility)>(); ReadOnlySpan prefix = XlsxXml.DetectElementPrefix(wbBytes); - ReadOnlySpan sheetTag = " sheetTag = " src) { return false; } - var attr = XlsxXml.Attr(src.Slice(pos, end - pos + 1), " date1904="u8); + var attr = XlsxXml.Attr(src.Slice(pos, end - pos + 1), "date1904"u8); return attr.SequenceEqual("1"u8) || attr.SequenceEqual("true"u8); } @@ -173,7 +173,7 @@ private int[] ParseSharedBody(BufferedStreamCursor io, Stream? stream, int partL int sstEnd = FindSeqGrowing(io, stream, GtToken); if (sstEnd > sstPos) { - uniqueCount = XlsxXml.ParseIntOr(XlsxXml.Attr(io.Buf.AsSpan(sstPos, sstEnd - sstPos), " uniqueCount="u8), 0); + uniqueCount = XlsxXml.ParseIntOr(XlsxXml.Attr(io.Buf.AsSpan(sstPos, sstEnd - sstPos), "uniqueCount"u8), 0); } } LimitChecks.ThrowIfSharedStringCountImplausible(uniqueCount, partLength); @@ -225,7 +225,7 @@ private async ValueTask ParseSharedBodyAsync(BufferedStreamCursor io, Str int sstEnd = await FindSeqGrowingAsync(io, stream, GtToken, ct).ConfigureAwait(false); if (sstEnd > sstPos) { - uniqueCount = XlsxXml.ParseIntOr(XlsxXml.Attr(io.Buf.AsSpan(sstPos, sstEnd - sstPos), " uniqueCount="u8), 0); + uniqueCount = XlsxXml.ParseIntOr(XlsxXml.Attr(io.Buf.AsSpan(sstPos, sstEnd - sstPos), "uniqueCount"u8), 0); } } LimitChecks.ThrowIfSharedStringCountImplausible(uniqueCount, partLength); diff --git a/src/ExcelReader.Core/Reader/Xlsx/XlsxWorkbook.Styles.cs b/src/ExcelReader.Core/Reader/Xlsx/XlsxWorkbook.Styles.cs index 832a217d..1ec147f3 100644 --- a/src/ExcelReader.Core/Reader/Xlsx/XlsxWorkbook.Styles.cs +++ b/src/ExcelReader.Core/Reader/Xlsx/XlsxWorkbook.Styles.cs @@ -12,23 +12,23 @@ private static bool[] ParseStyleDateFlags(ReadOnlySpan src) } ReadOnlySpan prefix = XlsxXml.DetectElementPrefix(src); - ReadOnlySpan numFmtTag = " cellXfsClose = ""u8, xfTag = " numFmtTag = " cellXfsClose = ""u8, xfTag = ""u8); - xfTag = XlsxXml.Token("<"u8, prefix, "xf "u8); + xfTag = XlsxXml.Token("<"u8, prefix, "xf"u8); } Dictionary custom = new(capacity: 16); foreach (var tag in Tags(src, numFmtTag)) { - int id = XlsxXml.ParseIntOr(XlsxXml.Attr(tag, " numFmtId="u8), -1); + int id = XlsxXml.ParseIntOr(XlsxXml.Attr(tag, "numFmtId"u8), -1); if (id >= 0) { - custom[id] = NumberFormat.LooksLikeDate(XlsxXml.DecodeToString(XlsxXml.Attr(tag, " formatCode="u8))); + custom[id] = NumberFormat.LooksLikeDate(XlsxXml.DecodeToString(XlsxXml.Attr(tag, "formatCode"u8))); } } @@ -50,7 +50,7 @@ private static bool[] ParseStyleDateFlags(ReadOnlySpan src) List flags = new(capacity: 16); foreach (var xf in Tags(src.Slice(open + 1, end - open - 1), xfTag)) { - int numFmtId = XlsxXml.ParseIntOr(XlsxXml.Attr(xf, " numFmtId="u8), 0); + int numFmtId = XlsxXml.ParseIntOr(XlsxXml.Attr(xf, "numFmtId"u8), 0); flags.Add(WorkbookLookups.ResolveDateFlag(custom, numFmtId)); } return [.. flags]; diff --git a/src/ExcelReader.Core/Reader/Xlsx/XlsxXml.cs b/src/ExcelReader.Core/Reader/Xlsx/XlsxXml.cs index 4b89e551..66ee7d99 100644 --- a/src/ExcelReader.Core/Reader/Xlsx/XlsxXml.cs +++ b/src/ExcelReader.Core/Reader/Xlsx/XlsxXml.cs @@ -9,6 +9,8 @@ namespace ExcelReader.Core.Reader.Xlsx internal static class XlsxXml { private static readonly SearchValues MarkupOrEscape = SearchValues.Create("<&_\r"u8); + private static readonly SearchValues NameTerminators = SearchValues.Create(" \t\r\n>/"u8); + private static readonly SearchValues TagEndOrQuote = SearchValues.Create(">\"'"u8); internal static int ParseIntOr(ReadOnlySpan source, int fallback) { @@ -17,24 +19,108 @@ internal static int ParseIntOr(ReadOnlySpan source, int fallback) public static ReadOnlySpan Attr(ReadOnlySpan openTag, ReadOnlySpan name) { - int i = openTag.IndexOf(name); - if (i < 0) + int pos = ElementNameEnd(openTag); + while (NextAttribute(openTag, ref pos, out ReadOnlySpan attributeName, out ReadOnlySpan value)) { - return default; + if (attributeName.SequenceEqual(name)) + { + return value; + } } - int start = i + name.Length; - if (start >= openTag.Length) + return default; + } + + internal static int ElementNameEnd(ReadOnlySpan openTag) + { + if (openTag.IsEmpty || openTag[0] != (byte)'<') { - return default; + return 0; } - byte quote = openTag[start]; - if (quote is not ((byte)'"' or (byte)'\'')) + int end = openTag.IndexOfAny(NameTerminators); + return end < 0 ? openTag.Length : end; + } + + // Reads the attribute at or after pos, allowing XML whitespace around '='. Skips anything that is + // not name="value" or name='value', and stops at the tag's closing '>' or '/>'. + internal static bool NextAttribute(ReadOnlySpan tag, scoped ref int pos, out ReadOnlySpan name, out ReadOnlySpan value) + { + int i = pos; + while (true) { - return default; + i = SkipSpace(tag, i); + if (i >= tag.Length || tag[i] is (byte)'>' or (byte)'/') + { + break; + } + int nameStart = i; + while (i < tag.Length && !IsXmlSpace(tag[i]) && tag[i] is not ((byte)'=' or (byte)'>' or (byte)'/')) + { + i++; + } + int nameEnd = i; + i = SkipSpace(tag, i); + if (i >= tag.Length || tag[i] != (byte)'=') + { + continue; + } + i = SkipSpace(tag, i + 1); + if (i >= tag.Length || tag[i] is not ((byte)'"' or (byte)'\'')) + { + continue; + } + int close = tag[(i + 1)..].IndexOf(tag[i]); + if (close < 0) + { + break; + } + name = tag[nameStart..nameEnd]; + value = tag.Slice(i + 1, close); + pos = i + close + 2; + return true; + } + pos = tag.Length; + name = value = default; + return false; + } + + private static int SkipSpace(ReadOnlySpan tag, int i) + { + while (i < tag.Length && IsXmlSpace(tag[i])) + { + i++; + } + return i; + } + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + internal static bool IsXmlSpace(byte b) + { + return b is (byte)' ' or (byte)'\t' or (byte)'\r' or (byte)'\n'; + } + + // The index of the '>' closing the tag that starts tag, skipping any '>' inside a quoted attribute value. + internal static int TagEnd(ReadOnlySpan tag) + { + int i = 0; + while (true) + { + int hit = tag[i..].IndexOfAny(TagEndOrQuote); + if (hit < 0) + { + return -1; + } + i += hit; + if (tag[i] == (byte)'>') + { + return i; + } + int close = tag[(i + 1)..].IndexOf(tag[i]); + if (close < 0) + { + return -1; + } + i += close + 2; } - start++; - int end = openTag[start..].IndexOf(quote); - return end < 0 ? default : openTag.Slice(start, end); } [MethodImpl(MethodImplOptions.AggressiveInlining)] @@ -414,6 +500,34 @@ private static string DecodeToString(ReadOnlySpan src, bool xstring) } internal static string NormalizePart(ReadOnlySpan target) + { + string part = PackagePath(target); + if (!part.Contains("./", StringComparison.Ordinal)) + { + return part; + } + List segments = []; + foreach (string segment in part.Split('/')) + { + if (string.Equals(segment, ".", StringComparison.Ordinal)) + { + continue; + } + if (!string.Equals(segment, "..", StringComparison.Ordinal)) + { + segments.Add(segment); + continue; + } + if (segments.Count == 0) + { + throw new InvalidDataException($"The part target '{target}' climbs above the package root."); + } + segments.RemoveAt(segments.Count - 1); + } + return string.Join('/', segments); + } + + private static string PackagePath(ReadOnlySpan target) { if (target.Length > 0 && target[0] == '/') { @@ -435,10 +549,10 @@ internal static Dictionary ParseRelationships(ReadOnlySpan } foreach (ReadOnlySpan tag in new TagSpanEnumerable(relsBytes, " 0) { - rels[id] = DecodeToString(Attr(tag, " Target="u8)); + rels[id] = DecodeToString(Attr(tag, "Target"u8)); } } return rels; @@ -449,9 +563,9 @@ internal static List RelationshipTargets(ReadOnlySpan relsBytes, R List targets = []; foreach (ReadOnlySpan tag in new TagSpanEnumerable(relsBytes, " tag = " name=\"S1\" r:id=\"rId1\""u8; - Assert.True(XlsxXml.Attr(tag, " name="u8).SequenceEqual("S1"u8)); - Assert.True(XlsxXml.Attr(tag, " r:id="u8).SequenceEqual("rId1"u8)); + Assert.True(XlsxXml.Attr(tag, "name"u8).SequenceEqual("S1"u8)); + Assert.True(XlsxXml.Attr(tag, "r:id"u8).SequenceEqual("rId1"u8)); } [Fact] public void AttrReadsSingleQuotedValue() { ReadOnlySpan tag = " name='S1' r:id='rId1'"u8; - Assert.True(XlsxXml.Attr(tag, " name="u8).SequenceEqual("S1"u8)); - Assert.True(XlsxXml.Attr(tag, " r:id="u8).SequenceEqual("rId1"u8)); + Assert.True(XlsxXml.Attr(tag, "name"u8).SequenceEqual("S1"u8)); + Assert.True(XlsxXml.Attr(tag, "r:id"u8).SequenceEqual("rId1"u8)); } [Fact] public void AttrReturnsEmptyForMissingOrUnquotedAttribute() { - Assert.True(XlsxXml.Attr(" name=\"S1\""u8, " id="u8).IsEmpty); - Assert.True(XlsxXml.Attr(" name=bad"u8, " name="u8).IsEmpty); + Assert.True(XlsxXml.Attr(" name=\"S1\""u8, "id"u8).IsEmpty); + Assert.True(XlsxXml.Attr(" name=bad"u8, "name"u8).IsEmpty); + } + + [Fact] + public void AttrAllowsWhitespaceAroundTheEqualsSign() + { + ReadOnlySpan tag = ""u8; + Assert.True(XlsxXml.Attr(tag, "name"u8).SequenceEqual("S1"u8)); + Assert.True(XlsxXml.Attr(tag, "r:id"u8).SequenceEqual("rId1"u8)); + Assert.True(XlsxXml.Attr(tag, "state"u8).SequenceEqual("hidden"u8)); + } + + [Fact] + public void AttrMatchesWholeNamesOnly() + { + ReadOnlySpan tag = ""u8; + Assert.True(XlsxXml.Attr(tag, "numFmtId"u8).SequenceEqual("14"u8)); + Assert.True(XlsxXml.Attr(tag, "id"u8).IsEmpty); + } + + [Fact] + public void TagsSkipCommentsAndQuotedGreaterThanSigns() + { + List tags = []; + foreach (ReadOnlySpan tag in new TagSpanEnumerable( + "1]0\" numFmtId=\"2\"/>"u8, "1]0\" numFmtId=\"2\"/>"], tags); + } + + [Theory] + [InlineData("worksheets/sheet1.xml")] + [InlineData("./worksheets/sheet1.xml")] + [InlineData("worksheets/./sheet1.xml")] + [InlineData("../xl/worksheets/sheet1.xml")] + [InlineData("/xl/worksheets/sheet1.xml")] + [InlineData("charts/../worksheets/sheet1.xml")] + public void NormalizePartResolvesDotSegments(string target) + { + Assert.Equal("xl/worksheets/sheet1.xml", XlsxXml.NormalizePart(target)); + } + + [Fact] + public void NormalizePartRejectsATargetAboveThePackageRoot() + { + Assert.Throws(() => XlsxXml.NormalizePart("../../sheet1.xml")); } diff --git a/tests/ExcelReader.Tests/Reader/Xlsx/XlsxLexicalTests.cs b/tests/ExcelReader.Tests/Reader/Xlsx/XlsxLexicalTests.cs new file mode 100644 index 00000000..72c53e26 --- /dev/null +++ b/tests/ExcelReader.Tests/Reader/Xlsx/XlsxLexicalTests.cs @@ -0,0 +1,139 @@ +using ExcelReader.Core.Reader; +using ExcelReader.Core.Reader.Xlsx; + +namespace ExcelReader.Tests.Reader.Xlsx +{ + public sealed class XlsxLexicalTests + { + private const string Main = "http://schemas.openxmlformats.org/spreadsheetml/2006/main"; + private const string Rel = "http://schemas.openxmlformats.org/officeDocument/2006/relationships"; + + private static string DateStyles(string xf, string numFmts = "") + { + return $"""{numFmts}{xf}"""; + } + + private static byte[] Build(string rows, string? styles = null) + { + using MemoryStream built = WorkbookBuilder.Build(rows, styles: styles); + return built.ToArray(); + } + + private static string Line(Row row) + { + List cells = []; + for (int column = 0; column < row.ColumnCount; column++) + { + Cell cell = row[column]; + cells.Add($"{cell.Type}:{cell.GetString()}"); + } + return string.Join("|", cells); + } + + // Reads the first row from memory, a non-seekable stream, and asynchronously, which must all agree. + private static async Task FirstRow(byte[] bytes) + { + string fromMemory; + using (IExcelWorkbook memory = Excel.FromXlsx(bytes)) + using (IExcelRowEnumerator rows = memory.FirstSheet.GetEnumerator()) + { + Assert.True(rows.MoveNext()); + fromMemory = Line(rows.Current); + } + + string fromStream; + using (IExcelWorkbook stream = Excel.FromXlsx(new NonSeekableStream(bytes), leaveOpen: false)) + using (IExcelRowEnumerator rows = stream.FirstSheet.GetEnumerator()) + { + Assert.True(rows.MoveNext()); + fromStream = Line(rows.Current); + } + + using IExcelWorkbook asyncReader = Excel.FromXlsx(new MemoryStream(bytes, writable: false), leaveOpen: false); + await using IExcelRowEnumerator asyncRows = asyncReader.FirstSheet.GetAsyncEnumerator(TestContext.Current.CancellationToken); + Assert.True(await asyncRows.MoveNextAsync()); + Assert.Equal(fromMemory, fromStream); + Assert.Equal(fromMemory, Line(asyncRows.Current)); + return fromMemory; + } + + [Theory] + [InlineData(" ", " ")] + [InlineData("\t", "\t")] + [InlineData("\r\n", "\n")] + [InlineData("", " ")] + public async Task Should_KeepCellCoordinates_When_AttributesHaveWhitespaceAroundEquals(string before, string after) + { + byte[] bytes = Build($"""AC"""); + + Assert.Equal("ExcelString:A|Empty:|ExcelString:C", await FirstRow(bytes)); + } + + [Theory] + [InlineData("")] + [InlineData("")] + [InlineData("")] + [InlineData("")] + public async Task Should_DetectDateStyles_When_TheXfTagUsesAnyWhitespace(string xf) + { + byte[] bytes = Build("""45000""", DateStyles(xf)); + + Assert.StartsWith("Date:", await FirstRow(bytes), StringComparison.Ordinal); + } + + [Fact] + public async Task Should_DetectADateFormat_When_ItsFormatCodeHoldsAGreaterThanSign() + { + string numFmts = """"""; + byte[] bytes = Build("""45000""", DateStyles("""""", numFmts)); + + Assert.StartsWith("Date:", await FirstRow(bytes), StringComparison.Ordinal); + } + + [Fact] + public async Task Should_IgnoreACommentedOutNumberFormat() + { + string numFmts = """"""; + byte[] bytes = Build("""45000""", DateStyles("""""", numFmts)); + + Assert.StartsWith("Number:", await FirstRow(bytes), StringComparison.Ordinal); + } + + [Fact] + public void Should_IgnoreCommentedOutSheetsAndRelationships() + { + using MemoryStream built = WorkbookBuilder.Build("""1"""); + byte[] bytes = Rewrite(built.ToArray(), "xl/workbook.xml", + $""""""); + + using XlsxWorkbook reader = Excel.FromXlsx(bytes); + Assert.Equal(1, reader.SheetCount); + Assert.Equal("S1", reader.FirstSheet.Name); + } + + [Theory] + [InlineData("./worksheets/sheet1.xml")] + [InlineData("../xl/worksheets/sheet1.xml")] + public async Task Should_ReadTheSheet_When_ItsRelationshipTargetHasDotSegments(string target) + { + using MemoryStream built = WorkbookBuilder.Build("""1"""); + byte[] bytes = Rewrite(built.ToArray(), "xl/_rels/workbook.xml.rels", + $""""""); + + Assert.Equal("Number:1", await FirstRow(bytes)); + } + + private static byte[] Rewrite(byte[] zip, string entry, string content) + { + using MemoryStream buffer = new(); + buffer.Write(zip); + using (System.IO.Compression.ZipArchive archive = new(buffer, System.IO.Compression.ZipArchiveMode.Update, leaveOpen: true)) + { + archive.GetEntry(entry)!.Delete(); + using StreamWriter writer = new(archive.CreateEntry(entry).Open()); + writer.Write(content); + } + return buffer.ToArray(); + } + } +} From fa24c9d6a9c37571ab22559df2bd058cb9e88fa4 Mon Sep 17 00:00:00 2001 From: GabrielMarquezMatte Date: Mon, 26 Oct 2026 13:17:52 -0300 Subject: [PATCH 04/20] fix(parser): parse enums exactly, unsigned and aliased 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. --- .../Parser/Internal/ColumnParserFactory.cs | 47 +++++-- .../Parser/EnumParsingTests.cs | 116 ++++++++++++++++++ 2 files changed, 152 insertions(+), 11 deletions(-) create mode 100644 tests/ExcelReader.Tests/Parser/EnumParsingTests.cs diff --git a/src/ExcelReader.Core/Parser/Internal/ColumnParserFactory.cs b/src/ExcelReader.Core/Parser/Internal/ColumnParserFactory.cs index 8a5d6ec1..b683ef5d 100644 --- a/src/ExcelReader.Core/Parser/Internal/ColumnParserFactory.cs +++ b/src/ExcelReader.Core/Parser/Internal/ColumnParserFactory.cs @@ -463,42 +463,67 @@ private static class EnumCache { private static readonly FrozenDictionary _nameMap = BuildNameMap(); private static readonly FrozenDictionary.AlternateLookup> _alternateLookup = _nameMap.GetAlternateLookup>(); - private static readonly FrozenDictionary _valueMap = BuildValueMap(); + private static readonly FrozenDictionary _valueMap = BuildValueMap(); private static FrozenDictionary BuildNameMap() { Dictionary map = new(StringComparer.OrdinalIgnoreCase); - foreach (TEnum value in Enum.GetValues()) + foreach (string name in Enum.GetNames()) { - string name = value.ToString(); - long numericValue = Convert.ToInt64(value, CultureInfo.InvariantCulture); + TEnum value = Enum.Parse(name); map[name] = value; - map[numericValue.ToString(CultureInfo.InvariantCulture)] = value; + map[value.ToString("D")] = value; } return map.ToFrozenDictionary(StringComparer.OrdinalIgnoreCase); } - private static FrozenDictionary BuildValueMap() + + private static FrozenDictionary BuildValueMap() { - Dictionary map = []; + Dictionary map = []; foreach (TEnum value in Enum.GetValues()) { - long numericValue = Convert.ToInt64(value, CultureInfo.InvariantCulture); - map[numericValue] = value; + map[Numeric(value)] = value; } return map.ToFrozenDictionary(); } + // Int128 holds every value of both a signed and an unsigned underlying type exactly. + private static Int128 Numeric(TEnum value) + { + if (Type.GetTypeCode(Enum.GetUnderlyingType(typeof(TEnum))) is TypeCode.Byte or TypeCode.UInt16 or TypeCode.UInt32 or TypeCode.UInt64) + { + return Convert.ToUInt64(value, CultureInfo.InvariantCulture); + } + return Convert.ToInt64(value, CultureInfo.InvariantCulture); + } + + // Integer text is parsed exactly; a double cannot tell apart integers above 2^53. + private static bool TryGetIntegral(in Cell cell, double number, out Int128 integral) + { + if (!cell.Value.IsEmpty && Int128.TryParse(cell.Value, NumberStyles.AllowLeadingSign, CultureInfo.InvariantCulture, out integral)) + { + return true; + } + if (number == Math.Truncate(number) && number is >= -9223372036854775808.0 and < 18446744073709551616.0) + { + integral = (Int128)number; + return true; + } + integral = default; + return false; + } + [SkipLocalsInit] public static bool TryParse(in Cell cell, out TEnum value) { if (cell.Type is CellType.Number or CellType.Date && cell.TryGetDouble(out double d)) { - if (d != Math.Truncate(d) || d < long.MinValue || d > long.MaxValue) + if (!TryGetIntegral(in cell, d, out Int128 integral)) { value = default; return false; } - return _valueMap.TryGetValue((long)d, out value); + return _valueMap.TryGetValue(integral, out value); } Span stack = stackalloc char[Utf8Text.StackChars]; ReadOnlySpan chars = Utf8Text.Decode(cell.Value, stack, out char[]? rented); diff --git a/tests/ExcelReader.Tests/Parser/EnumParsingTests.cs b/tests/ExcelReader.Tests/Parser/EnumParsingTests.cs new file mode 100644 index 00000000..f46a6157 --- /dev/null +++ b/tests/ExcelReader.Tests/Parser/EnumParsingTests.cs @@ -0,0 +1,116 @@ +using System.Diagnostics.CodeAnalysis; +using System.Text; +using ExcelReader.Core.Parser; +using ExcelReader.Core.Reader; + +namespace ExcelReader.Tests.Parser +{ + public sealed class EnumParsingTests + { + private enum Adjacent : long + { + Low = 9007199254740992, + High = 9007199254740993, + } + + private enum Unsigned : ulong + { + Low = 0, + High = ulong.MaxValue, + } + + [SuppressMessage("Design", "CA1069:Enums values should not be duplicated", Justification = "The alias is what the test covers.")] + private enum Aliased + { + First = 1, + Alias = 1, + } + + private sealed class AliasRow + { + public Aliased Value { get; set; } + } + + private static bool TryParse(CellType type, string text, out TEnum value) + where TEnum : struct, Enum + { + Cell cell = new(type, Encoding.UTF8.GetBytes(text)); + return ExcelCellReaders.Enum(in cell, default, out value); + } + + [Theory] + [InlineData("9007199254740992", 9007199254740992L)] + [InlineData("9007199254740993", 9007199254740993L)] + public void Should_PickTheExactMember_When_AdjacentValuesShareADouble(string text, long expected) + { + Assert.True(TryParse(CellType.Number, text, out Adjacent number)); + Assert.Equal((Adjacent)expected, number); + Assert.True(TryParse(CellType.ExcelString, text, out Adjacent textual)); + Assert.Equal((Adjacent)expected, textual); + } + + [Theory] + [InlineData("9007199254740993.5")] + [InlineData("1E300")] + [InlineData("NaN")] + [InlineData("-1")] + [InlineData("18446744073709551616")] + public void Should_Reject_When_TheNumberIsNotADeclaredIntegralValue(string text) + { + Assert.False(TryParse(CellType.Number, text, out Unsigned _)); + } + + [Theory] + [InlineData("Low", 0UL)] + [InlineData("high", ulong.MaxValue)] + [InlineData("0", 0UL)] + [InlineData("18446744073709551615", ulong.MaxValue)] + public void Should_ParseAnUnsignedEnum_When_AMemberExceedsInt64(string text, ulong expected) + { + Assert.True(TryParse(CellType.ExcelString, text, out Unsigned textual)); + Assert.Equal((Unsigned)expected, textual); + if (char.IsDigit(text[0])) + { + Assert.True(TryParse(CellType.Number, text, out Unsigned number)); + Assert.Equal((Unsigned)expected, number); + } + } + + [Fact] + public void Should_ReadABinaryNumberAsItsStoredDouble() + { + Cell cell = new(CellType.Number, default, 9007199254740992.0, hasNumber: true, styleIndex: 0); + + Assert.True(ExcelCellReaders.Enum(in cell, default, out Adjacent value)); + Assert.Equal(Adjacent.Low, value); + } + + [Theory] + [InlineData("First")] + [InlineData("alias")] + [InlineData("1")] + public void Should_ParseEveryAliasName(string text) + { + Assert.True(TryParse(CellType.ExcelString, text, out Aliased value)); + Assert.Equal(Aliased.First, value); + } + + [Fact] + public void Should_RejectAnUndefinedValue() + { + Assert.False(TryParse(CellType.ExcelString, "Second", out Aliased _)); + Assert.False(TryParse(CellType.Number, "2", out Aliased _)); + } + + [Fact] + public async Task Should_ParseAnAliasName_When_ReadingThroughAttributes() + { + await using var ms = await TypedWorkbook.BuildAsync(["Value"], ["Alias"], ["FIRST"]); + await using var reader = await Excel.FromXlsxAsync(ms, ct: TestContext.Current.CancellationToken); + + AliasRow[] rows = ExcelParser.FromAttributes().Parse(reader.FirstSheet).ToArray(); + + Assert.Equal([Aliased.First, Aliased.First], rows.Select(static row => row.Value)); + } + } +} From fee1b5c2c92f228b03eb99cad09a7e55cb08a77d Mon Sep 17 00:00:00 2001 From: GabrielMarquezMatte Date: Mon, 26 Oct 2026 13:17:52 -0300 Subject: [PATCH 05/20] fix(generator): give every model its own source file name 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. --- .../ExcelRowMapGenerator.cs | 3 +- .../Generator/GeneratorTests.cs | 39 +++++++++++++++++++ 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/src/ExcelReader.Generator/ExcelRowMapGenerator.cs b/src/ExcelReader.Generator/ExcelRowMapGenerator.cs index 6869f785..6804bad7 100644 --- a/src/ExcelReader.Generator/ExcelRowMapGenerator.cs +++ b/src/ExcelReader.Generator/ExcelRowMapGenerator.cs @@ -212,8 +212,7 @@ private static GeneratedResult Analyze(INamedTypeSymbol symbol) string source = GenerateSource(symbol, plans); string hintName = symbol.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat) .Replace("global::", "") - .Replace("@", "") - .Replace('.', '_'); + .Replace("@", ""); return new GeneratedResult(hintName, source, new EquatableArray([.. diagnostics])); } diff --git a/tests/ExcelReader.Tests/Generator/GeneratorTests.cs b/tests/ExcelReader.Tests/Generator/GeneratorTests.cs index 73a07cff..958e27bc 100644 --- a/tests/ExcelReader.Tests/Generator/GeneratorTests.cs +++ b/tests/ExcelReader.Tests/Generator/GeneratorTests.cs @@ -126,6 +126,45 @@ public partial class Model Assert.True(emit.Success, string.Join(Environment.NewLine, emit.Diagnostics.Select(static d => d.ToString()))); } + [Fact] + public void TypesWhoseNamesFlattenAlikeGetDistinctSourceFiles() + { + const string source = """ + using ExcelReader.Core.Parser; + + namespace A + { + [ExcelSerializable] + public partial class B_C + { + public int Value { get; set; } + } + + public partial class B + { + [ExcelSerializable] + public partial class C + { + public int Value { get; set; } + } + } + } + + namespace A.B_ + { + [ExcelSerializable] + public partial class C + { + public int Value { get; set; } + } + } + """; + (ImmutableCompilationResult result, ImmutableArray diagnostics) = RunGenerator(source); + Assert.Empty(diagnostics); + EmitResult emit = result.Emit(); + Assert.True(emit.Success, string.Join(Environment.NewLine, emit.Diagnostics.Select(static d => d.ToString()))); + } + [Fact] public void KeywordNamedPropertiesCompile() { From b5068d1242d77144bff29773fb08ac214403eccf Mon Sep 17 00:00:00 2001 From: GabrielMarquezMatte Date: Mon, 26 Oct 2026 13:17:52 -0300 Subject: [PATCH 06/20] fix(reader): keep TryGetDateTime from throwing at the last serial 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. --- src/ExcelReader.Core/Reader/Cell.cs | 18 +++++++++++++-- .../Reader/CellVariantTests.cs | 22 +++++++++++++++++++ 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/src/ExcelReader.Core/Reader/Cell.cs b/src/ExcelReader.Core/Reader/Cell.cs index 285e01ef..e42080e9 100644 --- a/src/ExcelReader.Core/Reader/Cell.cs +++ b/src/ExcelReader.Core/Reader/Cell.cs @@ -355,13 +355,27 @@ public bool TryGetDateTime(bool isDate1904, out DateTime result) double oadate = ExcelEpoch.SerialToOADate(serial, isDate1904); if (oadate is > -657435.0 and < 2958466.0) { - result = DateTime.FromOADate(oadate); - return true; + return TryFromOADate(oadate, out result); } result = default; return false; } + // FromOADate rounds to the millisecond, which can carry a serial just under the upper bound past DateTime.MaxValue. + private static bool TryFromOADate(double oadate, out DateTime result) + { + try + { + result = DateTime.FromOADate(oadate); + return true; + } + catch (ArgumentException) + { + result = default; + return false; + } + } + /// /// Writes the cell's text into as UTF-8, without allocating; /// returns false if the buffer is too small. diff --git a/tests/ExcelReader.Tests/Reader/CellVariantTests.cs b/tests/ExcelReader.Tests/Reader/CellVariantTests.cs index 2bc0f781..ab27e67f 100644 --- a/tests/ExcelReader.Tests/Reader/CellVariantTests.cs +++ b/tests/ExcelReader.Tests/Reader/CellVariantTests.cs @@ -180,6 +180,28 @@ public async Task TryGetDateTimeOutOfRangeSerialReturnsFalse() Assert.False(e.Current[0].TryGetDateTime(isDate1904: false, out _)); } + [Theory] + [InlineData("2958465.9999999995", false)] + [InlineData("2957003.9999999995", true)] + public void TryGetDateTimeReturnsFalse_When_TheSerialRoundsPastTheMaximumDate(string serial, bool isDate1904) + { + Cell cell = new(CellType.Date, System.Text.Encoding.UTF8.GetBytes(serial)); + + Assert.False(cell.TryGetDateTime(isDate1904, out DateTime result)); + Assert.Equal(default, result); + } + + [Theory] + [InlineData("2958465.99999998", false)] + [InlineData("2957003.99999998", true)] + public void TryGetDateTimeReadsTheLastRepresentableMillisecond(string serial, bool isDate1904) + { + Cell cell = new(CellType.Date, System.Text.Encoding.UTF8.GetBytes(serial)); + + Assert.True(cell.TryGetDateTime(isDate1904, out DateTime result)); + Assert.Equal(new DateTime(9999, 12, 31, 0, 0, 0, DateTimeKind.Unspecified), result.Date); + } + [Fact] public async Task LeaveOpenFalseClosesStreamOnDispose() { From a7962de6b351cf34a117d3ab7283cbb65cd77a25 Mon Sep 17 00:00:00 2001 From: GabrielMarquezMatte Date: Mon, 26 Oct 2026 13:17:52 -0300 Subject: [PATCH 07/20] fix(python): raise when a raw stream would block instead of dropping 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. --- python/src/excelreader/writer.py | 7 +++++- python/tests/test_writer.py | 42 ++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/python/src/excelreader/writer.py b/python/src/excelreader/writer.py index 0f070f8f..fc7c1729 100644 --- a/python/src/excelreader/writer.py +++ b/python/src/excelreader/writer.py @@ -215,12 +215,17 @@ def _typed_table(data: Any, types: list[ColumnType] | None) -> tuple[TypedTable, def _write_all(destination: IO[bytes], data: bytes) -> None: """Calls `write()` until it has taken all of `data`. A `write()` that returns None is taken to - have written everything, as files that predate the `int` return convention do. + have written everything, as files that predate the `int` return convention do, except on a raw + stream, where None means the write would block and nothing was written. """ view = memoryview(data) offset = 0 while offset < len(data): written = destination.write(data if offset == 0 else view[offset:]) + if written is None and isinstance(destination, io.RawIOBase): + raise BlockingIOError( + f"write() would block with {len(data) - offset} bytes left to write; use a blocking stream" + ) if written is None: return if not 0 < written <= len(data) - offset: diff --git a/python/tests/test_writer.py b/python/tests/test_writer.py index 4d0c522c..16bb743b 100644 --- a/python/tests/test_writer.py +++ b/python/tests/test_writer.py @@ -435,6 +435,48 @@ def test_encrypt_package_finishes_a_partial_write(source_csv): assert list(result.columns[0]) == ["widget", "gadget"] +class _WouldBlockRaw(io.RawIOBase): + def writable(self): + return True + + def write(self, data): + return None + + +class _LegacySink: + def __init__(self): + self.data = bytearray() + + def write(self, chunk): + self.data += chunk + + +def test_write_workbook_raises_when_a_raw_stream_would_block(source_csv): + with open_workbook(source_csv) as workbook: + table = workbook.sheets[0].parse(_SCHEMA) + + with pytest.raises(BlockingIOError): + write_workbook(_WouldBlockRaw(), table, _TYPES, format="xlsx") + + +def test_encrypt_package_raises_when_a_raw_stream_would_block(source_csv): + with open_workbook(source_csv) as workbook: + table = workbook.sheets[0].parse(_SCHEMA) + + with pytest.raises(BlockingIOError): + encrypt_package(_to_bytes(table), _WouldBlockRaw(), "hunter2") + + +def test_write_workbook_accepts_a_sink_whose_write_returns_none(source_csv): + with open_workbook(source_csv) as workbook: + table = workbook.sheets[0].parse(_SCHEMA) + destination = _LegacySink() + + write_workbook(destination, table, _TYPES, format="xlsx") + + assert bytes(destination.data) == _to_bytes(table) + + def _one_column_table(column, rows=1, validity=None): return TypedTable(row_count=rows, names=["n"], columns=[column], validity=[validity]) From c68f5b88e0d033f4568fac751bd40134b218dc1e Mon Sep 17 00:00:00 2001 From: GabrielMarquezMatte Date: Mon, 26 Oct 2026 13:47:03 -0300 Subject: [PATCH 08/20] fix(native): keep the global cell limit and interning for native CSV 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. --- src/ExcelReader.Core/Reader/Excel.cs | 2 +- src/ExcelReader.Native/NativeOpenOptions.cs | 10 +------ .../Reading/ReadApi.Sources.cs | 2 +- .../Native/CallbackStreamTests.cs | 30 +++++++++++++++++++ .../Native/NativeApiTests.Reading.cs | 26 ++++++++++++++++ 5 files changed, 59 insertions(+), 11 deletions(-) diff --git a/src/ExcelReader.Core/Reader/Excel.cs b/src/ExcelReader.Core/Reader/Excel.cs index 76ebff13..d30e98d9 100644 --- a/src/ExcelReader.Core/Reader/Excel.cs +++ b/src/ExcelReader.Core/Reader/Excel.cs @@ -588,7 +588,7 @@ public static ValueTask OpenAsync(Stream stream, ExcelFileFormat }; } - private static CsvReaderOptions CsvOf(ExcelReaderOptions? options) + internal static CsvReaderOptions CsvOf(ExcelReaderOptions? options) { options ??= ExcelReaderOptions.Default; CsvReaderOptions csv = options.Csv ?? CsvReaderOptions.Default; diff --git a/src/ExcelReader.Native/NativeOpenOptions.cs b/src/ExcelReader.Native/NativeOpenOptions.cs index 70e5a36e..8ffc9746 100644 --- a/src/ExcelReader.Native/NativeOpenOptions.cs +++ b/src/ExcelReader.Native/NativeOpenOptions.cs @@ -139,15 +139,7 @@ internal CsvReaderOptions ToCsvReaderOptions() { options = options with { DetectEncodingFromByteOrderMark = detectBom }; } - if (CsvMaxCellBytes is int maxCellBytes) - { - options = options with { MaxCellBytes = maxCellBytes }; - } - if (CsvInternStrings is bool internStrings) - { - options = options with { InternStrings = internStrings }; - } - return options with { MaxCellBytesOverride = options.MaxCellBytes, InternStringsOverride = options.InternStrings }; + return options with { MaxCellBytesOverride = CsvMaxCellBytes, InternStringsOverride = CsvInternStrings }; } internal ExcelReaderOptions ToExcelReaderOptions() diff --git a/src/ExcelReader.Native/Reading/ReadApi.Sources.cs b/src/ExcelReader.Native/Reading/ReadApi.Sources.cs index c08e7a60..ba4a1bf0 100644 --- a/src/ExcelReader.Native/Reading/ReadApi.Sources.cs +++ b/src/ExcelReader.Native/Reading/ReadApi.Sources.cs @@ -75,7 +75,7 @@ internal static int OpenStream(CallbackReadStream stream, int format, NativeOpen IExcelWorkbook workbook; if (format == NativeFormat.Csv || (format == NativeFormat.Auto && !IsWorkbookSignature(stream.Peek(8)))) { - workbook = Excel.OpenCsv(stream, leaveOpen: false, readerOptions.Csv ?? CsvReaderOptions.Default); + workbook = Excel.OpenCsv(stream, leaveOpen: false, Excel.CsvOf(readerOptions)); } else { diff --git a/tests/ExcelReader.Tests/Native/CallbackStreamTests.cs b/tests/ExcelReader.Tests/Native/CallbackStreamTests.cs index 97bc5e37..767d879e 100644 --- a/tests/ExcelReader.Tests/Native/CallbackStreamTests.cs +++ b/tests/ExcelReader.Tests/Native/CallbackStreamTests.cs @@ -138,6 +138,36 @@ public void An_Excel_Workbook_Is_Buffered_And_The_Stream_Released_Before_The_Ope Assert.Equal(1, test.Releases); } + [Theory] + [InlineData(NativeFormat.Csv)] + [InlineData(NativeFormat.Auto)] + public void A_Csv_Stream_Applies_The_Global_Max_Cell_Bytes(int format) + { + TestStream test = new(Encoding.UTF8.GetBytes("name\n" + new string('x', 200_000) + "\n")); + using Pinned pinned = new(test); + Assert.Equal(NativeStatus.Ok, Open(pinned, format, out NativeHandle? handle, Options() with { MaxCellBytes = 16 })); + using (handle) + { + Assert.Equal(NativeStatus.Ok, ReadApi.OpenRows(handle, 0, out NativeRowCursor? opened)); + using NativeRowCursor cursor = opened!; + Assert.Equal(NativeStatus.Ok, ReadApi.NextRowView(cursor, out _)); + Assert.Equal(NativeStatus.Error, ReadApi.NextRowView(cursor, out _)); + Assert.Contains("MaxCellBytes", NativeApi.LastErrorText(), StringComparison.Ordinal); + } + } + + [Fact] + public void A_Csv_Stream_Prefers_Csv_Max_Cell_Bytes_Over_The_Global_One() + { + TestStream test = new(Encoding.UTF8.GetBytes("name\n" + new string('x', 200) + "\n")); + using Pinned pinned = new(test); + Assert.Equal(NativeStatus.Ok, Open(pinned, NativeFormat.Csv, out NativeHandle? handle, Options() with { MaxCellBytes = 16, CsvMaxCellBytes = 1024 })); + using (handle) + { + Assert.Equal(2, CountRows(handle!, 0)); + } + } + [Fact] public void A_Stream_Past_The_Buffer_Limit_Fails_And_Is_Released_Once() { diff --git a/tests/ExcelReader.Tests/Native/NativeApiTests.Reading.cs b/tests/ExcelReader.Tests/Native/NativeApiTests.Reading.cs index 90af0fc3..5e660621 100644 --- a/tests/ExcelReader.Tests/Native/NativeApiTests.Reading.cs +++ b/tests/ExcelReader.Tests/Native/NativeApiTests.Reading.cs @@ -391,6 +391,32 @@ public void OpenFile_Applies_A_Tiny_Csv_Max_Cell_Bytes() } } + [Fact] + public void OpenFile_Applies_The_Global_Max_Cell_Bytes_To_A_Csv() + { + string path = Path.Combine(Path.GetTempPath(), $"excelreader-native-{Guid.NewGuid():N}.csv"); + File.WriteAllText(path, "name\n" + new string('x', 100_000) + "\n"); + try + { + NativeOpenOptionsRaw options = DefaultRawOptions() with { MaxCellBytes = 16 }; + Assert.Equal(NativeStatus.Ok, ReadApi.OpenFile(Encoding.UTF8.GetBytes(path), NativeFormat.Csv, options, out NativeHandle? handle)); + try + { + using NativeRowCursor cursor = OpenRows(handle); + Assert.Equal(NativeStatus.Ok, ReadApi.NextRow(cursor, new byte[128 * 1024], out _)); + Assert.Equal(NativeStatus.Error, ReadApi.NextRow(cursor, new byte[128 * 1024], out _)); + } + finally + { + ReadApi.Close(handle); + } + } + finally + { + File.Delete(path); + } + } + [Fact] public void Should_Open_When_Password_Passed_Across_Abi() { From 032c6b3effc5db406ca6ac6269bc034dac770045 Mon Sep 17 00:00:00 2001 From: GabrielMarquezMatte Date: Mon, 26 Oct 2026 13:47:04 -0300 Subject: [PATCH 09/20] fix(native): keep failing after a write handle fails to finish 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. --- .../Writer/NativeWriterHandle.cs | 30 +++++++++++++------ .../Native/NativeWriterHandleTests.cs | 16 ++++++++++ 2 files changed, 37 insertions(+), 9 deletions(-) diff --git a/src/ExcelReader.Native/Writer/NativeWriterHandle.cs b/src/ExcelReader.Native/Writer/NativeWriterHandle.cs index b017512b..70960667 100644 --- a/src/ExcelReader.Native/Writer/NativeWriterHandle.cs +++ b/src/ExcelReader.Native/Writer/NativeWriterHandle.cs @@ -87,6 +87,7 @@ internal sealed class NativeWriterHandle : NativeWriterHandle private TSheet? _sheet; private TRow? _row; private bool _closed; + private Exception? _closeFailure; internal NativeWriterHandle(IWorkbookWriter workbook, ParallelWriteOptions? parallel, Func, Action, ParallelWriteOptions, ValueTask>? writeParallel) @@ -205,21 +206,32 @@ internal override void WriteSheet(string name, NativeColumnSpec[] specs, NativeT internal override void Close() { + if (_closeFailure is not null) + { + throw new InvalidOperationException($"The workbook failed to finish and has no output: {_closeFailure.Message}", _closeFailure); + } if (_closed) { return; } - _closed = true; - - _row?.Dispose(); - _row = null; - if (_sheet is not null) + try + { + _row?.Dispose(); + _row = null; + if (_sheet is not null) + { + _sheet.End(); + _sheet.Dispose(); + _sheet = null; + } + _workbook.End(); + _closed = true; + } + catch (Exception exception) { - _sheet.End(); - _sheet.Dispose(); - _sheet = null; + _closeFailure = exception; + throw; } - _workbook.End(); } protected override void Dispose(bool disposing) diff --git a/tests/ExcelReader.Tests/Native/NativeWriterHandleTests.cs b/tests/ExcelReader.Tests/Native/NativeWriterHandleTests.cs index 15e4fc2b..137013a3 100644 --- a/tests/ExcelReader.Tests/Native/NativeWriterHandleTests.cs +++ b/tests/ExcelReader.Tests/Native/NativeWriterHandleTests.cs @@ -422,6 +422,22 @@ public void GetWriteHandleBytes_Should_Round_Trip_A_Streamed_Row_Through_OpenMem Assert.Equal(NativeStatus.Ok, WriteApi.CloseWriteHandle(handle)); } + [Fact] + public void GetWriteHandleBytes_Should_Keep_Failing_When_Finishing_The_Workbook_Failed() + { + Assert.Equal(NativeStatus.Ok, OpenWriteHandleToMemory(NativeFormat.Xlsx, out NativeWriterHandle? handle)); + Assert.NotNull(handle); + + Assert.Equal(NativeStatus.Error, WriteApi.GetWriteHandleBytes(handle, out byte[]? first)); + string firstError = NativeApi.LastErrorText(); + Assert.Equal(NativeStatus.Error, WriteApi.GetWriteHandleBytes(handle, out byte[]? second)); + Assert.Equal(NativeStatus.Error, WriteApi.CloseWriteHandle(handle)); + + Assert.Null(first); + Assert.Null(second); + Assert.Contains(firstError, NativeApi.LastErrorText(), StringComparison.Ordinal); + } + [Fact] public void GetWriteHandleBytes_Should_Not_Require_EndSheet_First() { From 40eb5adcf5f61b16a049bb7b1a03672dcf85c15c Mon Sep 17 00:00:00 2001 From: GabrielMarquezMatte Date: Mon, 26 Oct 2026 13:47:04 -0300 Subject: [PATCH 10/20] fix(schema): treat binary 0 and 1 as boolean-compatible in XLSB and XLS 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. --- .../Reader/Schema/SchemaInference.cs | 12 +++- .../Reader/SchemaInferenceTests.cs | 62 +++++++++++++++++++ 2 files changed, 73 insertions(+), 1 deletion(-) diff --git a/src/ExcelReader.Core/Reader/Schema/SchemaInference.cs b/src/ExcelReader.Core/Reader/Schema/SchemaInference.cs index 2b2ca687..0aab3ca5 100644 --- a/src/ExcelReader.Core/Reader/Schema/SchemaInference.cs +++ b/src/ExcelReader.Core/Reader/Schema/SchemaInference.cs @@ -156,7 +156,7 @@ internal void Observe(in Cell cell, bool isDate1904, bool parseText) { SawNonIntegralNumber = true; } - SawNonBinaryNumber |= !IsBinaryDigit(cell.Value); + SawNonBinaryNumber |= !IsBinaryNumber(in cell); break; case CellType.Date: bool hasTime = cell.TryGetDateTime(isDate1904, out DateTime date) && date.TimeOfDay != TimeSpan.Zero; @@ -223,6 +223,16 @@ private static bool HasCanonicalIntegerPart(ReadOnlySpan text, out bool al return end - start <= 1 || text[start] != (byte)'0'; } + // XLS and XLSB numbers carry no text, only the stored double. + private static bool IsBinaryNumber(in Cell cell) + { + if (!cell.Value.IsEmpty) + { + return IsBinaryDigit(cell.Value); + } + return cell.TryGetDouble(out double number) && number is 0.0 or 1.0; + } + private static bool IsBinaryDigit(ReadOnlySpan text) { return text.Length == 1 && text[0] is (byte)'0' or (byte)'1'; diff --git a/tests/ExcelReader.Tests/Reader/SchemaInferenceTests.cs b/tests/ExcelReader.Tests/Reader/SchemaInferenceTests.cs index 238019b8..715f1029 100644 --- a/tests/ExcelReader.Tests/Reader/SchemaInferenceTests.cs +++ b/tests/ExcelReader.Tests/Reader/SchemaInferenceTests.cs @@ -2,6 +2,9 @@ using ExcelReader.Core.Reader.Csv; using ExcelReader.Core.Reader.Schema; using ExcelReader.Core.Reader.Xlsx; +using ExcelReader.Core.Writer; +using ExcelReader.Core.Writer.Xls; +using ExcelReader.Core.Writer.Xlsb; using ExcelReader.Core.Writer.Xlsx; namespace ExcelReader.Tests.Reader @@ -94,6 +97,65 @@ public void ParseText_Should_Infer_Integers_And_Decimals_But_Not_Codes() TypesOf(schema)); } + [Theory] + [InlineData("xlsx")] + [InlineData("xlsb")] + [InlineData("xls")] + public void ParseText_Should_Infer_The_Same_Types_From_Every_Format_When_Booleans_Meet_Binary_Numbers(string extension) + { + using MemoryStream buffer = new(); + WriteBoolsAndNumbers(extension, buffer); + buffer.Position = 0; + using IExcelWorkbook reader = Excel.Open(buffer, ExcelFileFormat.Unknown); + + IReadOnlyList schema = Excel.InferSchema(reader.FirstSheet, new SchemaInferenceOptions { HeaderRow = 0, ParseText = true }); + + Assert.Equal([ExcelColumnType.StringColumn, ExcelColumnType.BoolColumn], schema.Select(static column => column.Type)); + } + + private static void WriteBoolsAndNumbers(string extension, Stream stream) + { + if (string.Equals(extension, "xlsb", StringComparison.Ordinal)) + { + WriteBoolsAndNumbers(XlsbWorkbookWriter.Create(stream, leaveOpen: true)); + return; + } + if (string.Equals(extension, "xls", StringComparison.Ordinal)) + { + WriteBoolsAndNumbers(XlsWorkbookWriter.Create(stream, leaveOpen: true)); + return; + } + WriteBoolsAndNumbers(XlsxWorkbookWriter.Create(stream, leaveOpen: true)); + } + + private static void WriteBoolsAndNumbers(IWorkbookWriter workbook) + where TSheet : ISheetWriter + where TRow : IRowWriter + { + using (workbook) + using (TSheet sheet = workbook.AddSheet("S")) + { + foreach ((object first, object second) in ((object, object)[])[(0, true), (true, 1), (2, true)]) + { + using TRow row = sheet.StartRow(); + WriteValue(row, first); + WriteValue(row, second); + } + sheet.End(); + workbook.End(); + } + } + + private static void WriteValue(IRowWriter row, object value) + { + if (value is bool flag) + { + row.Write(flag); + return; + } + row.Write((int)value); + } + [Fact] public void ParseText_Should_Infer_Booleans_Without_Swallowing_Binary_Integers() { From 133e08086d27fa5633be4eaf90f11aada48f08d7 Mon Sep 17 00:00:00 2001 From: GabrielMarquezMatte Date: Mon, 26 Oct 2026 13:47:04 -0300 Subject: [PATCH 11/20] fix(writer): give staged files a short name of their own 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-.tmp in the destination's directory. --- .../Writer/Internal/StagedFile.cs | 2 +- .../Writer/StagedFileTests.cs | 32 +++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) create mode 100644 tests/ExcelReader.Tests/Writer/StagedFileTests.cs diff --git a/src/ExcelReader.Core/Writer/Internal/StagedFile.cs b/src/ExcelReader.Core/Writer/Internal/StagedFile.cs index a0781f64..9921e434 100644 --- a/src/ExcelReader.Core/Writer/Internal/StagedFile.cs +++ b/src/ExcelReader.Core/Writer/Internal/StagedFile.cs @@ -16,7 +16,7 @@ internal StagedFile(string destination, bool useAsync = false) { _destination = Path.GetFullPath(destination); _temporary = Path.Combine(Path.GetDirectoryName(_destination) ?? string.Empty, - $".{Path.GetFileName(_destination)}.{Guid.NewGuid():N}.tmp"); + $".excelreader-{Guid.NewGuid():N}.tmp"); Stream = new FileStream(_temporary, FileMode.CreateNew, FileAccess.Write, FileShare.None, bufferSize: 4096, useAsync); } diff --git a/tests/ExcelReader.Tests/Writer/StagedFileTests.cs b/tests/ExcelReader.Tests/Writer/StagedFileTests.cs new file mode 100644 index 00000000..5c706182 --- /dev/null +++ b/tests/ExcelReader.Tests/Writer/StagedFileTests.cs @@ -0,0 +1,32 @@ +using ExcelReader.Core.Writer.Internal; + +namespace ExcelReader.Tests.Writer +{ + public sealed class StagedFileTests + { + [Fact] + public void Should_Commit_When_TheDestinationNameIsAsLongAsTheFileSystemAllows() + { + string directory = Path.Combine(Path.GetTempPath(), $"excelreader-staged-{Guid.NewGuid():N}"); + Directory.CreateDirectory(directory); + try + { + string destination = Path.Combine(directory, new string('a', 240) + ".xlsx"); + File.WriteAllText(destination, "ORIGINAL"); + + using (StagedFile staged = new(destination)) + { + staged.Stream.Write("new"u8); + staged.Commit(); + } + + Assert.Equal("new", File.ReadAllText(destination)); + Assert.Equal([destination], Directory.GetFiles(directory)); + } + finally + { + Directory.Delete(directory, recursive: true); + } + } + } +} From 83dbb13f0448f25fe5eed41b5b4f654e9cf6eaf3 Mon Sep 17 00:00:00 2001 From: GabrielMarquezMatte Date: Mon, 26 Oct 2026 15:04:46 -0300 Subject: [PATCH 12/20] refactor(native): write xl_write_typed through NativeWriterHandle 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. --- .../Writer/NativeWriterHandle.cs | 6 ++- src/ExcelReader.Native/Writer/WriteApi.cs | 50 ++----------------- .../Write/WritePathBenchmark.cs | 36 +++++++++++++ .../Native/NativeApiTests.Writer.cs | 14 ++++-- 4 files changed, 52 insertions(+), 54 deletions(-) diff --git a/src/ExcelReader.Native/Writer/NativeWriterHandle.cs b/src/ExcelReader.Native/Writer/NativeWriterHandle.cs index 70960667..b59bce87 100644 --- a/src/ExcelReader.Native/Writer/NativeWriterHandle.cs +++ b/src/ExcelReader.Native/Writer/NativeWriterHandle.cs @@ -240,8 +240,10 @@ protected override void Dispose(bool disposing) { return; } - _row?.Dispose(); - _sheet?.Dispose(); + // A row or sheet still open here belongs to output that is being discarded; ending it must + // not replace the failure that left it open. + FailureCleanup.Dispose(_row); + FailureCleanup.Dispose(_sheet); _workbook.Dispose(); } } diff --git a/src/ExcelReader.Native/Writer/WriteApi.cs b/src/ExcelReader.Native/Writer/WriteApi.cs index 5063de78..745b4710 100644 --- a/src/ExcelReader.Native/Writer/WriteApi.cs +++ b/src/ExcelReader.Native/Writer/WriteApi.cs @@ -1,11 +1,7 @@ using System.Diagnostics.CodeAnalysis; using System.Text; using ExcelReader.Core.Writer; -using ExcelReader.Core.Writer.Csv; using ExcelReader.Core.Writer.Internal; -using ExcelReader.Core.Writer.Xls; -using ExcelReader.Core.Writer.Xlsb; -using ExcelReader.Core.Writer.Xlsx; using ExcelReader.Native.Typed; namespace ExcelReader.Native.Writer @@ -193,49 +189,9 @@ private static bool IsWritableFormat(int format) private static void WriteToStream(Stream stream, int format, NativeColumnSpec[] specs, NativeTable table, NativeWriteOptions options, string sheetName, bool hasHeader) { - bool date1904 = options.Date1904 ?? false; - bool sharedStrings = options.UseSharedStrings ?? false; - ParallelWriteOptions? parallel = options.ToParallelWriteOptions(format); - switch (format) - { - case NativeFormat.Xlsx: - WriteWorkbook( - XlsxWorkbookWriter.Create(stream, options: new XlsxWriterOptions { Date1904 = date1904, UseSharedStrings = sharedStrings }), specs, table, sheetName, hasHeader, - parallel, static (sheet, rows, writeRow, opts) => sheet.WriteRecordsParallelAsync(rows, writeRow, opts)); - return; - case NativeFormat.Xlsb: - WriteWorkbook( - XlsbWorkbookWriter.Create(stream, options: new XlsbWriterOptions { Date1904 = date1904, UseSharedStrings = sharedStrings }), specs, table, sheetName, hasHeader, - parallel, static (sheet, rows, writeRow, opts) => sheet.WriteRecordsParallelAsync(rows, writeRow, opts)); - return; - case NativeFormat.Xls: - WriteWorkbook( - XlsWorkbookWriter.Create(stream, options: new XlsWriterOptions { Date1904 = date1904 }), specs, table, sheetName, hasHeader, null, null); - return; - default: - WriteWorkbook( - CsvWorkbookWriter.Create(stream, options: options.ToCsvWriterOptions()), specs, table, sheetName, hasHeader, null, null); - return; - } - } - - private static void WriteWorkbook(IWorkbookWriter workbook, NativeColumnSpec[] specs, NativeTable table, string sheetName, bool hasHeader, - ParallelWriteOptions? parallel, Func, Action, ParallelWriteOptions, ValueTask>? writeParallel) - where TSheet : ISheetWriter - where TRow : IRowWriter - { - try - { - TSheet sheet = workbook.AddSheet(sheetName); - WriteSheetBody(workbook, sheet, specs, table, hasHeader, null, parallel, writeParallel); - sheet.End(); - sheet.Dispose(); - workbook.End(); - } - finally - { - workbook.Dispose(); - } + using NativeWriterHandle handle = NativeWriterHandle.Create(stream, format, options); + handle.WriteSheet(sheetName, specs, table, hasHeader, excelTable: null); + handle.Close(); } internal static void WriteSheetBody(IWorkbookWriter workbook, TSheet sheet, NativeColumnSpec[] specs, NativeTable table, bool hasHeader, diff --git a/tests/ExcelReader.Benchmarks/Write/WritePathBenchmark.cs b/tests/ExcelReader.Benchmarks/Write/WritePathBenchmark.cs index 91a16a18..ff4e1817 100644 --- a/tests/ExcelReader.Benchmarks/Write/WritePathBenchmark.cs +++ b/tests/ExcelReader.Benchmarks/Write/WritePathBenchmark.cs @@ -21,6 +21,7 @@ public unsafe class WritePathBenchmark private readonly MemoryStream _output = new(32 * 1024 * 1024); private NativeColumn[] _columns = []; + private NativeColumnSpec[] _specs = []; private readonly List _native = []; private RecordBatch _batch = null!; @@ -55,6 +56,7 @@ public void Setup() schema.Field(new Field($"c{c}", Apache.Arrow.Types.StringType.Default, nullable: false)); } _batch = new RecordBatch(schema.Build(), arrays, Rows); + _specs = [.. Enumerable.Range(0, StringColumns).Select(static c => new NativeColumnSpec { Names = [$"c{c}"], Type = NativeColumnType.String })]; } private IntPtr Pin(ReadOnlySpan bytes) @@ -98,6 +100,40 @@ public long NativeStrings_Xlsx() return _output.Length; } + [Benchmark] + public long BulkTyped_Xlsx_Serial() + { + return BulkTyped(NativeFormat.Xlsx, degreeOfParallelism: 1); + } + + [Benchmark] + public long BulkTyped_Xlsb_Serial() + { + return BulkTyped(NativeFormat.Xlsb, degreeOfParallelism: 1); + } + + [Benchmark] + public long BulkTyped_Xlsx_Parallel() + { + return BulkTyped(NativeFormat.Xlsx, degreeOfParallelism: 0); + } + + [Benchmark] + public long BulkTyped_Xlsb_Parallel() + { + return BulkTyped(NativeFormat.Xlsb, degreeOfParallelism: 0); + } + + private long BulkTyped(int format, int degreeOfParallelism) + { + fixed (NativeColumn* columns = _columns) + { + NativeTable table = new() { ColumnCount = StringColumns, RowCount = Rows, Columns = (IntPtr)columns }; + WriteApi.WriteTypedToMemory(format, _specs, table, new NativeWriteOptions { DegreeOfParallelism = degreeOfParallelism }, out byte[]? bytes); + return bytes!.Length; + } + } + [Benchmark] public long NativeStrings_Csv() { diff --git a/tests/ExcelReader.Tests/Native/NativeApiTests.Writer.cs b/tests/ExcelReader.Tests/Native/NativeApiTests.Writer.cs index decef2a2..d6298131 100644 --- a/tests/ExcelReader.Tests/Native/NativeApiTests.Writer.cs +++ b/tests/ExcelReader.Tests/Native/NativeApiTests.Writer.cs @@ -1244,21 +1244,25 @@ public void WriteTyped_Should_Reject_A_Spec_With_More_Than_One_Name() } } - [Fact] - public void WriteTyped_Should_Keep_The_Existing_File_When_A_Cell_Is_Rejected() + [Theory] + [InlineData(NativeFormat.Xlsx)] + [InlineData(NativeFormat.Xlsb)] + [InlineData(NativeFormat.Xls)] + public void WriteTyped_Should_Keep_The_Existing_File_When_A_Cell_Is_Rejected(int format) { string directory = Path.Combine(Path.GetTempPath(), $"excelreader-native-{Guid.NewGuid():N}"); Directory.CreateDirectory(directory); - string path = Path.Combine(directory, "out.xlsx"); + string path = Path.Combine(directory, "out.dat"); File.WriteAllText(path, "ORIGINAL"); - NativeTable table = BuildFloat64Table([double.NaN]); + NativeTable table = BuildFloat64Table([1.5, double.NaN]); try { NativeColumnSpec[] specs = [new() { Names = ["x"], Type = NativeColumnType.Float64 }]; Assert.Equal(NativeStatus.Error, WriteApi.WriteTyped( - Encoding.UTF8.GetBytes(path), NativeFormat.Xlsx, specs, table, DefaultWriteOptions())); + Encoding.UTF8.GetBytes(path), format, specs, table, DefaultWriteOptions())); + Assert.Contains("non-finite", NativeApi.LastErrorText(), StringComparison.Ordinal); Assert.Equal("ORIGINAL", File.ReadAllText(path)); Assert.Equal([path], Directory.GetFiles(directory)); } From d0fea37da1f418a041feb2391e5121b287a64547 Mon Sep 17 00:00:00 2001 From: GabrielMarquezMatte Date: Mon, 26 Oct 2026 15:04:46 -0300 Subject: [PATCH 13/20] refactor(native): copy xl_last_error from the bytes SetLastError encoded 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. --- src/ExcelReader.Native/NativeApi.cs | 14 +++----------- .../Native/NativeApiTests.cs | 19 +++++++++++++++++++ 2 files changed, 22 insertions(+), 11 deletions(-) diff --git a/src/ExcelReader.Native/NativeApi.cs b/src/ExcelReader.Native/NativeApi.cs index e6f3088e..f989dc57 100644 --- a/src/ExcelReader.Native/NativeApi.cs +++ b/src/ExcelReader.Native/NativeApi.cs @@ -37,21 +37,13 @@ internal static string LastErrorText() internal static int LastError(Span buffer, out int length) { - string? message = _lastError; - if (string.IsNullOrEmpty(message)) - { - length = 0; - return NativeStatus.Ok; - } - - int required = Encoding.UTF8.GetByteCount(message); - length = required; - if (buffer.Length < required) + length = _lastErrorUtf8Length; + if (buffer.Length < length) { return NativeStatus.BufferTooSmall; } - Encoding.UTF8.GetBytes(message, buffer); + _lastErrorUtf8.AsSpan(0, length).CopyTo(buffer); return NativeStatus.Ok; } diff --git a/tests/ExcelReader.Tests/Native/NativeApiTests.cs b/tests/ExcelReader.Tests/Native/NativeApiTests.cs index b5f38724..cc661e14 100644 --- a/tests/ExcelReader.Tests/Native/NativeApiTests.cs +++ b/tests/ExcelReader.Tests/Native/NativeApiTests.cs @@ -160,6 +160,25 @@ public void LastError_Should_Report_Required_Size_When_Buffer_Too_Small() Assert.Equal(4, length); } + [Fact] + public void LastError_Should_Return_Only_The_Latest_Non_Ascii_Message() + { + NativeApi.SetLastError("planilha não encontrada: relatório ✓"); + NativeApi.SetLastError("ação"); + + Span buffer = stackalloc byte[64]; + Assert.Equal(NativeStatus.Ok, NativeApi.LastError(buffer, out int length)); + Assert.Equal("ação", Encoding.UTF8.GetString(buffer[..length])); + Assert.Equal(NativeStatus.Ok, NativeApi.LastError(buffer, out int again)); + Assert.Equal(length, again); + + Span tooSmall = stackalloc byte[5]; + tooSmall.Fill(0xAA); + Assert.Equal(NativeStatus.BufferTooSmall, NativeApi.LastError(tooSmall, out int required)); + Assert.Equal(6, required); + Assert.All(tooSmall.ToArray(), static b => Assert.Equal(0xAA, b)); + } + [Fact] public void LastError_Should_Return_Zero_Length_When_Cleared() { From 7bfca3155f1ee92c9956434700145f1e91838f32 Mon Sep 17 00:00:00 2001 From: GabrielMarquezMatte Date: Mon, 26 Oct 2026 17:37:16 -0300 Subject: [PATCH 14/20] perf(native): bind typed column names through one pass over the header 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 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. --- src/ExcelReader.Native/Typed/TypedApi.cs | 19 ++++---- .../Native/NativeWideHeaderBenchmark.cs | 46 +++++++++++++++++++ .../Native/NativeApiTests.Typed.cs | 29 ++++++++++++ 3 files changed, 84 insertions(+), 10 deletions(-) create mode 100644 tests/ExcelReader.Benchmarks/Native/NativeWideHeaderBenchmark.cs diff --git a/src/ExcelReader.Native/Typed/TypedApi.cs b/src/ExcelReader.Native/Typed/TypedApi.cs index b2b0fb29..e8e0f1ce 100644 --- a/src/ExcelReader.Native/Typed/TypedApi.cs +++ b/src/ExcelReader.Native/Typed/TypedApi.cs @@ -218,7 +218,7 @@ private static bool TryResolveColumns(IExcelRowEnumerator rows, NativeColumnSpec return false; } - Row header = rows.Current; + Dictionary.AlternateLookup>? header = null; for (int i = 0; i < specs.Length; i++) { string[] names = specs[i].Names; @@ -227,14 +227,15 @@ private static bool TryResolveColumns(IExcelRowEnumerator rows, NativeColumnSpec columnIndices[i] = specs[i].Index; continue; } + header ??= HeaderColumns(rows.Current).GetAlternateLookup>(); int found = -1; foreach (string name in names) { - found = FindHeaderColumn(header, name); - if (found >= 0) + if (header.Value.TryGetValue(name.AsSpan().Trim(), out found)) { break; } + found = -1; } if (found < 0) { @@ -251,17 +252,15 @@ private static string FormatCandidates(string[] names) return string.Join(", ", Array.ConvertAll(names, n => $"\"{n}\"")); } - private static int FindHeaderColumn(Row header, string name) + // The first column carrying a header wins, as a left-to-right search would find it. + private static Dictionary HeaderColumns(Row header) { - string target = name.Trim(); + Dictionary columns = new(StringComparer.OrdinalIgnoreCase); foreach (RowCell cell in header.Cells) { - if (string.Equals(cell.Value.GetString().Trim(), target, StringComparison.OrdinalIgnoreCase)) - { - return cell.ColumnIndex; - } + columns.TryAdd(cell.Value.GetString().Trim(), cell.ColumnIndex); } - return -1; + return columns; } private static NativeTable BuildTable(ColumnBuilder[] builders) diff --git a/tests/ExcelReader.Benchmarks/Native/NativeWideHeaderBenchmark.cs b/tests/ExcelReader.Benchmarks/Native/NativeWideHeaderBenchmark.cs new file mode 100644 index 00000000..7d88fb6a --- /dev/null +++ b/tests/ExcelReader.Benchmarks/Native/NativeWideHeaderBenchmark.cs @@ -0,0 +1,46 @@ +using System.Text; +using BenchmarkDotNet.Attributes; +using ExcelReader.Native; +using ExcelReader.Native.Reading; +using ExcelReader.Native.Typed; + +namespace ExcelReader.Benchmarks +{ + // Binding every column of a wide header by name: the setup cost of a typed parse, before any data row. + [MemoryDiagnoser] + public class NativeWideHeaderBenchmark + { + [Params(512, 4096)] + public int Columns { get; set; } + + private byte[] _csv = []; + private NativeColumnSpec[] _specs = []; + + [GlobalSetup] + public void Setup() + { + string[] names = [.. Enumerable.Range(0, Columns).Select(static c => $"column_{c:D5}")]; + _csv = Encoding.UTF8.GetBytes(string.Join(',', names) + "\n"); + _specs = [.. names.Select(static name => new NativeColumnSpec { Names = [name], Type = NativeColumnType.String })]; + } + + [Benchmark] + public long BindByName() + { + if (ReadApi.OpenMemory(_csv, NativeFormat.Csv, out NativeHandle? handle) != NativeStatus.Ok || handle is null) + { + throw new InvalidOperationException("opening the header-only CSV failed."); + } + using (handle) + { + int status = TypedApi.ParseTyped(handle, 0, _specs, headerRow: 1, out NativeTable table); + if (status != NativeStatus.Ok) + { + throw new InvalidOperationException($"xl_parse_typed failed with status {status}."); + } + TypedApi.FreeTable(ref table); + return table.ColumnCount; + } + } + } +} diff --git a/tests/ExcelReader.Tests/Native/NativeApiTests.Typed.cs b/tests/ExcelReader.Tests/Native/NativeApiTests.Typed.cs index 77a73c7b..6e4d9dae 100644 --- a/tests/ExcelReader.Tests/Native/NativeApiTests.Typed.cs +++ b/tests/ExcelReader.Tests/Native/NativeApiTests.Typed.cs @@ -413,6 +413,35 @@ public void ParseTyped_Should_Resolve_The_First_Candidate_Name_Present_In_The_He } } + [Fact] + public void ParseTyped_Should_Match_Trimmed_Headers_First_Column_First_And_Candidates_In_Order() + { + string path = Path.Combine(Path.GetTempPath(), $"excelreader-native-{Guid.NewGuid():N}.csv"); + File.WriteAllText(path, "id, Qty ,qty,amount\n1,10,20,30\n"); + try + { + Assert.Equal(NativeStatus.Ok, OpenPath(path, NativeFormat.Csv, out NativeHandle? handle)); + NativeColumnSpec[] specs = + [ + new() { Names = ["amount", "qty"], Type = NativeColumnType.Int64 }, + new() { Names = [" QTY"], Type = NativeColumnType.Int64 }, + new() { Index = 0, Type = NativeColumnType.Int64 }, + ]; + + Assert.Equal(NativeStatus.Ok, TypedApi.ParseTyped(handle, 0, specs, headerRow: 1, out NativeTable table)); + + Assert.Equal(30, Marshal.ReadInt64(ColumnAt(table, 0).Values)); + Assert.Equal(10, Marshal.ReadInt64(ColumnAt(table, 1).Values)); + Assert.Equal(1, Marshal.ReadInt64(ColumnAt(table, 2).Values)); + TypedApi.FreeTable(ref table); + ReadApi.Close(handle); + } + finally + { + File.Delete(path); + } + } + [Fact] public void ParseTyped_Should_Fail_With_A_Message_Listing_Every_Candidate_When_None_Match() { From edbc95092f84bfe32cfa73d53979c7443fe326e3 Mon Sep 17 00:00:00 2001 From: GabrielMarquezMatte Date: Mon, 26 Oct 2026 17:37:16 -0300 Subject: [PATCH 15/20] fix(pipeline): return every chunk buffer when a sheet pipeline stops 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. --- .../Parser/SheetPipeline/ChunkProducer.cs | 18 +- .../SheetPipeline/SheetPipelineRunner.cs | 4 + .../SheetPipeline/ChunkOwnershipTests.cs | 176 ++++++++++++++++++ 3 files changed, 196 insertions(+), 2 deletions(-) create mode 100644 tests/ExcelReader.Tests/Parser/SheetPipeline/ChunkOwnershipTests.cs diff --git a/src/ExcelReader.Core/Parser/SheetPipeline/ChunkProducer.cs b/src/ExcelReader.Core/Parser/SheetPipeline/ChunkProducer.cs index 807342ad..b045a087 100644 --- a/src/ExcelReader.Core/Parser/SheetPipeline/ChunkProducer.cs +++ b/src/ExcelReader.Core/Parser/SheetPipeline/ChunkProducer.cs @@ -38,7 +38,7 @@ internal static async Task RunAsync( { byte[] last = buffer; buffer = null; - await emit(new SheetChunk(last, length)).ConfigureAwait(false); + await EmitAsync(emit, new SheetChunk(last, length)).ConfigureAwait(false); } return; } @@ -63,7 +63,7 @@ internal static async Task RunAsync( filled = tail; target = Math.Max(chunkBytes, tail); cutter.Rebase(cut); - await emit(new SheetChunk(full, cut)).ConfigureAwait(false); + await EmitAsync(emit, new SheetChunk(full, cut)).ConfigureAwait(false); } } finally @@ -74,5 +74,19 @@ internal static async Task RunAsync( } } } + + // emit owns the chunk once it returns; a throw means no consumer took it, so it goes back here. + private static async ValueTask EmitAsync(Func emit, SheetChunk chunk) + { + try + { + await emit(chunk).ConfigureAwait(false); + } + catch + { + ArrayPool.Shared.Return(chunk.Buffer); + throw; + } + } } } diff --git a/src/ExcelReader.Core/Parser/SheetPipeline/SheetPipelineRunner.cs b/src/ExcelReader.Core/Parser/SheetPipeline/SheetPipelineRunner.cs index b53ca460..f4ffb803 100644 --- a/src/ExcelReader.Core/Parser/SheetPipeline/SheetPipelineRunner.cs +++ b/src/ExcelReader.Core/Parser/SheetPipeline/SheetPipelineRunner.cs @@ -88,6 +88,10 @@ internal static async IAsyncEnumerable RunAsync( { await cts.CancelAsync().ConfigureAwait(false); await Task.WhenAll(tasks).ConfigureAwait(ConfigureAwaitOptions.SuppressThrowing); + while (work.Reader.TryRead(out Work unparsed)) + { + ArrayPool.Shared.Return(unparsed.Chunk.Buffer); + } } if (recover) diff --git a/tests/ExcelReader.Tests/Parser/SheetPipeline/ChunkOwnershipTests.cs b/tests/ExcelReader.Tests/Parser/SheetPipeline/ChunkOwnershipTests.cs new file mode 100644 index 00000000..9542fef1 --- /dev/null +++ b/tests/ExcelReader.Tests/Parser/SheetPipeline/ChunkOwnershipTests.cs @@ -0,0 +1,176 @@ +using System.Collections.Concurrent; +using System.Diagnostics.Tracing; +using System.Globalization; +using System.Text; +using ExcelReader.Core.Parser.ParallelCsv; +using ExcelReader.Core.Parser.SheetPipeline; +using ExcelReader.Core.Reader; +using ExcelReader.Core.Writer.Xlsb; +using ExcelReader.Core.Writer.Xlsx; + +namespace ExcelReader.Tests.Parser.SheetPipeline +{ + [CollectionDefinition(nameof(ArrayPoolEvents), DisableParallelization = true)] + public sealed class ArrayPoolEvents + { + } + + // ArrayPool.Shared reports every rent and return through its event source; running alone keeps + // other tests' rentals of the same size out of the counts. + [Collection(nameof(ArrayPoolEvents))] + public sealed class ChunkOwnershipTests + { + // Rounds up to a 512 KiB bucket, which nothing else in these tests rents. + private const int ChunkBytes = 300_000; + private const int PooledChunkSize = 512 * 1024; + + private sealed class PoolEvents : EventListener + { + internal readonly ConcurrentDictionary Outstanding = new(); + internal readonly ConcurrentBag Returned = []; + + protected override void OnEventSourceCreated(EventSource eventSource) + { + if (string.Equals(eventSource.Name, "System.Buffers.ArrayPoolEventSource", StringComparison.Ordinal)) + { + EnableEvents(eventSource, EventLevel.Verbose); + } + } + + protected override void OnEventWritten(EventWrittenEventArgs eventData) + { + if (eventData.Payload is not { Count: >= 2 } payload || payload[0] is not int id || payload[1] is not int size) + { + return; + } + if (string.Equals(eventData.EventName, "BufferReturned", StringComparison.Ordinal)) + { + Returned.Add(id); + } + if (size != PooledChunkSize) + { + return; + } + if (string.Equals(eventData.EventName, "BufferRented", StringComparison.Ordinal)) + { + Outstanding.AddOrUpdate(id, 1, static (_, count) => count + 1); + } + if (string.Equals(eventData.EventName, "BufferReturned", StringComparison.Ordinal)) + { + Outstanding.AddOrUpdate(id, -1, static (_, count) => count - 1); + } + } + } + + private static byte[] SheetXml(int rows) + { + var sb = new StringBuilder(""); + for (int r = 1; r <= rows; r++) + { + sb.Append(CultureInfo.InvariantCulture, $"{r}"); + } + return Encoding.UTF8.GetBytes(sb.Append("").ToString()); + } + + private static async Task RejectedEmitAsync(byte[] input, int chunkBytes) + { + int rejected = 0; + await Assert.ThrowsAsync(() => ChunkProducer.RunAsync( + new MemoryStream(input), new XlsxRowCutter(" + { + rejected = chunk.Buffer.GetHashCode(); + return ValueTask.FromException(new OperationCanceledException("the consumer is gone")); + }, + CancellationToken.None)); + return rejected; + } + + [Fact] + public async Task AChunkWhoseEmitFailsIsReturnedToThePool() + { + using PoolEvents events = new(); + + int rejected = await RejectedEmitAsync(SheetXml(10_000), 4096); + + Assert.Contains(rejected, events.Returned); + } + + [Fact] + public async Task TheLastChunkIsReturnedToThePoolWhenItsEmitFails() + { + using PoolEvents events = new(); + + int rejected = await RejectedEmitAsync(SheetXml(3), 4096); + + Assert.Contains(rejected, events.Returned); + } + + private static readonly RowAggregation Counting = new() + { + Seed = static () => 0, + Accumulate = static (ref long count, Row _) => count++, + Combine = static (left, right) => left + right, + }; + + private static async Task AssertEveryChunkReturnedAfterEarlyDisposeAsync(IExcelSheet sheet) + { + using PoolEvents events = new(); + SheetPipelineSource source = SheetPipelineSource.Create(sheet)!; + await using (IAsyncEnumerator results = SheetPipelineRunner.RunAsync(source, Counting, null, 0, 4, ChunkBytes, CancellationToken.None).GetAsyncEnumerator(TestContext.Current.CancellationToken)) + { + Assert.True(await results.MoveNextAsync()); + await Task.Delay(50, TestContext.Current.CancellationToken); + } + + Assert.NotEmpty(events.Outstanding); + Assert.All(events.Outstanding, static pair => Assert.Equal(0, pair.Value)); + } + + [Fact] + public async Task EveryXlsxChunkIsReturned_When_TheResultsAreAbandonedEarly() + { + using MemoryStream buffer = new(); + using (XlsxWorkbookWriter workbook = XlsxWorkbookWriter.Create(buffer, leaveOpen: true)) + { + WriteRows(workbook.AddSheet("S")); + workbook.End(); + } + buffer.Position = 0; + using var reader = Excel.FromXlsx(buffer); + + await AssertEveryChunkReturnedAfterEarlyDisposeAsync(reader.FirstSheet); + } + + [Fact] + public async Task EveryXlsbChunkIsReturned_When_TheResultsAreAbandonedEarly() + { + using MemoryStream buffer = new(); + using (XlsbWorkbookWriter workbook = XlsbWorkbookWriter.Create(buffer, leaveOpen: true)) + { + WriteRows(workbook.AddSheet("S")); + workbook.End(); + } + buffer.Position = 0; + using var reader = Excel.FromXlsb(buffer); + + await AssertEveryChunkReturnedAfterEarlyDisposeAsync(reader.FirstSheet); + } + + private static void WriteRows(Core.Writer.ISheetWriter sheet) + where TRow : Core.Writer.IRowWriter + { + using (sheet) + { + for (int r = 0; r < 200_000; r++) + { + using TRow row = sheet.StartRow(); + row.Write(string.Create(CultureInfo.InvariantCulture, $"item-{r:D7}")); + row.Write(r); + row.Write(r * 0.5); + } + sheet.End(); + } + } + } +} From 9a318de75e46d1902c2ffc3bbf1a53403399c2a0 Mon Sep 17 00:00:00 2001 From: GabrielMarquezMatte Date: Mon, 26 Oct 2026 17:37:16 -0300 Subject: [PATCH 16/20] perf(writer): stream the shared-string part instead of building it whole 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. --- .../Writer/Internal/SharedStringTable.cs | 106 +++++++++++++----- .../Writer/Xlsb/XlsbWorkbookWriter.cs | 14 ++- .../Writer/Xlsx/XlsxWorkbookWriter.cs | 6 +- .../Writer/Zip/ZipWorkbookPackage.cs | 15 +++ .../Write/SharedStringWriteBenchmark.cs | 61 ++++++++++ .../Writer/Parallel/WorkerStringCacheTests.cs | 5 +- .../Writer/WriterSharedStringTests.cs | 65 +++++++++++ 7 files changed, 234 insertions(+), 38 deletions(-) create mode 100644 tests/ExcelReader.Benchmarks/Write/SharedStringWriteBenchmark.cs diff --git a/src/ExcelReader.Core/Writer/Internal/SharedStringTable.cs b/src/ExcelReader.Core/Writer/Internal/SharedStringTable.cs index 0aa0f16a..c790aee7 100644 --- a/src/ExcelReader.Core/Writer/Internal/SharedStringTable.cs +++ b/src/ExcelReader.Core/Writer/Internal/SharedStringTable.cs @@ -89,49 +89,95 @@ internal void AddReferences(int count) Interlocked.Add(ref _count, count); } - internal BiffBuffer ToXlsxBytes() + // The part is written to its entry in pieces of about this size, not built whole first. + internal const int FlushBytes = 64 * 1024; + + internal void WriteTo(Stream entry, bool binary) + { + string[] values = Values(); + using BiffBuffer buffer = new(FlushBytes + 4096); + using BiffBuffer payload = new(128); + int next = WriteStart(buffer, payload, binary); + while (next <= values.Length) + { + next = Fill(buffer, payload, values, next, binary); + entry.Write(buffer.Span); + buffer.Reset(); + } + } + + internal async ValueTask WriteToAsync(Stream entry, bool binary, CancellationToken ct) { string[] values = Values(); - BiffBuffer xml = new(Math.Max(256, values.Length * 32)); - xml.WriteUtf8( + using BiffBuffer buffer = new(FlushBytes + 4096); + using BiffBuffer payload = new(128); + int next = WriteStart(buffer, payload, binary); + while (next <= values.Length) + { + next = Fill(buffer, payload, values, next, binary); + await entry.WriteAsync(buffer.Memory, ct).ConfigureAwait(false); + buffer.Reset(); + } + } + + private int WriteStart(BiffBuffer buffer, BiffBuffer payload, bool binary) + { + if (binary) + { + payload.WriteU32((uint)Count); + payload.WriteU32((uint)UniqueCount); + Biff12RecordWriter.WriteRecord(buffer, Brt.BeginSst, payload.Span); + return 0; + } + buffer.WriteUtf8( "" + $""u8); - foreach (string value in values) - { - xml.Write("'); - CellFormatter.WriteEscaped(xml, value); - xml.Write(""u8); + buffer.WriteUtf8(Count.ToString(CultureInfo.InvariantCulture)); + buffer.Write("\" uniqueCount=\""u8); + buffer.WriteUtf8(UniqueCount.ToString(CultureInfo.InvariantCulture)); + buffer.Write("\">"u8); + return 0; + } + + // Appends items from next until the buffer reaches FlushBytes, and the closing record after the + // last one; returns where to resume, past values.Length once the part is complete. + private static int Fill(BiffBuffer buffer, BiffBuffer payload, string[] values, int next, bool binary) + { + while (next < values.Length && buffer.Length < FlushBytes) + { + WriteItem(buffer, payload, values[next++], binary); } - xml.Write(""u8); - return xml; + if (next < values.Length) + { + return next; + } + if (binary) + { + Biff12RecordWriter.WriteRecord(buffer, Brt.EndSst); + return next + 1; + } + buffer.Write(""u8); + return next + 1; } - internal BiffBuffer ToXlsbBytes() + private static void WriteItem(BiffBuffer buffer, BiffBuffer payload, string value, bool binary) { - string[] values = Values(); - var data = new BiffBuffer(Math.Max(128, values.Length * 24)); - using var payload = new BiffBuffer(128); - payload.WriteU32((uint)Count); - payload.WriteU32((uint)UniqueCount); - Biff12RecordWriter.WriteRecord(data, Brt.BeginSst, payload.Span); - foreach (string value in values) + if (binary) { payload.Reset(); payload.WriteByte(0); Biff12RecordWriter.WriteWideString(payload, value); - Biff12RecordWriter.WriteRecord(data, Brt.SSTItem, payload.Span); + Biff12RecordWriter.WriteRecord(buffer, Brt.SSTItem, payload.Span); + return; + } + buffer.Write("'); + CellFormatter.WriteEscaped(buffer, value); + buffer.Write(""u8); } private string[] Values() diff --git a/src/ExcelReader.Core/Writer/Xlsb/XlsbWorkbookWriter.cs b/src/ExcelReader.Core/Writer/Xlsb/XlsbWorkbookWriter.cs index 1d4c7a7d..a8472db4 100644 --- a/src/ExcelReader.Core/Writer/Xlsb/XlsbWorkbookWriter.cs +++ b/src/ExcelReader.Core/Writer/Xlsb/XlsbWorkbookWriter.cs @@ -319,13 +319,23 @@ private static void WriteFmt(BiffBuffer data, BiffBuffer payload, int numFmtId, private void WriteSharedStrings() { - using var data = _sharedStrings is null ? EmptySharedStrings() : _sharedStrings.ToXlsbBytes(); + if (_sharedStrings is not null) + { + _package.WriteSharedStrings("xl/sharedStrings.bin", _sharedStrings, binary: true); + return; + } + using BiffBuffer data = EmptySharedStrings(); _package.WriteEntry("xl/sharedStrings.bin", data.Span); } private async ValueTask WriteSharedStringsAsync(CancellationToken ct) { - using var data = _sharedStrings is null ? EmptySharedStrings() : _sharedStrings.ToXlsbBytes(); + if (_sharedStrings is not null) + { + await _package.WriteSharedStringsAsync("xl/sharedStrings.bin", _sharedStrings, binary: true, ct).ConfigureAwait(false); + return; + } + using BiffBuffer data = EmptySharedStrings(); await _package.WriteEntryAsync("xl/sharedStrings.bin", data.Memory, ct).ConfigureAwait(false); } diff --git a/src/ExcelReader.Core/Writer/Xlsx/XlsxWorkbookWriter.cs b/src/ExcelReader.Core/Writer/Xlsx/XlsxWorkbookWriter.cs index 1b791561..8c186fa3 100644 --- a/src/ExcelReader.Core/Writer/Xlsx/XlsxWorkbookWriter.cs +++ b/src/ExcelReader.Core/Writer/Xlsx/XlsxWorkbookWriter.cs @@ -162,8 +162,7 @@ void IZipWorkbookParts.WriteParts() _package.WriteEntry("xl/styles.xml", BuildStylesXml()); if (_sharedStrings is not null) { - using var bytes = _sharedStrings.ToXlsxBytes(); - _package.WriteEntry("xl/sharedStrings.xml", bytes.Memory.Span); + _package.WriteSharedStrings("xl/sharedStrings.xml", _sharedStrings, binary: false); } _package.WriteEntry("xl/workbook.xml", BuildWorkbookXml()); _package.WriteEntry("xl/_rels/workbook.xml.rels", BuildWorkbookRelsXml()); @@ -184,8 +183,7 @@ async ValueTask IZipWorkbookParts.WritePartsAsync(CancellationToken ct) await _package.WriteEntryAsync("xl/styles.xml", BuildStylesXml(), ct).ConfigureAwait(false); if (_sharedStrings is not null) { - using var bytes = _sharedStrings.ToXlsxBytes(); - await _package.WriteEntryAsync("xl/sharedStrings.xml", bytes.Memory, ct).ConfigureAwait(false); + await _package.WriteSharedStringsAsync("xl/sharedStrings.xml", _sharedStrings, binary: false, ct).ConfigureAwait(false); } await _package.WriteEntryAsync("xl/workbook.xml", BuildWorkbookXml(), ct).ConfigureAwait(false); await _package.WriteEntryAsync("xl/_rels/workbook.xml.rels", BuildWorkbookRelsXml(), ct).ConfigureAwait(false); diff --git a/src/ExcelReader.Core/Writer/Zip/ZipWorkbookPackage.cs b/src/ExcelReader.Core/Writer/Zip/ZipWorkbookPackage.cs index 8709241d..6370daab 100644 --- a/src/ExcelReader.Core/Writer/Zip/ZipWorkbookPackage.cs +++ b/src/ExcelReader.Core/Writer/Zip/ZipWorkbookPackage.cs @@ -269,5 +269,20 @@ internal ValueTask WriteEntryAsync(string entryName, ReadOnlyMemory conten { return ZipEntryWriter.WriteBytesAsync(Zip, entryName, content, _compression, ct); } + + internal void WriteSharedStrings(string entryName, SharedStringTable strings, bool binary) + { + using Stream entry = Zip.OpenEntry(entryName, _compression); + strings.WriteTo(entry, binary); + } + + internal async ValueTask WriteSharedStringsAsync(string entryName, SharedStringTable strings, bool binary, CancellationToken ct) + { + Stream entry = Zip.OpenEntry(entryName, _compression); + await using (entry.ConfigureAwait(false)) + { + await strings.WriteToAsync(entry, binary, ct).ConfigureAwait(false); + } + } } } diff --git a/tests/ExcelReader.Benchmarks/Write/SharedStringWriteBenchmark.cs b/tests/ExcelReader.Benchmarks/Write/SharedStringWriteBenchmark.cs new file mode 100644 index 00000000..fd67897e --- /dev/null +++ b/tests/ExcelReader.Benchmarks/Write/SharedStringWriteBenchmark.cs @@ -0,0 +1,61 @@ +using System.Globalization; +using System.IO; +using System.Linq; +using BenchmarkDotNet.Attributes; +using ExcelReader.Core.Writer.Xlsb; +using ExcelReader.Core.Writer.Xlsx; + +namespace ExcelReader.Benchmarks +{ + // Every text distinct, so the shared-string part is as large as the sheet that references it. + [MemoryDiagnoser] + public class SharedStringWriteBenchmark + { + [Params(100_000)] + public int Texts { get; set; } + + private string[] _texts = []; + + [GlobalSetup] + public void Setup() + { + _texts = [.. Enumerable.Range(0, Texts).Select(static i => i.ToString("D8", CultureInfo.InvariantCulture) + new string('x', 120))]; + } + + [Benchmark] + public long Xlsx() + { + using MemoryStream ms = new(32 * 1024 * 1024); + using (XlsxWorkbookWriter workbook = XlsxWorkbookWriter.Create(ms, leaveOpen: true, options: new XlsxWriterOptions { UseSharedStrings = true })) + { + using XlsxSheetWriter sheet = workbook.AddSheet("S"); + foreach (string text in _texts) + { + using XlsxRowWriter row = sheet.StartRow(); + row.Write(text); + } + sheet.End(); + workbook.End(); + } + return ms.Length; + } + + [Benchmark] + public long Xlsb() + { + using MemoryStream ms = new(32 * 1024 * 1024); + using (XlsbWorkbookWriter workbook = XlsbWorkbookWriter.Create(ms, leaveOpen: true, options: new XlsbWriterOptions { UseSharedStrings = true })) + { + using XlsbSheetWriter sheet = workbook.AddSheet("S"); + foreach (string text in _texts) + { + using XlsbRowWriter row = sheet.StartRow(); + row.Write(text); + } + sheet.End(); + workbook.End(); + } + return ms.Length; + } + } +} diff --git a/tests/ExcelReader.Tests/Writer/Parallel/WorkerStringCacheTests.cs b/tests/ExcelReader.Tests/Writer/Parallel/WorkerStringCacheTests.cs index d5502fc6..8aacce49 100644 --- a/tests/ExcelReader.Tests/Writer/Parallel/WorkerStringCacheTests.cs +++ b/tests/ExcelReader.Tests/Writer/Parallel/WorkerStringCacheTests.cs @@ -57,8 +57,9 @@ public void A_Text_Added_By_One_Path_Should_Keep_Its_Index_On_The_Other() Assert.Equal([0, 0, 1, 1, 2], [serialA, concurrentA, concurrentB, serialB, serialC]); Assert.Equal(3, table.UniqueCount); - using BiffBuffer xml = table.ToXlsxBytes(); - Assert.Contains("abc", System.Text.Encoding.UTF8.GetString(xml.Span), StringComparison.Ordinal); + using MemoryStream xml = new(); + table.WriteTo(xml, binary: false); + Assert.Contains("abc", System.Text.Encoding.UTF8.GetString(xml.ToArray()), StringComparison.Ordinal); } [Fact] diff --git a/tests/ExcelReader.Tests/Writer/WriterSharedStringTests.cs b/tests/ExcelReader.Tests/Writer/WriterSharedStringTests.cs index f371fc9e..a90ae7ba 100644 --- a/tests/ExcelReader.Tests/Writer/WriterSharedStringTests.cs +++ b/tests/ExcelReader.Tests/Writer/WriterSharedStringTests.cs @@ -210,6 +210,71 @@ public async Task XlsbWriterOptInSharedStringsDeduplicatesAndRoundTrips() Assert.False(rows.MoveNext()); } + private static string ManyText(int i) + { + string lead = i % 3 == 0 ? " " : ""; + string trail = i % 5 == 0 ? "\t" : ""; + return string.Create(System.Globalization.CultureInfo.InvariantCulture, $"{lead}value {i} & \"q\" ✓{trail}"); + } + + private static void WriteManyTexts(Core.Writer.IWorkbookWriter workbook, int texts) + where TSheet : Core.Writer.ISheetWriter + where TRow : Core.Writer.IRowWriter + { + using TSheet sheet = workbook.AddSheet("S"); + for (int i = 0; i < texts; i++) + { + using TRow row = sheet.StartRow(); + row.Write(ManyText(i)); + row.Write(ManyText(i / 2)); + } + sheet.End(); + } + + [Theory] + [InlineData(false, false)] + [InlineData(false, true)] + [InlineData(true, false)] + [InlineData(true, true)] + public async Task SharedStringsSpanningManyFlushesRoundTrip(bool xlsb, bool endAsync) + { + const int Texts = 30_000; + using MemoryStream ms = new(); + if (xlsb) + { + await using XlsbWorkbookWriter writer = XlsbWorkbookWriter.Create(ms, leaveOpen: true, options: new XlsbWriterOptions { UseSharedStrings = true }); + WriteManyTexts(writer, Texts); + await EndAsync(writer, endAsync); + } + if (!xlsb) + { + await using XlsxWorkbookWriter writer = XlsxWorkbookWriter.Create(ms, leaveOpen: true, options: new XlsxWriterOptions { UseSharedStrings = true }); + WriteManyTexts(writer, Texts); + await EndAsync(writer, endAsync); + } + ms.Position = 0; + + using IExcelWorkbook reader = Excel.Open(ms, ExcelFileFormat.Unknown); + using IExcelRowEnumerator rows = reader.FirstSheet.GetEnumerator(); + for (int i = 0; i < Texts; i++) + { + Assert.True(rows.MoveNext()); + Assert.Equal(ManyText(i), rows.Current[0].GetString()); + Assert.Equal(ManyText(i / 2), rows.Current[1].GetString()); + } + Assert.False(rows.MoveNext()); + } + + private static ValueTask EndAsync(Core.Writer.IWorkbookWriter writer, bool endAsync) + { + if (endAsync) + { + return writer.EndAsync(TestContext.Current.CancellationToken); + } + writer.End(); + return ValueTask.CompletedTask; + } + private static async Task WriteXlsxAsync(bool useSharedStrings, Func build) { var ms = new MemoryStream(); From 5ab42d1d937e37d3674be7fb59d77ff1035aad5d Mon Sep 17 00:00:00 2001 From: GabrielMarquezMatte Date: Mon, 26 Oct 2026 17:37:16 -0300 Subject: [PATCH 17/20] perf(arrow): append text cells to the string builder without a scratch 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. --- src/ExcelReader.Arrow/ColumnAppender.cs | 8 ++--- .../Interop/ArrowConversionBenchmark.cs | 14 ++++++++ .../Arrow/ArrowConversionTests.cs | 35 +++++++++++++++++++ 3 files changed, 53 insertions(+), 4 deletions(-) diff --git a/src/ExcelReader.Arrow/ColumnAppender.cs b/src/ExcelReader.Arrow/ColumnAppender.cs index 7b5e9376..3ab10c4b 100644 --- a/src/ExcelReader.Arrow/ColumnAppender.cs +++ b/src/ExcelReader.Arrow/ColumnAppender.cs @@ -48,7 +48,7 @@ private sealed class StringColumnAppender : ColumnAppender private const int NumberFormatMaxBytes = 32; private readonly StringArray.Builder _builder = new(); - private byte[] _scratch = []; + private readonly byte[] _scratch = new byte[NumberFormatMaxBytes]; internal StringColumnAppender(ExcelColumnSchema schema) : base(schema) { @@ -69,10 +69,10 @@ internal override void Append(in Cell cell, bool isDate1904) _builder.AppendNull(); return; } - int capacity = Math.Max(cell.Value.Length, NumberFormatMaxBytes); - if (_scratch.Length < capacity) + if (!cell.Value.IsEmpty) { - _scratch = new byte[capacity]; + _builder.Append(cell.Value); + return; } if (!cell.TryFormat(_scratch, out int written)) { diff --git a/tests/ExcelReader.Benchmarks/Interop/ArrowConversionBenchmark.cs b/tests/ExcelReader.Benchmarks/Interop/ArrowConversionBenchmark.cs index f1815589..40d25f4e 100644 --- a/tests/ExcelReader.Benchmarks/Interop/ArrowConversionBenchmark.cs +++ b/tests/ExcelReader.Benchmarks/Interop/ArrowConversionBenchmark.cs @@ -1,3 +1,6 @@ +using System.IO; +using System.Linq; +using System.Threading.Tasks; using Apache.Arrow; using BenchmarkDotNet.Attributes; using ExcelReader.Arrow; @@ -13,6 +16,7 @@ public class ArrowConversionBenchmark public int Rows { get; set; } private byte[] _csvAllString = []; + private byte[] _csvLongText = []; private byte[] _csvTyped = []; private byte[] _xlsbTyped = []; @@ -40,6 +44,7 @@ private static ExcelColumnSchema[] BuildAllStringSchema(int columns) public async Task SetupAsync() { _csvAllString = CsvGenerator.BuildWide(Rows, AllStringSchema.Length); + _csvLongText = System.Text.Encoding.UTF8.GetBytes(string.Concat(Enumerable.Repeat(new string('x', 64 * 1024) + "\n", 500))); _csvTyped = CsvGenerator.BuildTyped(Rows); _xlsbTyped = await WorkbookGenerator.BuildTypedXlsbAsync(Rows); } @@ -52,6 +57,15 @@ public long CsvAllString() return batch.Length; } + // A few long cells instead of many short ones: one 64 KiB text per row. + [Benchmark] + public long CsvLongText() + { + using var reader = Excel.FromCsv(_csvLongText); + using RecordBatch batch = reader.FirstSheet.ToArrowRecordBatch(new ArrowReadOptions { Schema = AllStringSchema[..1], HeaderRow = 0 }); + return batch.Length; + } + [Benchmark] public long CsvTyped() { diff --git a/tests/ExcelReader.Tests/Arrow/ArrowConversionTests.cs b/tests/ExcelReader.Tests/Arrow/ArrowConversionTests.cs index 068b20ba..57e32358 100644 --- a/tests/ExcelReader.Tests/Arrow/ArrowConversionTests.cs +++ b/tests/ExcelReader.Tests/Arrow/ArrowConversionTests.cs @@ -124,6 +124,41 @@ public void ToArrowRecordBatch_Should_Convert_Float64_Column() Assert.Equal(2.25, price.GetValue(1)); } + [Theory] + [InlineData(true)] + [InlineData(false)] + public void ToArrowRecordBatch_Should_Convert_Text_Numbers_Empties_And_Long_Cells_To_A_String_Column(bool nullable) + { + string longText = new('é', 32_000); + using MemoryStream ms = new(); + using (Core.Writer.Xlsb.XlsbWorkbookWriter workbook = Core.Writer.Xlsb.XlsbWorkbookWriter.Create(ms, leaveOpen: true)) + { + using Core.Writer.Xlsb.XlsbSheetWriter sheet = workbook.AddSheet("S"); + foreach (Action write in (Action[]) + [row => row.Write(1.5), row => row.Write("short"), row => row.Skip(), row => row.Write(longText), row => row.Write(42)]) + { + using Core.Writer.Xlsb.XlsbRowWriter row = sheet.StartRow(); + row.Write("key"); + write(row); + } + sheet.End(); + workbook.End(); + } + ms.Position = 0; + using var reader = Excel.FromXlsb(ms); + ExcelColumnSchema[] schema = [new() { Index = 1, Name = "text", Type = ExcelColumnType.StringColumn, IsNullable = nullable }]; + + using RecordBatch batch = reader.FirstSheet.ToArrowRecordBatch(new ArrowReadOptions { Schema = schema, HeaderRow = 0 }); + + var text = Assert.IsType(batch.Column(0)); + Assert.Equal("1.5", text.GetString(0)); + Assert.Equal("short", text.GetString(1)); + Assert.Equal(nullable, text.IsNull(2)); + Assert.Equal(nullable ? null : "", text.GetString(2)); + Assert.Equal(longText, text.GetString(3)); + Assert.Equal("42", text.GetString(4)); + } + [Fact] public void ToArrowRecordBatch_Should_Convert_Bool_Column() { From dca2bb1603c32999866ff1326c6376411cd17e16 Mon Sep 17 00:00:00 2001 From: GabrielMarquezMatte Date: Mon, 26 Oct 2026 18:06:03 -0300 Subject: [PATCH 18/20] fix(benchmark): update filters for write and native benchmark groups to include shared string and wide header benchmarks --- tests/ExcelReader.Benchmarks/benchmark-groups.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/ExcelReader.Benchmarks/benchmark-groups.json b/tests/ExcelReader.Benchmarks/benchmark-groups.json index fc91cad6..8e021245 100644 --- a/tests/ExcelReader.Benchmarks/benchmark-groups.json +++ b/tests/ExcelReader.Benchmarks/benchmark-groups.json @@ -19,7 +19,7 @@ }, { "name": "write", - "filter": "ExcelReader.Benchmarks.WriteBenchmark.* ExcelReader.Benchmarks.RecordWriteBenchmark.* ExcelReader.Benchmarks.XlsWriteBenchmark.* ExcelReader.Benchmarks.CsvWriteBenchmark.* ExcelReader.Benchmarks.WritePathBenchmark.* ExcelReader.Benchmarks.ParallelRecordWriteBenchmark.*", + "filter": "ExcelReader.Benchmarks.WriteBenchmark.* ExcelReader.Benchmarks.RecordWriteBenchmark.* ExcelReader.Benchmarks.XlsWriteBenchmark.* ExcelReader.Benchmarks.CsvWriteBenchmark.* ExcelReader.Benchmarks.WritePathBenchmark.* ExcelReader.Benchmarks.ParallelRecordWriteBenchmark.* ExcelReader.Benchmarks.SharedStringWriteBenchmark.*", "pagesArgs": "--warmupCount 3 --iterationCount 10", "prArgs": "--warmupCount 1 --iterationCount 5" }, @@ -31,7 +31,7 @@ }, { "name": "native", - "filter": "ExcelReader.Benchmarks.NativeRowReadBenchmark.* ExcelReader.Benchmarks.NativeTypedParseBenchmark.* ExcelReader.Benchmarks.ChunkedParseBenchmark.* ExcelReader.Benchmarks.NativeOpenBenchmark.*", + "filter": "ExcelReader.Benchmarks.NativeRowReadBenchmark.* ExcelReader.Benchmarks.NativeTypedParseBenchmark.* ExcelReader.Benchmarks.ChunkedParseBenchmark.* ExcelReader.Benchmarks.NativeOpenBenchmark.* ExcelReader.Benchmarks.NativeWideHeaderBenchmark.*", "pagesArgs": "--warmupCount 3 --iterationCount 10", "prArgs": "--warmupCount 1 --iterationCount 5" }, From e17ada8f8ddcaa7073ccbbb34666066ce826168d Mon Sep 17 00:00:00 2001 From: GabrielMarquezMatte Date: Mon, 26 Oct 2026 18:29:28 -0300 Subject: [PATCH 19/20] fix(docs): update performance benchmarks in README and improve benchmark 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 --- python/README.md | 18 ++-- python/benchmarks/bench_write.py | 4 +- python/src/excelreader/writer.py | 139 +++++++++++++------------- python/tests/test_arrow_conversion.py | 123 +++++++++++++++++++++++ 4 files changed, 205 insertions(+), 79 deletions(-) create mode 100644 python/tests/test_arrow_conversion.py diff --git a/python/README.md b/python/README.md index 601718f6..a4237229 100644 --- a/python/README.md +++ b/python/README.md @@ -627,17 +627,17 @@ threads](#reading-a-large-sheet-on-several-threads). | `write_workbook()` → xlsb | 51.7 ms | 4.5 MB | | `write_workbook()` → xls | 59.2 ms | 13.4 MB | | `write_workbook()` → xlsx | 68.5 ms | 5.1 MB | -| `write_workbook(polars_df)` → xlsx | 456.9 ms | 5.1 MB | -| `write_workbook(pandas_df)` → xlsx | 465.5 ms | 5.1 MB | -| `polars.DataFrame.write_excel()` | 4,663.7 ms | 5.6 MB | -| `pandas.DataFrame.to_excel()` | 7,173.6 ms | 5.5 MB | +| `write_workbook(polars_df)` → xlsx | 76.6 ms | 5.1 MB | +| `write_workbook(pandas_df)` → xlsx | 82.5 ms | 5.1 MB | +| `polars.DataFrame.write_excel()` | 4,714.9 ms | 5.6 MB | +| `pandas.DataFrame.to_excel()` | 7,371.4 ms | 5.5 MB | The two DataFrame comparisons are matched work — same DataFrame in, xlsx out both times: -`write_workbook(polars_df)` is ~10.2x faster than polars' own `write_excel()`, and -`write_workbook(pandas_df)` ~15.4x faster than `to_excel()`. Both of ours pay a conversion the raw path does not: the DataFrame goes -through Arrow and then a Python list before reaching the native columns, which is most of the gap -between the ~460 ms rows and the 68.5 ms one. Handing `write_workbook()` buffers that are already -columnar — what `parse()` returns — skips all of it. +`write_workbook(polars_df)` is ~62x faster than polars' own `write_excel()`, and +`write_workbook(pandas_df)` ~89x faster than `to_excel()`. Both of ours pay a conversion the raw path does not: the DataFrame goes +through Arrow, and each column is cast to the layout the native side reads and copied out of Arrow's +buffers, which is the gap between those rows and the 68.5 ms one. Handing `write_workbook()` buffers +that are already columnar — what `parse()` returns — skips it. `write_workbook(xlsx)` at 68.5 ms lands within ~4 ms of the C++ binding's `xl::write_columns` on the same 14 columns (64.2 ms), which is the expected result: both are thin wrappers diff --git a/python/benchmarks/bench_write.py b/python/benchmarks/bench_write.py index 939bf8cf..ef2b29bd 100644 --- a/python/benchmarks/bench_write.py +++ b/python/benchmarks/bench_write.py @@ -82,7 +82,7 @@ def benchmark_pandas(args: argparse.Namespace, tmp: Path): with excelreader.open_workbook(args.path, format="xlsb") as workbook: pandas_df = workbook.sheets[0].to_pandas(_FIXTURE_SCHEMA) - print("write_workbook(pandas) [pandas.DataFrame -> Arrow -> pylist -> native columns -> xlsx]:") + print("write_workbook(pandas) [pandas.DataFrame -> Arrow -> native columns -> xlsx]:") _time_and_assert( " write_workbook(pandas)", lambda p: excelreader.write_workbook(p, pandas_df), @@ -115,7 +115,7 @@ def benchmark_polars(args: argparse.Namespace, tmp: Path): schema = workbook.sheets[0].infer_schema(sample_size=10) polars_df = workbook.sheets[0].to_polars(schema) - print("write_workbook(polars) [polars.DataFrame -> Arrow -> pylist -> native columns -> xlsx]:") + print("write_workbook(polars) [polars.DataFrame -> Arrow -> native columns -> xlsx]:") _time_and_assert( " write_workbook(polars)", lambda p: excelreader.write_workbook(p, polars_df), diff --git a/python/src/excelreader/writer.py b/python/src/excelreader/writer.py index fc7c1729..5991d56d 100644 --- a/python/src/excelreader/writer.py +++ b/python/src/excelreader/writer.py @@ -10,7 +10,6 @@ from __future__ import annotations import ctypes -import datetime import io import os import sys @@ -288,18 +287,17 @@ def _write_memory( "timestamp[us]": ColumnType.TIMESTAMP, } -_ARRAY_TYPECODES = { - ColumnType.I64: "q", - ColumnType.TIME: "q", - ColumnType.TIMESTAMP: "q", - ColumnType.F64: "d", - ColumnType.BOOL: "b", - ColumnType.DATE: "i", +# The Arrow type each column is cast to before its values are read, and the array typecode of the +# same layout: date32 is days, time64[us] and timestamp[us] are microseconds, as the native side takes them. +_STORAGE = { + ColumnType.I64: ("int64", "q"), + ColumnType.TIME: ("int64", "q"), + ColumnType.TIMESTAMP: ("int64", "q"), + ColumnType.F64: ("float64", "d"), + ColumnType.BOOL: ("int8", "b"), + ColumnType.DATE: ("int32", "i"), } -_EPOCH_DATE = datetime.date(1970, 1, 1) -_EPOCH_DATETIME = datetime.datetime(1970, 1, 1, tzinfo=datetime.timezone.utc) - def _arrow_column_type(field: Any) -> ColumnType: key = str(field.type) @@ -312,68 +310,73 @@ def _arrow_column_type(field: Any) -> ColumnType: ) from None -def _column_from_pylist(values: list[Any], column_type: ColumnType, row_count: int) -> tuple[Any, bytes | None]: - """Builds one column's native buffer layout from a plain Python list, as `to_pylist()` returns it. - - Returns `(column, validity)`: `column` is a `StringColumn` for `ColumnType.STRING` (UTF-8 data plus - an `array("i")` of `row_count + 1` offsets), or an `array.array` of the typecode matching - `column_type` otherwise. `validity` is an Arrow-style LSB-first bitmap — `(row_count + 7) // 8` - bytes, bit `r` set when row `r` is valid — or `None` when no value in `values` was ever `None`, - matching `TypedTable.validity`'s "None means no nulls" convention. A `None` value contributes a - zero/empty placeholder to `column` and a cleared bit to the bitmap. +def _slots(array_: Any, width: int, count: int, buffer_index: int = 1) -> memoryview: + """The bytes of `count` fixed-width slots of an Arrow array's buffer, from its slice offset on.""" + buffer = array_.buffers()[buffer_index] + start = array_.offset * width + return memoryview(buffer)[start : start + count * width] + + +def _validity_bitmap(column: Any) -> bytes: + """An LSB-first bitmap of `(len + 7) // 8` bytes, bit `r` set when row `r` is valid.""" + import pyarrow.compute + + valid = pyarrow.compute.is_valid(column) + row_count = len(column) + if valid.offset % 8: + bits = bytearray((row_count + 7) // 8) + for row, flag in enumerate(valid.to_pylist()): + bits[row // 8] |= flag << (row % 8) + return bytes(bits) + start = valid.offset // 8 + bits = bytearray(memoryview(valid.buffers()[1])[start : start + (row_count + 7) // 8]) + if row_count % 8: + bits[-1] &= (1 << (row_count % 8)) - 1 + return bytes(bits) + + +def _string_column(strings: Any) -> StringColumn: + """UTF-8 data plus `len + 1` offsets starting at 0, from an Arrow `string` array without nulls.""" + row_count = len(strings) + if row_count == 0: + return StringColumn(array("i", [0]), b"") + offsets = array("i") + offsets.frombytes(_slots(strings, 4, row_count + 1)) + first, last = offsets[0], offsets[-1] + data_buffer = strings.buffers()[2] + data = b"" if data_buffer is None else bytes(memoryview(data_buffer)[first:last]) + if first: + offsets = array("i", [offset - first for offset in offsets]) + return StringColumn(offsets, data) + + +def _column_from_arrow(column: Any, column_type: ColumnType) -> tuple[Any, bytes | None]: + """Builds one column's native buffer layout and its validity bitmap, `None` when nothing is null. + + Arrow first casts the column to the one layout this reads, and replaces nulls with an empty or + zero placeholder, so the producer's choices of string variant, null slots or bit packing never + reach the buffer reads. """ - validity = bytearray((row_count + 7) // 8) - has_null = False + import pyarrow + import pyarrow.compute + validity = _validity_bitmap(column) if column.null_count else None if column_type is ColumnType.STRING: - offsets = array("i", [0]) - chunks: list[bytes] = [] - total = 0 - for row, value in enumerate(values): - if value is None: - has_null = True - else: - validity[row // 8] |= 1 << (row % 8) - encoded = value.encode("utf-8") - chunks.append(encoded) - total += len(encoded) - offsets.append(total) - string_column: Any = StringColumn(offsets, b"".join(chunks)) - return string_column, (bytes(validity) if has_null else None) - - data_array = array(_ARRAY_TYPECODES[column_type]) - for row, value in enumerate(values): - if value is None: - has_null = True - data_array.append(0) - continue - validity[row // 8] |= 1 << (row % 8) - if column_type is ColumnType.DATE: - data_array.append((value - _EPOCH_DATE).days) - elif column_type is ColumnType.TIME: - data_array.append( - value.hour * 3_600_000_000 - + value.minute * 60_000_000 - + value.second * 1_000_000 - + value.microsecond - ) - elif column_type is ColumnType.TIMESTAMP: - delta = value.replace(tzinfo=datetime.timezone.utc) - _EPOCH_DATETIME - data_array.append(delta.days * 86_400_000_000 + delta.seconds * 1_000_000 + delta.microseconds) - elif column_type is ColumnType.BOOL: - data_array.append(1 if value else 0) - else: - data_array.append(value) - - return data_array, (bytes(validity) if has_null else None) + strings = column.cast(pyarrow.string()) + if strings.null_count: + strings = pyarrow.compute.fill_null(strings, "") + return _string_column(strings), validity + storage, typecode = _STORAGE[column_type] + values = column.cast(storage) + if values.null_count: + values = pyarrow.compute.fill_null(values, 0) + data = array(typecode) + if len(values): + data.frombytes(_slots(values, data.itemsize, len(values))) + return data, validity def _table_from_arrow(batch: Any) -> tuple[TypedTable, list[ColumnType]]: - """Converts through Python lists rather than borrowing Arrow's buffers directly: Arrow's null - representation, offset conventions and chunking are the producer's choice, and reproducing all of - them faithfully is exactly the parser this package deliberately does not have. The conversion is - one pass and keeps the native side reading only layouts it produced itself. - """ import pyarrow if isinstance(batch, pyarrow.Table): @@ -384,7 +387,7 @@ def _table_from_arrow(batch: Any) -> tuple[TypedTable, list[ColumnType]]: columns: list[Any] = [] validity: list[bytes | None] = [] for index, column in enumerate(batch.columns): - built, mask = _column_from_pylist(column.to_pylist(), types[index], batch.num_rows) + built, mask = _column_from_arrow(column, types[index]) columns.append(built) validity.append(mask) diff --git a/python/tests/test_arrow_conversion.py b/python/tests/test_arrow_conversion.py new file mode 100644 index 00000000..aaa212b0 --- /dev/null +++ b/python/tests/test_arrow_conversion.py @@ -0,0 +1,123 @@ +import datetime + +import pytest + +pa = pytest.importorskip("pyarrow") + +from excelreader.types import ColumnType, StringColumn +from excelreader.writer import _table_from_arrow + +_EPOCH_DATE = datetime.date(1970, 1, 1) +_EPOCH = datetime.datetime(1970, 1, 1) + + +def _decode(column, column_type, validity, row_count): + """Reads one converted column back into Python values, None where the validity bit is clear.""" + values = [] + for row in range(row_count): + if validity is not None and not validity[row // 8] >> (row % 8) & 1: + values.append(None) + continue + if column_type is ColumnType.STRING: + values.append(column.data[column.offsets[row] : column.offsets[row + 1]].decode("utf-8")) + continue + raw = column[row] + if column_type is ColumnType.DATE: + values.append(_EPOCH_DATE + datetime.timedelta(days=raw)) + elif column_type is ColumnType.TIME: + values.append((datetime.datetime.min + datetime.timedelta(microseconds=raw)).time()) + elif column_type is ColumnType.TIMESTAMP: + values.append(_EPOCH + datetime.timedelta(microseconds=raw)) + elif column_type is ColumnType.BOOL: + values.append(bool(raw)) + else: + values.append(raw) + return values + + +def _assert_round_trips(data, expected_types): + table, types = _table_from_arrow(data) + batch = data.combine_chunks() if isinstance(data, pa.Table) else data + assert types == expected_types + assert table.names == list(batch.schema.names) + for index, column_type in enumerate(types): + column, validity = table.columns[index], table.validity[index] + expected = batch.column(index).to_pylist() + assert _decode(column, column_type, validity, table.row_count) == expected + assert (validity is None) == (None not in expected) + if validity is not None: + assert len(validity) == (table.row_count + 7) // 8 + spare = table.row_count % 8 + assert spare == 0 or validity[-1] >> spare == 0 + if column_type is ColumnType.STRING: + assert isinstance(column, StringColumn) + assert column.offsets[0] == 0 and column.offsets[-1] == len(column.data) + return table + + +_TYPED = pa.record_batch( + { + "text": pa.array(["a", None, "ção ✓", "", "last"]), + "large": pa.array(["x", "y", None, "zz", ""], pa.large_string()), + "view": pa.array(["v1", None, "v3", "v4", "v5"], pa.string_view()), + "i64": pa.array([1, None, -(2**63), 2**63 - 1, 0], pa.int64()), + "i32": pa.array([7, -7, None, 2**31 - 1, 0], pa.int32()), + "f64": pa.array([1.5, None, -0.0, 1e308, float("inf")], pa.float64()), + "f32": pa.array([0.25, 2.5, None, -1.0, 3.0], pa.float32()), + "flag": pa.array([True, False, None, True, False]), + "day": pa.array([datetime.date(2020, 1, 2), None, datetime.date(1900, 3, 1), _EPOCH_DATE, datetime.date(9999, 12, 31)]), + "clock": pa.array([datetime.time(0, 0), datetime.time(23, 59, 59, 999999), None, datetime.time(12, 30), datetime.time(1, 2, 3)]), + "stamp": pa.array( + [datetime.datetime(2024, 5, 6, 12, 30), None, datetime.datetime(1969, 12, 31, 23, 59, 59, 1), _EPOCH, datetime.datetime(2000, 1, 1)], + pa.timestamp("us"), + ), + } +) +_TYPES = [ + ColumnType.STRING, ColumnType.STRING, ColumnType.STRING, + ColumnType.I64, ColumnType.I64, ColumnType.F64, ColumnType.F64, + ColumnType.BOOL, ColumnType.DATE, ColumnType.TIME, ColumnType.TIMESTAMP, +] + + +def test_every_supported_type_with_nulls_converts_value_for_value(): + _assert_round_trips(_TYPED, _TYPES) + + +@pytest.mark.parametrize(("offset", "length"), [(1, 3), (2, 3), (3, 2), (0, 0), (5, 0)]) +def test_a_sliced_batch_converts_only_its_own_rows(offset, length): + table = _assert_round_trips(_TYPED.slice(offset, length), _TYPES) + assert table.row_count == length + + +def test_a_chunked_table_converts_as_one_batch(): + chunked = pa.Table.from_batches([_TYPED.slice(0, 2), _TYPED.slice(2, 3), _TYPED.slice(0, 0)]) + assert chunked.column(0).num_chunks > 1 + _assert_round_trips(chunked, _TYPES) + + +def test_an_empty_table_converts_to_zero_rows(): + table = _assert_round_trips(_TYPED.schema.empty_table(), _TYPES) + assert table.row_count == 0 + + +def test_columns_without_nulls_carry_no_validity_bitmap(): + batch = pa.record_batch({"n": pa.array(range(20), pa.int64()), "s": pa.array([str(i) for i in range(20)])}) + table = _assert_round_trips(batch, [ColumnType.I64, ColumnType.STRING]) + assert table.validity == [None, None] + + +def test_an_all_null_column_and_row_counts_past_a_byte_boundary_convert(): + batch = pa.record_batch( + { + "s": pa.array([None] * 13, pa.string()), + "b": pa.array([None if i % 3 == 0 else i % 2 == 0 for i in range(13)]), + "f": pa.array([None if i % 4 == 0 else i / 3 for i in range(13)]), + } + ) + _assert_round_trips(batch, [ColumnType.STRING, ColumnType.BOOL, ColumnType.F64]) + + +def test_an_unsupported_arrow_type_is_rejected(): + with pytest.raises(ValueError, match="int8"): + _table_from_arrow(pa.record_batch({"small": pa.array([1], pa.int8())})) From b88a1fa8347d7da41bc54e6ceb9f815900f2fe7e Mon Sep 17 00:00:00 2001 From: Gabriel Matte Date: Tue, 27 Oct 2026 09:51:51 -0300 Subject: [PATCH 20/20] fix(tests): enhance validity mask handling in arrow conversion tests --- .github/workflows/native-bindings.yml | 2 +- python/tests/test_arrow_conversion.py | 9 +++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/.github/workflows/native-bindings.yml b/.github/workflows/native-bindings.yml index 1052889f..ea1b7069 100644 --- a/.github/workflows/native-bindings.yml +++ b/.github/workflows/native-bindings.yml @@ -219,7 +219,7 @@ jobs: run: chmod +x python/src/excelreader/_lib/* - name: Install the Python package - run: pip install -e "python[dev]" ${{ matrix.os == 'ubuntu-latest' && 'pytest-cov' || '' }} + run: pip install -e "python[dev]" ${{ matrix.os == 'ubuntu-latest' && 'pytest-cov pyarrow pandas polars' || '' }} - name: Run the Python tests run: >- diff --git a/python/tests/test_arrow_conversion.py b/python/tests/test_arrow_conversion.py index aaa212b0..e8592eea 100644 --- a/python/tests/test_arrow_conversion.py +++ b/python/tests/test_arrow_conversion.py @@ -118,6 +118,15 @@ def test_an_all_null_column_and_row_counts_past_a_byte_boundary_convert(): _assert_round_trips(batch, [ColumnType.STRING, ColumnType.BOOL, ColumnType.F64]) +def test_a_validity_mask_off_a_byte_boundary_converts(monkeypatch): + import pyarrow.compute + + is_valid = pyarrow.compute.is_valid + monkeypatch.setattr(pyarrow.compute, "is_valid", lambda column: pa.concat_arrays([pa.array([False] * 3), is_valid(column)]).slice(3)) + batch = pa.record_batch({"n": pa.array([None if i % 3 == 0 else i for i in range(11)], pa.int64())}) + _assert_round_trips(batch, [ColumnType.I64]) + + def test_an_unsupported_arrow_type_is_rejected(): with pytest.raises(ValueError, match="int8"): _table_from_arrow(pa.record_batch({"small": pa.array([1], pa.int8())}))