From 86fb936cd9e3ce0cd48d63042e22eec0761550fd Mon Sep 17 00:00:00 2001 From: Gabriel Matte Date: Tue, 20 Oct 2026 16:06:01 -0300 Subject: [PATCH 01/34] perf(csv): read a chunk's quote state off its first quote before guessing its start Every CSV chunk was guessed to start outside quotes, so one that began inside a quoted field found the wrong record start, or none, and was read again serially in the merge. On a CSV with quoted fields that is most chunks: a 600,000-row file with every field quoted re-read 14 of 16 chunks at four threads and ran at 0.57x the sequential speed. The first quote in the chunk's first 4 KB that is not half of a "" pair now decides: one followed by a delimiter or a line end closes a field, so the chunk starts inside one. The confirmation after the parse is unchanged, so a wrong inference still costs only a re-read; that now takes a quoted field that begins with the delimiter or a line break, or that runs past 4 KB without a quote. Same file, four threads: 58.8 to 18.5 ms with every field quoted, 34.8 to 14.3 ms with text fields quoted, 30.0 to 17.8 ms with quoted newlines; unquoted CSV is unchanged. --- .../Parser/ParallelCsv/CsvBoundaryResolver.cs | 19 ++++++++++ .../Parser/ParallelCsv/CsvChunkWorker.cs | 38 ++++++++++++++----- .../ParallelCsv/ParallelCsvProcessor.cs | 2 +- src/ExcelReader.Core/Reader/SheetParallel.cs | 4 +- .../ParallelCsv/CsvBoundaryResolverTests.cs | 32 ++++++++++++++++ .../Parser/ParallelCsv/CsvChunkWorkerTests.cs | 16 ++++++-- .../ParallelCsv/ForEachCsvParallelTests.cs | 33 +++++++++++++++- 7 files changed, 127 insertions(+), 17 deletions(-) diff --git a/src/ExcelReader.Core/Parser/ParallelCsv/CsvBoundaryResolver.cs b/src/ExcelReader.Core/Parser/ParallelCsv/CsvBoundaryResolver.cs index 577f03c9..d0410eae 100644 --- a/src/ExcelReader.Core/Parser/ParallelCsv/CsvBoundaryResolver.cs +++ b/src/ExcelReader.Core/Parser/ParallelCsv/CsvBoundaryResolver.cs @@ -20,6 +20,25 @@ internal static bool StartsRecord(ReadOnlySpan pair) return pair[0] == Cr && pair[1] != Lf; } + // Skipping "" pairs, which leave the parity alone, the first quote followed by a delimiter or a line end + // closes a field, so the window started inside one. Anything else is taken as outside; a wrong guess + // still costs only a re-read, since the chunk's start is confirmed afterwards. + internal static CsvQuoteParity InferParity(ReadOnlySpan window, byte quote, byte delimiter) + { + int i = window.IndexOf(quote); + while (i >= 0 && i + 1 < window.Length) + { + byte next = window[i + 1]; + if (next != quote) + { + return next == delimiter || next == Lf || next == Cr ? CsvQuoteParity.Inside : CsvQuoteParity.Outside; + } + int after = window[(i + 2)..].IndexOf(quote); + i = after < 0 ? -1 : i + 2 + after; + } + return CsvQuoteParity.Outside; + } + internal static int FindRecordStart(ReadOnlySpan window, byte quote, CsvQuoteParity parity) { bool inside = parity == CsvQuoteParity.Inside; diff --git a/src/ExcelReader.Core/Parser/ParallelCsv/CsvChunkWorker.cs b/src/ExcelReader.Core/Parser/ParallelCsv/CsvChunkWorker.cs index 347cb8a5..48daef01 100644 --- a/src/ExcelReader.Core/Parser/ParallelCsv/CsvChunkWorker.cs +++ b/src/ExcelReader.Core/Parser/ParallelCsv/CsvChunkWorker.cs @@ -119,7 +119,7 @@ internal static ValueTask> ParseAsync( Utf8StringCache? contentCache, CancellationToken ct) { - long start = confirmedStart ?? GuessStart(source, chunk, readerOptions.Quote); + long start = confirmedStart ?? GuessStart(source, chunk, readerOptions.Quote, readerOptions.Delimiter); if (start >= source.Length) { return new ValueTask>(new CsvChunkResult(chunk.Index, models, start, long.MaxValue)); @@ -213,7 +213,7 @@ private static async ValueTask> ParseFromAsync( return (resolvedNextStart, failure, records); } - internal static long GuessStart(CsvChunkSource source, CsvChunk chunk, byte quote) + internal static long GuessStart(CsvChunkSource source, CsvChunk chunk, byte quote, byte delimiter) { long chunkLength = chunk.End - chunk.Start; if (chunkLength <= 0) @@ -221,21 +221,22 @@ internal static long GuessStart(CsvChunkSource source, CsvChunk chunk, byte quot return long.MaxValue; } - if (chunk.Start > 0 && AlreadyAtRecordStart(source, chunk.Start)) - { - return chunk.Start; - } - if (source.IsMemory) { int cap = (int)Math.Min(chunkLength, int.MaxValue - 1); - int found = CsvBoundaryResolver.FindRecordStart( - source.WindowSpan(chunk.Start, cap), quote, CsvQuoteParity.Outside); + ReadOnlySpan window = source.WindowSpan(chunk.Start, cap); + CsvQuoteParity parity = Infer(window, quote, delimiter); + if (parity == CsvQuoteParity.Outside && AlreadyAtRecordStart(source, chunk.Start)) + { + return chunk.Start; + } + int found = CsvBoundaryResolver.FindRecordStart(window, quote, parity); return found < 0 ? long.MaxValue : chunk.Start + found; } long remaining = Math.Min(chunkLength, source.Length - chunk.Start); int windowLength = (int)Math.Min(remaining, InitialBoundaryWindow); + CsvQuoteParity? inferred = null; while (true) { int rentSize = windowLength == remaining ? windowLength + 1 : windowLength; @@ -247,8 +248,16 @@ internal static long GuessStart(CsvChunkSource source, CsvChunk chunk, byte quot { return long.MaxValue; } + if (inferred is null) + { + inferred = Infer(buffer.AsSpan(0, read), quote, delimiter); + if (inferred == CsvQuoteParity.Outside && AlreadyAtRecordStart(source, chunk.Start)) + { + return chunk.Start; + } + } int found = CsvBoundaryResolver.FindRecordStart( - buffer.AsSpan(0, read), quote, CsvQuoteParity.Outside); + buffer.AsSpan(0, read), quote, inferred.Value); if (found >= 0) { return chunk.Start + found; @@ -266,8 +275,17 @@ internal static long GuessStart(CsvChunkSource source, CsvChunk chunk, byte quot } } + private static CsvQuoteParity Infer(ReadOnlySpan window, byte quote, byte delimiter) + { + return CsvBoundaryResolver.InferParity(window[..Math.Min(window.Length, InitialBoundaryWindow)], quote, delimiter); + } + private static bool AlreadyAtRecordStart(CsvChunkSource source, long start) { + if (start <= 0) + { + return false; + } Span pair = stackalloc byte[2]; return source.ReadWindow(start - 1, pair) == pair.Length && CsvBoundaryResolver.StartsRecord(pair); } diff --git a/src/ExcelReader.Core/Parser/ParallelCsv/ParallelCsvProcessor.cs b/src/ExcelReader.Core/Parser/ParallelCsv/ParallelCsvProcessor.cs index fe054d8a..1bd9ef17 100644 --- a/src/ExcelReader.Core/Parser/ParallelCsv/ParallelCsvProcessor.cs +++ b/src/ExcelReader.Core/Parser/ParallelCsv/ParallelCsvProcessor.cs @@ -153,7 +153,7 @@ private static async Task DrainPlanAsync( while (plan.TryTakeNext(out CsvChunk chunk)) { ct.ThrowIfCancellationRequested(); - long start = chunk.Index == 0 ? dataStart : CsvChunkWorker.GuessStart(source, chunk, reader.Quote); + long start = chunk.Index == 0 ? dataStart : CsvChunkWorker.GuessStart(source, chunk, reader.Quote, reader.Delimiter); outcomes[chunk.Index] = await ParseChunkAsync(source, chunk, start, reader, aggregation, ct).ConfigureAwait(false); } } diff --git a/src/ExcelReader.Core/Reader/SheetParallel.cs b/src/ExcelReader.Core/Reader/SheetParallel.cs index 0dfd53b2..6a39f2c1 100644 --- a/src/ExcelReader.Core/Reader/SheetParallel.cs +++ b/src/ExcelReader.Core/Reader/SheetParallel.cs @@ -97,7 +97,9 @@ public static IAsyncEnumerable ParseAsync( /// records, which may also be misparsed: a wrong start shifts every field boundary. An exception /// throws during a discarded pass is discarded with it. A start is only guessed /// wrongly inside a quoted field, so a CSV in which the quote character never appears delivers every - /// record exactly once. XLSB delivers every row exactly once. XLSX does too for a well-formed sheet; on + /// record exactly once. A start inside a quoted field is read off the next quote, so a wrong guess also + /// needs a quoted field that begins with the delimiter or a line break, or that runs past 4 KB without a + /// quote. XLSB delivers every row exactly once. XLSX does too for a well-formed sheet; on /// a malformed one, workers may already have called for rows that are then /// dropped, so a row can be delivered twice or be one the sequential reader would not produce. /// diff --git a/tests/ExcelReader.Tests/Parser/ParallelCsv/CsvBoundaryResolverTests.cs b/tests/ExcelReader.Tests/Parser/ParallelCsv/CsvBoundaryResolverTests.cs index f9d8824d..de92b81a 100644 --- a/tests/ExcelReader.Tests/Parser/ParallelCsv/CsvBoundaryResolverTests.cs +++ b/tests/ExcelReader.Tests/Parser/ParallelCsv/CsvBoundaryResolverTests.cs @@ -1,3 +1,4 @@ +using System.Text; using ExcelReader.Core.Parser.ParallelCsv; namespace ExcelReader.Tests.Parser.ParallelCsv @@ -5,6 +6,7 @@ namespace ExcelReader.Tests.Parser.ParallelCsv public class CsvBoundaryResolverTests { private const byte Quote = (byte)'"'; + private const byte Delimiter = (byte)','; [Fact] public void FindsTheByteAfterAPlainNewline() @@ -95,5 +97,35 @@ public void FindsABoundaryThatLandsOnTheVeryLastByte() Assert.Equal(3, start); } + + [Theory] + [InlineData("th\nree\",4\n")] + [InlineData("ree\"\n")] + [InlineData("ree\"\r\n")] + [InlineData("a\"\"b\",4\n")] + public void AQuoteThatClosesAFieldMeansTheWindowStartsInside(string window) + { + Assert.Equal(CsvQuoteParity.Inside, CsvBoundaryResolver.InferParity(Encoding.UTF8.GetBytes(window), Quote, Delimiter)); + } + + [Theory] + [InlineData("4,\"three\",5\n")] + [InlineData(",4\n\"x\ny\",5\n")] + [InlineData("4,\"\",5\n")] + [InlineData("4,\"\"\"x\",5\n")] + [InlineData("plain,4\nplain,5\n")] + [InlineData("")] + [InlineData("ab\"")] + public void AnyOtherFirstQuoteOrNoneMeansOutside(string window) + { + Assert.Equal(CsvQuoteParity.Outside, CsvBoundaryResolver.InferParity(Encoding.UTF8.GetBytes(window), Quote, Delimiter)); + } + + [Fact] + public void HonoursTheDelimiter() + { + Assert.Equal(CsvQuoteParity.Inside, CsvBoundaryResolver.InferParity("ree\";4\n"u8, Quote, (byte)';')); + Assert.Equal(CsvQuoteParity.Outside, CsvBoundaryResolver.InferParity("ree\";4\n"u8, Quote, Delimiter)); + } } } diff --git a/tests/ExcelReader.Tests/Parser/ParallelCsv/CsvChunkWorkerTests.cs b/tests/ExcelReader.Tests/Parser/ParallelCsv/CsvChunkWorkerTests.cs index 04a46369..1bc8c719 100644 --- a/tests/ExcelReader.Tests/Parser/ParallelCsv/CsvChunkWorkerTests.cs +++ b/tests/ExcelReader.Tests/Parser/ParallelCsv/CsvChunkWorkerTests.cs @@ -92,13 +92,23 @@ public async Task GuessesItsOwnStartWhenNoConfirmedStartIsGiven() } [Fact] - public async Task GuessesWrongWhenANewlineHidesInsideAQuotedField() + public async Task SkipsANewlineHiddenInsideTheQuotedFieldItStartsIn() { byte[] csv = "Name,Age\n\"a\nb\",0001\nCid,0003\n"u8.ToArray(); - CsvChunkResult result = await ParseAsync(csv, start: 10, end: 20); + CsvChunkResult result = await ParseAsync(csv, start: 10, end: 22); - Assert.Equal(12, result.ActualStart); + Assert.Equal(20, result.ActualStart); + } + + [Fact] + public async Task GuessesWrongWhenAQuotedFieldStartsWithTheDelimiter() + { + byte[] csv = "Name,Age\nAda,0001\n\",b\",0002\nCid,0003\n"u8.ToArray(); + + CsvChunkResult result = await ParseAsync(csv, start: 10, end: 30); + + Assert.NotEqual(18, result.ActualStart); } [Fact] diff --git a/tests/ExcelReader.Tests/Parser/ParallelCsv/ForEachCsvParallelTests.cs b/tests/ExcelReader.Tests/Parser/ParallelCsv/ForEachCsvParallelTests.cs index 75358416..c1938880 100644 --- a/tests/ExcelReader.Tests/Parser/ParallelCsv/ForEachCsvParallelTests.cs +++ b/tests/ExcelReader.Tests/Parser/ParallelCsv/ForEachCsvParallelTests.cs @@ -506,15 +506,44 @@ await ChunkedMapped( } } + [Theory] + [InlineData("\"x\ny\",{0}\n")] + [InlineData("\"x\",{0},\"y\"\n")] + [InlineData("\"said \"\"hi\"\"\",{0}\n")] + public async Task ForEachRecord_Chunked_QuotedFields_ReadsEveryChunkAndRecordOnce(string row) + { + const int rows = 4000; + const string header = "Name,Units\n"; + var sb = new StringBuilder(); + sb.Append(header); + for (int i = 1; i <= rows; i++) + { + sb.AppendFormat(CultureInfo.InvariantCulture, row, i); + } + byte[] csv = Encoding.UTF8.GetBytes(sb.ToString()); + var probe = new ChunkProbe(); + var calls = new ConcurrentDictionary(); + + await ChunkedRecord(csv, sale => calls.AddOrUpdate(sale.Units, 1, static (_, n) => n + 1), probe, dop: 4, chunkSize: 2048); + + int planned = CsvChunkPlan.Create(header.Length, csv.Length - header.Length, 4, 2048).Count; + Assert.True(planned > 1, $"expected the source to partition, planned {planned} chunk(s)"); + Assert.Equal(planned, probe.Chunks); + Assert.Equal(rows, calls.Count); + Assert.All(calls, pair => Assert.Equal(1, pair.Value)); + } + + // A quoted field that starts with the delimiter makes the opening quote look like a closing one, + // so a chunk that starts outside it is guessed to start inside and read twice. [Fact] - public async Task ForEachRecord_Chunked_QuotedNewlines_InvokesBodyTwiceForSomeRecords() + public async Task ForEachRecord_Chunked_AmbiguousQuotes_InvokesBodyTwiceForSomeRecords() { const int rows = 4000; var sb = new StringBuilder(); sb.Append("Name,Units\n"); for (int i = 1; i <= rows; i++) { - sb.Append(CultureInfo.InvariantCulture, $"\"x\ny\",{i}\n"); + sb.Append(CultureInfo.InvariantCulture, $"\",x\ny\",{i}\n"); } byte[] csv = Encoding.UTF8.GetBytes(sb.ToString()); var probe = new ChunkProbe(); From 911b06cc51964cfd8eb191b464ad2865de90f780 Mon Sep 17 00:00:00 2001 From: Gabriel Matte Date: Tue, 20 Oct 2026 16:40:37 -0300 Subject: [PATCH 02/34] fix(parallel): report the sequential row for a partitioned ForEachAsync or AggregateAsync failure A partitioned read binds rows without numbering them, so an ExcelParseException from ForEachAsync or AggregateAsync carried Row 0 on every format. The merges already walk the partitions in sheet order and know how many rows each read, failing one included, so they now place the failure: CSV from the records each chunk delivered, XLSX and XLSB from each chunk's row count. The XLSX recovery tail reports its failure through the same outcome as a chunk, so it is numbered the same way. ParseAsync and the parallel CSV enumerable share the renumbering helper. --- docs/guide/reading.md | 2 +- .../Parser/Internal/ProjectionRules.cs | 7 +++++ .../ParallelCsv/ParallelCsvEnumerable.cs | 11 +------ .../ParallelCsv/ParallelCsvProcessor.cs | 13 +++++++- .../SheetPipeline/SheetPipelineRunner.cs | 25 ++++++--------- .../SheetPipeline/SheetPipelineSource.cs | 2 +- .../SheetPipeline/XlsbPipelineSource.cs | 2 +- .../SheetPipeline/XlsxPipelineSource.cs | 24 ++++++++------ src/ExcelReader.Core/Reader/SheetParallel.cs | 10 +++--- .../ParallelCsv/AggregateCsvModelTests.cs | 14 ++++++--- .../Parser/SheetParallelXlsxTests.cs | 31 +++++++++++++++++++ 11 files changed, 93 insertions(+), 48 deletions(-) diff --git a/docs/guide/reading.md b/docs/guide/reading.md index c02d2186..195f2d76 100644 --- a/docs/guide/reading.md +++ b/docs/guide/reading.md @@ -125,7 +125,7 @@ These cases read on one thread with the same results and no error: A callback that writes to a database or sends messages must be idempotent on CSV, and on XLSX if the sheet may be malformed, and must also tolerate records that do not exist: deduplicating on a key does not filter those out. -[`ExcelParseException.Row`](xref:ExcelReader.Core.Parser.ExcelParseException.Row) on `ParseAsync` counts the rows emitted so far. On `ForEachAsync` and `AggregateAsync` it is the exact row when the read is sequential and `0` when it was partitioned. +[`ExcelParseException.Row`](xref:ExcelReader.Core.Parser.ExcelParseException.Row) on `ParseAsync` counts the rows emitted so far. On `ForEachAsync` and `AggregateAsync` it is the row a sequential read reports, whether or not the read was partitioned. See the [parallel sheet benchmarks](../performance/benchmarks.md#parallel-sheet-reading) and the [parallel CSV benchmarks](../performance/benchmarks.md#parallel-csv) for what this buys. On a real-data XLSX the sheet stage drops from 28 ms (prefetched, sequential) to about 17 ms, and on XLSB from 17 ms to about 13 ms; the floor is inflate, which runs on one thread. CSV has no such floor: its `AggregateAsync` at dop 4 is ~1.8x faster than the typed path while allocating ~450x less, with zero garbage collections. diff --git a/src/ExcelReader.Core/Parser/Internal/ProjectionRules.cs b/src/ExcelReader.Core/Parser/Internal/ProjectionRules.cs index 3bc6b544..3ea05966 100644 --- a/src/ExcelReader.Core/Parser/Internal/ProjectionRules.cs +++ b/src/ExcelReader.Core/Parser/Internal/ProjectionRules.cs @@ -20,5 +20,12 @@ internal static ExcelParseException MissingRequiredValue(string name, int row) { return new ExcelParseException(row, name); } + + internal static ExcelParseException AtRow(ExcelParseException original, long row) + { + return original.RawValue.Length == 0 + ? MissingRequiredValue(original.ColumnName, (int)row) + : new ExcelParseException((int)row, original.ColumnName, original.RawValue); + } } } diff --git a/src/ExcelReader.Core/Parser/ParallelCsv/ParallelCsvEnumerable.cs b/src/ExcelReader.Core/Parser/ParallelCsv/ParallelCsvEnumerable.cs index e6b6d84b..dc1d54e6 100644 --- a/src/ExcelReader.Core/Parser/ParallelCsv/ParallelCsvEnumerable.cs +++ b/src/ExcelReader.Core/Parser/ParallelCsv/ParallelCsvEnumerable.cs @@ -101,7 +101,7 @@ private long AdvanceOrThrow(CsvChunkResult result, long recordsBefore) { if (result.Failure is not null) { - throw RenumberFailure(result.Failure, _config.HeaderRow + recordsBefore + result.Records + 1); + throw ProjectionRules.AtRow(result.Failure, _config.HeaderRow + recordsBefore + result.Records + 1); } return result.ResolvedNextStart; } @@ -214,14 +214,5 @@ private static async Task WaitForChunkAsync(Task chunkReady, Task allWorke } await chunkReady.ConfigureAwait(false); } - - private static ExcelParseException RenumberFailure(ExcelParseException original, long globalRow) - { - if (original.RawValue.Length == 0) - { - return ProjectionRules.MissingRequiredValue(original.ColumnName, (int)globalRow); - } - return new ExcelParseException((int)globalRow, original.ColumnName, original.RawValue); - } } } diff --git a/src/ExcelReader.Core/Parser/ParallelCsv/ParallelCsvProcessor.cs b/src/ExcelReader.Core/Parser/ParallelCsv/ParallelCsvProcessor.cs index 1bd9ef17..4c0d2bff 100644 --- a/src/ExcelReader.Core/Parser/ParallelCsv/ParallelCsvProcessor.cs +++ b/src/ExcelReader.Core/Parser/ParallelCsv/ParallelCsvProcessor.cs @@ -1,4 +1,5 @@ using System.Runtime.ExceptionServices; +using ExcelReader.Core.Parser.Internal; using ExcelReader.Core.Reader; using ExcelReader.Core.Reader.Csv; @@ -132,6 +133,7 @@ private static async Task PartitionedAsync( TState merged = default!; long confirmedStart = dataStart; + long recordsBefore = headerRow; for (int i = 0; i < plan.Count && confirmedStart < long.MaxValue; i++) { ChunkOutcome outcome = outcomes[i]; @@ -139,9 +141,18 @@ private static async Task PartitionedAsync( { outcome = await ParseChunkAsync(source, plan[i], confirmedStart, reader, aggregation, ct).ConfigureAwait(false); } - outcome.Failure?.Throw(); + if (outcome.Failure is not null) + { + // A partitioned run parses without row numbers; the failing record is the chunk's last delivered one. + if (outcome.Failure.SourceException is ExcelParseException { Row: 0 } parse) + { + throw ProjectionRules.AtRow(parse, recordsBefore + outcome.Delivered); + } + outcome.Failure.Throw(); + } merged = i == 0 ? outcome.State : aggregation.Combine(merged, outcome.State); confirmedStart = outcome.ResolvedNextStart; + recordsBefore += outcome.Delivered; } return merged; } diff --git a/src/ExcelReader.Core/Parser/SheetPipeline/SheetPipelineRunner.cs b/src/ExcelReader.Core/Parser/SheetPipeline/SheetPipelineRunner.cs index a2262549..b53ca460 100644 --- a/src/ExcelReader.Core/Parser/SheetPipeline/SheetPipelineRunner.cs +++ b/src/ExcelReader.Core/Parser/SheetPipeline/SheetPipelineRunner.cs @@ -68,7 +68,7 @@ internal static async IAsyncEnumerable RunAsync( ChunkOutcome outcome = await pending.WaitAsync(ct).ConfigureAwait(false); if (outcome.Failure is not null) { - ThrowRenumbered(outcome.Failure, headerRow + rows); + ThrowRenumbered(outcome.Failure, headerRow + rows, outcome.Rows); } rows += outcome.Rows; yield return outcome.State; @@ -92,16 +92,12 @@ internal static async IAsyncEnumerable RunAsync( if (recover) { - TState tail = default!; - try + ChunkOutcome tail = source.ParseTail(headerRow, rows, aggregation, cancellationToken); + if (tail.Failure is not null) { - tail = source.ParseTail(headerRow, rows, aggregation, cancellationToken); + ThrowRenumbered(tail.Failure, headerRow + rows, tail.Rows); } - catch (ExcelParseException ex) - { - ThrowRenumbered(ExceptionDispatchInfo.Capture(ex), headerRow + rows); - } - yield return tail; + yield return tail.State; } } finally @@ -168,14 +164,13 @@ private static async Task WorkAsync( } } - private static void ThrowRenumbered(ExceptionDispatchInfo failure, long rowsBefore) + // A chunk numbers its rows from 1 when it parses into models; a callback or accumulator run leaves Row + // at 0, and then the chunk's count of rows read, which includes the failing one, places it. + private static void ThrowRenumbered(ExceptionDispatchInfo failure, long rowsBefore, long rowsInChunk) { - if (failure.SourceException is ExcelParseException { Row: > 0 } parse) + if (failure.SourceException is ExcelParseException parse) { - int row = (int)(rowsBefore + parse.Row); - throw parse.RawValue.Length == 0 - ? ProjectionRules.MissingRequiredValue(parse.ColumnName, row) - : new ExcelParseException(row, parse.ColumnName, parse.RawValue); + throw ProjectionRules.AtRow(parse, rowsBefore + (parse.Row > 0 ? parse.Row : rowsInChunk)); } failure.Throw(); } diff --git a/src/ExcelReader.Core/Parser/SheetPipeline/SheetPipelineSource.cs b/src/ExcelReader.Core/Parser/SheetPipeline/SheetPipelineSource.cs index bf096983..8300a1be 100644 --- a/src/ExcelReader.Core/Parser/SheetPipeline/SheetPipelineSource.cs +++ b/src/ExcelReader.Core/Parser/SheetPipeline/SheetPipelineSource.cs @@ -43,7 +43,7 @@ internal abstract ChunkOutcome ParseChunk( ReadOnlyMemory chunk, bool first, RowAggregation aggregation, CancellationToken ct); // Reads the sheet again from its start: skipRows counts data rows, after the header. - internal abstract TState ParseTail(int headerRow, long skipRows, RowAggregation aggregation, CancellationToken ct); + internal abstract ChunkOutcome ParseTail(int headerRow, long skipRows, RowAggregation aggregation, CancellationToken ct); internal static SheetPipelineSource? Create(IExcelSheet sheet) { diff --git a/src/ExcelReader.Core/Parser/SheetPipeline/XlsbPipelineSource.cs b/src/ExcelReader.Core/Parser/SheetPipeline/XlsbPipelineSource.cs index 217efc3b..444b8f97 100644 --- a/src/ExcelReader.Core/Parser/SheetPipeline/XlsbPipelineSource.cs +++ b/src/ExcelReader.Core/Parser/SheetPipeline/XlsbPipelineSource.cs @@ -82,7 +82,7 @@ internal override ChunkOutcome ParseChunk( return outcome; } - internal override TState ParseTail(int headerRow, long skipRows, RowAggregation aggregation, CancellationToken ct) + internal override ChunkOutcome ParseTail(int headerRow, long skipRows, RowAggregation aggregation, CancellationToken ct) { throw new System.Diagnostics.UnreachableException("An XLSB chunk always ends on a record boundary."); } diff --git a/src/ExcelReader.Core/Parser/SheetPipeline/XlsxPipelineSource.cs b/src/ExcelReader.Core/Parser/SheetPipeline/XlsxPipelineSource.cs index ef852c05..f6b66167 100644 --- a/src/ExcelReader.Core/Parser/SheetPipeline/XlsxPipelineSource.cs +++ b/src/ExcelReader.Core/Parser/SheetPipeline/XlsxPipelineSource.cs @@ -80,28 +80,34 @@ internal override ChunkOutcome ParseChunk( return outcome; } - internal override TState ParseTail(int headerRow, long skipRows, RowAggregation aggregation, CancellationToken ct) + internal override ChunkOutcome ParseTail(int headerRow, long skipRows, RowAggregation aggregation, CancellationToken ct) { - TState state = aggregation.Seed(); + var outcome = new ChunkOutcome(aggregation.Seed()); using XlsxWorkbook.Enumerator rows = workbook.OpenSheetWithoutPrefetch(index); for (long skipped = 0; skipped < headerRow + skipRows; skipped++) { if (!rows.MoveNext()) { - return state; + return outcome; } } RowAction accumulate = aggregation.Accumulate; - long delivered = 0; - while (rows.MoveNext()) + try { - if (++delivered % RowsPerCancellationCheck == 0) + while (rows.MoveNext()) { - ct.ThrowIfCancellationRequested(); + if (++outcome.Rows % RowsPerCancellationCheck == 0) + { + ct.ThrowIfCancellationRequested(); + } + accumulate(ref outcome.State, rows.Current); } - accumulate(ref state, rows.Current); } - return state; + catch (Exception ex) when (ex is not OperationCanceledException) + { + outcome.Failure = ExceptionDispatchInfo.Capture(ex); + } + return outcome; } } } diff --git a/src/ExcelReader.Core/Reader/SheetParallel.cs b/src/ExcelReader.Core/Reader/SheetParallel.cs index 6a39f2c1..9e53f288 100644 --- a/src/ExcelReader.Core/Reader/SheetParallel.cs +++ b/src/ExcelReader.Core/Reader/SheetParallel.cs @@ -32,7 +32,7 @@ namespace ExcelReader.Core.Reader /// /// /// : ParseAsync renumbers it from the count emitted at the - /// merge; ForEachAsync and AggregateAsync report the exact row when sequential and 0 when partitioned. + /// merge; ForEachAsync and AggregateAsync report the row a sequential read reports, partitioned or not. /// /// public static class SheetParallel @@ -147,8 +147,8 @@ public static Task ForEachAsync( /// value throws ; empty rows are skipped. /// /// - /// is exact when the sheet is read sequentially and - /// 0 when it is partitioned, since a partition does not know how many rows precede it. + /// is the row a sequential read reports, partitioned or not: the + /// partitions merge in sheet order, so the first failure in that order is the one thrown. /// /// public static Task ForEachAsync( @@ -214,8 +214,8 @@ public static Task AggregateAsync( /// [ExcelRequired] column or value throws ; empty rows are skipped. /// /// - /// is exact when the sheet is read sequentially and 0 when it is - /// partitioned, since a partition does not know how many rows precede it. + /// is the row a sequential read reports, partitioned or not: the + /// partitions merge in sheet order, so the first failure in that order is the one thrown. /// /// Otherwise carries the same contract and fallbacks as the overload. /// diff --git a/tests/ExcelReader.Tests/Parser/ParallelCsv/AggregateCsvModelTests.cs b/tests/ExcelReader.Tests/Parser/ParallelCsv/AggregateCsvModelTests.cs index 96a14747..a476e06b 100644 --- a/tests/ExcelReader.Tests/Parser/ParallelCsv/AggregateCsvModelTests.cs +++ b/tests/ExcelReader.Tests/Parser/ParallelCsv/AggregateCsvModelTests.cs @@ -271,18 +271,22 @@ public async Task SkipsEmptyRecords() } [Fact] - public async Task ReportsTheExactRowSequentiallyAndZeroWhenPartitioned() + public async Task ReportsTheSequentialRowWhenPartitioned() { - byte[] csv = "Name,Id,Note\nada,1,n\nbob,2,n\ncid,oops,n\ndan,4,n\n"u8.ToArray(); + byte[] csv = "Name,Id,Note\nada,1,\"x\ny\"\n\nbob,2,n\ncid,oops,n\ndan,4,n\neve,bad,n\n"u8.ToArray(); ExcelParser map = ExcelParser.Generated(new ExcelParserOptions { ThrowOnParseFailure = true }); using CsvReader reader = Excel.FromCsv(csv); ExcelParseException sequential = await Assert.ThrowsAsync(() => SheetParallel.AggregateAsync( reader.FirstSheet, map, new SheetParallelOptions { DegreeOfParallelism = 1, HeaderRow = 1 }, TestContext.Current.CancellationToken)); - Assert.Equal(4, sequential.Row); + Assert.Equal(5, sequential.Row); - ExcelParseException partitioned = await Assert.ThrowsAsync(() => Chunked(csv, map, dop: 4, chunkSize: 8, headerRow: 1)); - Assert.Equal(0, partitioned.Row); + for (int chunkSize = 1; chunkSize <= csv.Length; chunkSize++) + { + ExcelParseException partitioned = await Assert.ThrowsAsync(() => Chunked(csv, map, dop: 4, chunkSize, headerRow: 1)); + Assert.Equal(sequential.Row, partitioned.Row); + Assert.Equal(sequential.ColumnName, partitioned.ColumnName); + } } [Fact] diff --git a/tests/ExcelReader.Tests/Parser/SheetParallelXlsxTests.cs b/tests/ExcelReader.Tests/Parser/SheetParallelXlsxTests.cs index b4696b3b..a68904f8 100644 --- a/tests/ExcelReader.Tests/Parser/SheetParallelXlsxTests.cs +++ b/tests/ExcelReader.Tests/Parser/SheetParallelXlsxTests.cs @@ -2,6 +2,7 @@ using System.Globalization; using System.Text; using ExcelReader.Core.Parser; +using ExcelReader.Core.Parser.ParallelCsv; using ExcelReader.Core.Parser.SheetPipeline; using ExcelReader.Core.Reader; using ExcelReader.Core.Reader.Xlsx; @@ -189,6 +190,36 @@ public async Task AParseFailureAfterPipelineRecoveryReportsTheSheetRow() } } + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task APartitionedAggregateFailureReportsTheSheetRow(bool needsRecovery) + { + CancellationToken ct = TestContext.Current.CancellationToken; + var sb = new StringBuilder("NameQty"); + for (int r = 2; r <= 300; r++) + { + string name = needsRecovery && r == 20 ? " ]]>" : string.Create(CultureInfo.InvariantCulture, $"n{r}"); + string qty = r == 211 ? "oops" : string.Create(CultureInfo.InvariantCulture, $"{r}"); + sb.Append(CultureInfo.InvariantCulture, $"{name}{qty}"); + } + using MemoryStream ms = WorkbookBuilder.Build(sb.ToString()); + using XlsxWorkbook workbook = Excel.FromXlsx(ms); + ExcelParser parser = ExcelParser.FromAttributes(new ExcelParserOptions { ThrowOnParseFailure = true }); + ExcelParseException sequential = await Assert.ThrowsAsync(() => SheetParallel.AggregateAsync( + workbook.FirstSheet, parser, new SheetParallelOptions { DegreeOfParallelism = 1, HeaderRow = 1 }, ct)); + Assert.Equal(211, sequential.Row); + SheetPipelineSource source = SheetPipelineSource.Create(workbook.FirstSheet)!; + + foreach (int chunkBytes in new[] { 64, 90, 96, 150, 400 }) + { + ExcelParseException parallel = await Assert.ThrowsAsync(() => SheetPipelineRunner.AggregateAsync( + source, MappedAggregation.Unbound, MappedAggregation.Binder(parser, parser.Info, false, 1), 1, 4, chunkBytes, ct)); + Assert.Equal(sequential.Row, parallel.Row); + Assert.Equal(sequential.ColumnName, parallel.ColumnName); + } + } + [Fact] public async Task ACallbackExceptionReachesTheCaller() { From 01389ec80c17d25ae34877c1bdcd34ebf11c35f0 Mon Sep 17 00:00:00 2001 From: Gabriel Matte Date: Tue, 20 Oct 2026 16:53:57 -0300 Subject: [PATCH 03/34] fix(csv): stop later chunks once a parallel ForEachAsync or AggregateAsync chunk fails Workers drained the whole chunk plan even after a chunk failed, so a body that threw on the first record still ran for nearly every other record in the file (19,811 of 20,000 in the new test). The merge only ever throws the first failure in sheet order, so that work was wasted and its side effects happened after the error. A shared gate records the lowest failed chunk index; chunks past it are skipped, and one in progress stops at its next record. Chunks before the failure still run to the end, so everything a sequential read would deliver first is delivered. If the failing chunk was misguessed and its re-read succeeds, the merge reads the skipped chunks itself. --- .../ParallelCsv/ParallelCsvProcessor.cs | 68 +++++++++++++++---- src/ExcelReader.Core/Reader/SheetParallel.cs | 5 ++ .../ParallelCsv/ForEachCsvParallelTests.cs | 24 +++++++ 3 files changed, 85 insertions(+), 12 deletions(-) diff --git a/src/ExcelReader.Core/Parser/ParallelCsv/ParallelCsvProcessor.cs b/src/ExcelReader.Core/Parser/ParallelCsv/ParallelCsvProcessor.cs index 4c0d2bff..0c6e3a89 100644 --- a/src/ExcelReader.Core/Parser/ParallelCsv/ParallelCsvProcessor.cs +++ b/src/ExcelReader.Core/Parser/ParallelCsv/ParallelCsvProcessor.cs @@ -12,15 +12,43 @@ internal static class ParallelCsvProcessor private const int MaxChunkBytes = 64 * 1024 * 1024; private const int RowsPerCancellationCheck = 4096; - private sealed class ChunkOutcome(long actualStart, TState state) + private sealed class ChunkOutcome(long actualStart, TState state, FailureGate? gate) { public readonly long ActualStart = actualStart; + public readonly FailureGate? Gate = gate; public long ResolvedNextStart = long.MaxValue; public TState State = state; public long Delivered; + public bool Abandoned; public ExceptionDispatchInfo? Failure; } + // The lowest index of a chunk that failed. Chunks past it stop: the merge throws at that failure before + // reaching them, unless the failing chunk was misguessed and is re-read clean, and then it reads them itself. + private sealed class FailureGate + { + private int _first = int.MaxValue; + + internal void Fail(int index) + { + int seen = Volatile.Read(ref _first); + while (index < seen) + { + int prior = Interlocked.CompareExchange(ref _first, index, seen); + if (prior == seen) + { + return; + } + seen = prior; + } + } + + internal bool Past(int index) + { + return index > Volatile.Read(ref _first); + } + } + internal static Task RunWithChunkSizeAsync( ReadOnlyMemory data, RowAggregation aggregation, RowActionFactory? factory, CsvReaderOptions reader, int headerRow, int degreeOfParallelism, int chunkSize, CancellationToken ct) @@ -123,11 +151,12 @@ private static async Task PartitionedAsync( : (int)Math.Clamp(dataLength / ((long)dop * ChunksPerWorker), MinChunkBytes, MaxChunkBytes); CsvChunkPlan plan = CsvChunkPlan.Create(dataStart, dataLength, dop, chunkSize); - var outcomes = new ChunkOutcome[plan.Count]; + var outcomes = new ChunkOutcome?[plan.Count]; + var gate = new FailureGate(); var workers = new Task[Math.Min(dop, plan.Count)]; for (int w = 0; w < workers.Length; w++) { - workers[w] = Task.Run(() => DrainPlanAsync(source, plan, dataStart, reader, aggregation, outcomes, ct), ct); + workers[w] = Task.Run(() => DrainPlanAsync(source, plan, dataStart, reader, aggregation, outcomes, gate, ct), ct); } await Task.WhenAll(workers).ConfigureAwait(false); @@ -136,10 +165,10 @@ private static async Task PartitionedAsync( long recordsBefore = headerRow; for (int i = 0; i < plan.Count && confirmedStart < long.MaxValue; i++) { - ChunkOutcome outcome = outcomes[i]; - if (outcome.ActualStart != confirmedStart) + ChunkOutcome? outcome = outcomes[i]; + if (outcome is null || outcome.Abandoned || outcome.ActualStart != confirmedStart) { - outcome = await ParseChunkAsync(source, plan[i], confirmedStart, reader, aggregation, ct).ConfigureAwait(false); + outcome = await ParseChunkAsync(source, plan[i], confirmedStart, reader, aggregation, gate: null, ct).ConfigureAwait(false); } if (outcome.Failure is not null) { @@ -159,21 +188,25 @@ private static async Task PartitionedAsync( private static async Task DrainPlanAsync( CsvChunkSource source, CsvChunkPlan plan, long dataStart, CsvReaderOptions reader, - RowAggregation aggregation, ChunkOutcome[] outcomes, CancellationToken ct) + RowAggregation aggregation, ChunkOutcome?[] outcomes, FailureGate gate, CancellationToken ct) { while (plan.TryTakeNext(out CsvChunk chunk)) { ct.ThrowIfCancellationRequested(); + if (gate.Past(chunk.Index)) + { + continue; + } long start = chunk.Index == 0 ? dataStart : CsvChunkWorker.GuessStart(source, chunk, reader.Quote, reader.Delimiter); - outcomes[chunk.Index] = await ParseChunkAsync(source, chunk, start, reader, aggregation, ct).ConfigureAwait(false); + outcomes[chunk.Index] = await ParseChunkAsync(source, chunk, start, reader, aggregation, gate, ct).ConfigureAwait(false); } } private static ValueTask> ParseChunkAsync( CsvChunkSource source, CsvChunk chunk, long start, CsvReaderOptions reader, - RowAggregation aggregation, CancellationToken ct) + RowAggregation aggregation, FailureGate? gate, CancellationToken ct) { - var outcome = new ChunkOutcome(start, aggregation.Seed()); + var outcome = new ChunkOutcome(start, aggregation.Seed(), gate); if (start >= source.Length) { return new ValueTask>(outcome); @@ -204,7 +237,7 @@ private static void ParseMemoryChunk( } catch (Exception ex) when (ex is not OperationCanceledException) { - outcome.Failure = ExceptionDispatchInfo.Capture(ex); + Fail(outcome, chunk, ex); } finally { @@ -234,13 +267,19 @@ private static async ValueTask> ParseFileChunkAsync } catch (Exception ex) when (ex is not OperationCanceledException) { - outcome.Failure = ExceptionDispatchInfo.Capture(ex); + Fail(outcome, chunk, ex); } } } return outcome; } + private static void Fail(ChunkOutcome outcome, CsvChunk chunk, Exception ex) + { + outcome.Failure = ExceptionDispatchInfo.Capture(ex); + outcome.Gate?.Fail(chunk.Index); + } + private static bool DeliverWithinChunk( CsvReader.Enumerator rows, CsvChunk chunk, ChunkOutcome outcome, RowAction accumulate, CancellationToken ct) { @@ -250,6 +289,11 @@ private static bool DeliverWithinChunk( outcome.ResolvedNextStart = absolute; return false; } + if (outcome.Gate is not null && outcome.Gate.Past(chunk.Index)) + { + outcome.Abandoned = true; + return false; + } if (++outcome.Delivered % RowsPerCancellationCheck == 0) { ct.ThrowIfCancellationRequested(); diff --git a/src/ExcelReader.Core/Reader/SheetParallel.cs b/src/ExcelReader.Core/Reader/SheetParallel.cs index 9e53f288..142ab05b 100644 --- a/src/ExcelReader.Core/Reader/SheetParallel.cs +++ b/src/ExcelReader.Core/Reader/SheetParallel.cs @@ -110,6 +110,11 @@ public static IAsyncEnumerable ParseAsync( /// discarded partition's accumulator is thrown away, and apply the side effects once it returns. /// /// + /// When or a parse throws, partitions further into the sheet stop: on CSV at their + /// next record, on XLSX and XLSB within the few chunks already in flight. Partitions before it run to the + /// end, so every record a sequential read would deliver before the failure is delivered. + /// + /// /// A column points into a pooled buffer the worker reuses once /// returns. Copy anything that must outlive the call. /// diff --git a/tests/ExcelReader.Tests/Parser/ParallelCsv/ForEachCsvParallelTests.cs b/tests/ExcelReader.Tests/Parser/ParallelCsv/ForEachCsvParallelTests.cs index c1938880..1940cfec 100644 --- a/tests/ExcelReader.Tests/Parser/ParallelCsv/ForEachCsvParallelTests.cs +++ b/tests/ExcelReader.Tests/Parser/ParallelCsv/ForEachCsvParallelTests.cs @@ -160,6 +160,30 @@ await Assert.ThrowsAsync( TestContext.Current.CancellationToken)); } + [Fact] + public async Task ForEachRecord_BodyThrows_LaterChunksStopDelivering() + { + const int rows = 20_000; + byte[] csv = SalesCsv(rows); + int delivered = 0; + + await Assert.ThrowsAsync(() => ChunkedRecord( + csv, + sale => + { + if (sale.Units == 1) + { + throw new InvalidOperationException("boom"); + } + Interlocked.Increment(ref delivered); + }, + new ChunkProbe(), + dop: 2, + chunkSize: 2048)); + + Assert.True(delivered < rows / 4, $"{delivered} of {rows} records were delivered after the first one failed"); + } + [Fact] public async Task ForEachRecord_Cancelled_Throws() { From 767db66b2a37916a8b31142eb4581bab6b54e261 Mon Sep 17 00:00:00 2001 From: Gabriel Matte Date: Tue, 20 Oct 2026 17:38:33 -0300 Subject: [PATCH 04/34] fix(csv): deliver exactly once in a parallel ForEachAsync on RFC 4180 quoting A chunk's quote state was read off the first quote after its start that was not half of a "" pair, so a run like ,""",x" (a field opening with a literal quote) or a field starting with the delimiter or a line break was guessed wrong, and ForEachAsync delivered that chunk twice. The guess now works on whole runs of quotes, reaching back over a run the start cuts. Under RFC 4180 an odd run that opens a field follows a delimiter or a line start and one that closes it precedes a delimiter, a line end or the end of the source, so a run fitting only one decides. A run fitting both is no longer guessed: the chunk is left to the merge, which reads it from its confirmed start. The window grows from 4 KB to 64 KB and may look past the chunk's end. ForEachAsync is now exactly once on a CSV that follows RFC 4180 quoting with no quoted field past 64 KB, which a seeded random-CSV test checks over 25 files and five chunk sizes. Text after a closing quote, a stray quote in an unquoted field or a longer field still guess outside and keep the at-least-once fallback. None of the four benchmark files defers or misguesses a chunk, and four-thread timings match the previous commit within noise. --- docs/guide/reading.md | 5 +- .../Parser/ParallelCsv/CsvBoundaryResolver.cs | 67 ++++++++++++++---- .../Parser/ParallelCsv/CsvChunkWorker.cs | 59 ++++++++++------ .../Reader/IRowAccumulator.cs | 5 +- src/ExcelReader.Core/Reader/SheetParallel.cs | 37 +++++----- .../ParallelCsv/AggregateCsvParallelTests.cs | 3 +- .../ParallelCsv/CsvBoundaryResolverTests.cs | 67 ++++++++++++++++-- .../Parser/ParallelCsv/CsvChunkWorkerTests.cs | 5 +- .../ParallelCsv/ForEachCsvParallelTests.cs | 70 ++++++++++++++++++- 9 files changed, 252 insertions(+), 66 deletions(-) diff --git a/docs/guide/reading.md b/docs/guide/reading.md index 195f2d76..4a9b038c 100644 --- a/docs/guide/reading.md +++ b/docs/guide/reading.md @@ -121,9 +121,10 @@ These cases read on one thread with the same results and no error: - **`ParseAsync` and `AggregateAsync`** are exact on every format. `ParseAsync` yields the same items in the same order as the sequential parse; `AggregateAsync` equals its own single-threaded run. - **XLSB** delivers every row exactly once, in `ForEachAsync` too. - **XLSX** delivers exactly once for a well-formed sheet. The pipeline cuts the inflated sheet at what looks like a row start; on a sheet where those bytes sit inside cell content (a CDATA run, a comment) or the file is truncated, a worker detects it, and the read recovers by reading the rest sequentially. `ParseAsync` and `AggregateAsync` stay exact. In `ForEachAsync`, workers may already have called the callback for chunks that are then dropped, so on such a sheet rows can be delivered twice, or be rows the sequential reader would not produce. -- **CSV** is at least once in `ForEachAsync`, unchanged from v6: the callback may be invoked more than once for the same record. A partition whose start offset was guessed wrongly is read again from the confirmed offset, and the discarded pass may already have delivered a whole partition's worth of records, 1 MiB to 64 MiB of them, not just a few near the seam. On a source with quoted fields those records may also be misparsed, carrying values that appear nowhere in the file, and an exception the callback threw during a discarded pass is discarded with it. A start is only guessed wrongly inside a quoted field, so a source in which the quote character never appears delivers every record exactly once. For that guarantee on any source, use `AggregateAsync`, where the discarded partition's accumulator is thrown away, and apply side effects once it returns. +- **CSV** delivers exactly once when it follows RFC 4180 quoting (every quote opens or closes a quoted field, or is doubled inside one) and no quoted field is longer than 64 KB. A partition's start is read off the quotes around it, and a partition whose quotes could open as well as close a field waits for its confirmed start instead of being guessed. +- **CSV outside those rules** (a stray quote inside an unquoted field, text after a closing quote, a quoted field past 64 KB) is at least once in `ForEachAsync`: the callback may be invoked more than once for the same record. A partition whose start offset was guessed wrongly is read again from the confirmed offset, and the discarded pass may already have delivered a whole partition's worth of records, 1 MiB to 64 MiB of them, not just a few near the seam. Those records may also be misparsed, carrying values that appear nowhere in the file, and an exception the callback threw during a discarded pass is discarded with it. For exactly once on any source, use `AggregateAsync`, where the discarded partition's accumulator is thrown away, and apply side effects once it returns. -A callback that writes to a database or sends messages must be idempotent on CSV, and on XLSX if the sheet may be malformed, and must also tolerate records that do not exist: deduplicating on a key does not filter those out. +A callback that writes to a database or sends messages must be idempotent on a CSV that may break those rules, and on XLSX if the sheet may be malformed, and must also tolerate records that do not exist: deduplicating on a key does not filter those out. [`ExcelParseException.Row`](xref:ExcelReader.Core.Parser.ExcelParseException.Row) on `ParseAsync` counts the rows emitted so far. On `ForEachAsync` and `AggregateAsync` it is the row a sequential read reports, whether or not the read was partitioned. diff --git a/src/ExcelReader.Core/Parser/ParallelCsv/CsvBoundaryResolver.cs b/src/ExcelReader.Core/Parser/ParallelCsv/CsvBoundaryResolver.cs index d0410eae..56f0d199 100644 --- a/src/ExcelReader.Core/Parser/ParallelCsv/CsvBoundaryResolver.cs +++ b/src/ExcelReader.Core/Parser/ParallelCsv/CsvBoundaryResolver.cs @@ -20,23 +20,66 @@ internal static bool StartsRecord(ReadOnlySpan pair) return pair[0] == Cr && pair[1] != Lf; } - // Skipping "" pairs, which leave the parity alone, the first quote followed by a delimiter or a line end - // closes a field, so the window started inside one. Anything else is taken as outside; a wrong guess - // still costs only a re-read, since the chunk's start is confirmed afterwards. - internal static CsvQuoteParity InferParity(ReadOnlySpan window, byte quote, byte delimiter) + // Reads the quote state at window[start] off the first odd run of quotes there, starting from the whole of + // a run that start cuts; even runs leave the parity alone. Under RFC 4180 a run that opens a field follows a + // delimiter or a line start, and one that closes a field precedes a delimiter, a line end or the end of the + // source, so a run that fits only one of the two decides. A run that fits both is null, as is one the + // window cuts off: only the confirmed start can place that chunk. No run, or one that fits neither, is + // guessed outside, and the confirmation after the parse still checks it. + internal static CsvQuoteParity? InferParity( + ReadOnlySpan window, int start, bool startsAtSource, bool endsAtEof, byte quote, byte delimiter) { - int i = window.IndexOf(quote); - while (i >= 0 && i + 1 < window.Length) + int runStart = start; + while (runStart > 0 && window[runStart - 1] == quote) { - byte next = window[i + 1]; - if (next != quote) + runStart--; + } + if (runStart == 0 && start > 0 && !startsAtSource) + { + return null; + } + bool flipped = ((start - runStart) & 1) == 1; + int i = runStart; + if (runStart == start) + { + int found = window[start..].IndexOf(quote); + i = found < 0 ? -1 : start + found; + } + while (i >= 0) + { + int end = i + 1; + while (end < window.Length && window[end] == quote) + { + end++; + } + if (((end - i) & 1) == 0) + { + int next = window[end..].IndexOf(quote); + i = next < 0 ? -1 : end + next; + continue; + } + if (end == window.Length && !endsAtEof) { - return next == delimiter || next == Lf || next == Cr ? CsvQuoteParity.Inside : CsvQuoteParity.Outside; + return flipped ? null : CsvQuoteParity.Outside; } - int after = window[(i + 2)..].IndexOf(quote); - i = after < 0 ? -1 : i + 2 + after; + if (i == 0 && !startsAtSource) + { + return null; + } + bool closes = end == window.Length || EndsField(window[end], delimiter); + bool opens = i == 0 || EndsField(window[i - 1], delimiter); + if (closes && opens) + { + return null; + } + return closes != flipped ? CsvQuoteParity.Inside : CsvQuoteParity.Outside; } - return CsvQuoteParity.Outside; + return flipped ? null : CsvQuoteParity.Outside; + } + + private static bool EndsField(byte b, byte delimiter) + { + return b == delimiter || b == Lf || b == Cr; } internal static int FindRecordStart(ReadOnlySpan window, byte quote, CsvQuoteParity parity) diff --git a/src/ExcelReader.Core/Parser/ParallelCsv/CsvChunkWorker.cs b/src/ExcelReader.Core/Parser/ParallelCsv/CsvChunkWorker.cs index 48daef01..3fe18e5e 100644 --- a/src/ExcelReader.Core/Parser/ParallelCsv/CsvChunkWorker.cs +++ b/src/ExcelReader.Core/Parser/ParallelCsv/CsvChunkWorker.cs @@ -105,6 +105,8 @@ internal CsvChunkResult(int index, List models, long actualStart, long resolv internal static class CsvChunkWorker { private const int InitialBoundaryWindow = 4 * 1024; + private const int InferenceWindow = 64 * 1024; + private const int InferenceLookBehind = 64; private const int BoundaryWindowGrowth = 8; internal static ValueTask> ParseAsync( @@ -213,30 +215,29 @@ private static async ValueTask> ParseFromAsync( return (resolvedNextStart, failure, records); } + // long.MaxValue when the chunk holds no record start, or when its quote state cannot be told from its + // own bytes; either way the merge reads it from the confirmed start. internal static long GuessStart(CsvChunkSource source, CsvChunk chunk, byte quote, byte delimiter) { long chunkLength = chunk.End - chunk.Start; - if (chunkLength <= 0) + if (chunkLength <= 0 || InferParity(source, chunk.Start, quote, delimiter) is not CsvQuoteParity parity) { return long.MaxValue; } + if (parity == CsvQuoteParity.Outside && AlreadyAtRecordStart(source, chunk.Start)) + { + return chunk.Start; + } if (source.IsMemory) { int cap = (int)Math.Min(chunkLength, int.MaxValue - 1); - ReadOnlySpan window = source.WindowSpan(chunk.Start, cap); - CsvQuoteParity parity = Infer(window, quote, delimiter); - if (parity == CsvQuoteParity.Outside && AlreadyAtRecordStart(source, chunk.Start)) - { - return chunk.Start; - } - int found = CsvBoundaryResolver.FindRecordStart(window, quote, parity); + int found = CsvBoundaryResolver.FindRecordStart(source.WindowSpan(chunk.Start, cap), quote, parity); return found < 0 ? long.MaxValue : chunk.Start + found; } long remaining = Math.Min(chunkLength, source.Length - chunk.Start); int windowLength = (int)Math.Min(remaining, InitialBoundaryWindow); - CsvQuoteParity? inferred = null; while (true) { int rentSize = windowLength == remaining ? windowLength + 1 : windowLength; @@ -248,16 +249,7 @@ internal static long GuessStart(CsvChunkSource source, CsvChunk chunk, byte quot { return long.MaxValue; } - if (inferred is null) - { - inferred = Infer(buffer.AsSpan(0, read), quote, delimiter); - if (inferred == CsvQuoteParity.Outside && AlreadyAtRecordStart(source, chunk.Start)) - { - return chunk.Start; - } - } - int found = CsvBoundaryResolver.FindRecordStart( - buffer.AsSpan(0, read), quote, inferred.Value); + int found = CsvBoundaryResolver.FindRecordStart(buffer.AsSpan(0, read), quote, parity); if (found >= 0) { return chunk.Start + found; @@ -275,9 +267,34 @@ internal static long GuessStart(CsvChunkSource source, CsvChunk chunk, byte quot } } - private static CsvQuoteParity Infer(ReadOnlySpan window, byte quote, byte delimiter) + // The window reaches back over a run of quotes the start may cut, and past the chunk's end when it has to: + // it only serves to find the first run of quotes. + private static CsvQuoteParity? InferParity(CsvChunkSource source, long start, byte quote, byte delimiter) { - return CsvBoundaryResolver.InferParity(window[..Math.Min(window.Length, InitialBoundaryWindow)], quote, delimiter); + if (start <= 0) + { + return CsvQuoteParity.Outside; + } + int back = (int)Math.Min(InferenceLookBehind, start); + int size = back + (int)Math.Min(InferenceWindow, source.Length - start); + bool startsAtSource = start == back; + if (source.IsMemory) + { + ReadOnlySpan window = source.WindowSpan(start - back, size)[..size]; + return CsvBoundaryResolver.InferParity(window, back, startsAtSource, start - back + size == source.Length, quote, delimiter); + } + byte[] buffer = ArrayPool.Shared.Rent(size); + try + { + int read = source.ReadWindow(start - back, buffer.AsSpan(0, size)); + return read <= back + ? CsvQuoteParity.Outside + : CsvBoundaryResolver.InferParity(buffer.AsSpan(0, read), back, startsAtSource, start - back + read == source.Length, quote, delimiter); + } + finally + { + ArrayPool.Shared.Return(buffer); + } } private static bool AlreadyAtRecordStart(CsvChunkSource source, long start) diff --git a/src/ExcelReader.Core/Reader/IRowAccumulator.cs b/src/ExcelReader.Core/Reader/IRowAccumulator.cs index ec30d5b2..78f8a439 100644 --- a/src/ExcelReader.Core/Reader/IRowAccumulator.cs +++ b/src/ExcelReader.Core/Reader/IRowAccumulator.cs @@ -9,8 +9,9 @@ namespace ExcelReader.Core.Reader /// /// /// runs concurrently across instances on worker threads, and must mutate only the - /// instance it is called on. For CSV, a partition that started at a misguessed row boundary is read again - /// into a new instance and the first one is discarded, so an instance may see rows that are later + /// instance it is called on. For CSV, a partition that started at a misguessed row boundary, which takes a + /// quote outside RFC 4180 rules or a quoted field longer than 64 KB, is read again into a new instance and + /// the first one is discarded, so an instance may see rows that are later /// thrown away — including rows whose parse or threw, and rows misparsed from /// the wrong start that appear nowhere in the sheet. A side effect outside the instance is not undone. /// XLSX and XLSB never re-read a partition. diff --git a/src/ExcelReader.Core/Reader/SheetParallel.cs b/src/ExcelReader.Core/Reader/SheetParallel.cs index 142ab05b..52eaaee8 100644 --- a/src/ExcelReader.Core/Reader/SheetParallel.cs +++ b/src/ExcelReader.Core/Reader/SheetParallel.cs @@ -23,10 +23,10 @@ namespace ExcelReader.Core.Reader /// or a non-UTF-8 encoding). /// /// - /// Delivery guarantees. The general contract is the weaker CSV one: ForEachAsync is at-least-once, - /// and a partition whose start was guessed wrongly is discarded and read again. XLSB delivers every row - /// exactly once, in ForEachAsync too. XLSX delivers exactly once for a well-formed sheet; a - /// malformed one can make ForEachAsync deliver a row twice. ParseAsync yields the same items in + /// Delivery guarantees. XLSB delivers every row exactly once, in ForEachAsync too. XLSX does for a + /// well-formed sheet, and CSV for one that follows RFC 4180 quoting with no quoted field longer than 64 KB; + /// outside those, ForEachAsync is at-least-once, since a partition whose start was guessed wrongly + /// is discarded and read again. ParseAsync yields the same items in /// the same order as the sequential on every format, and /// AggregateAsync and ForEachAsync equal their own single-thread run. /// @@ -54,9 +54,10 @@ public static class SheetParallel /// Parsing and type conversion run in parallel; whatever the caller does per row does not. A caller /// whose per-row work dominates will see little gain from raising . /// For CSV, yields substantially less here than sequentially, - /// since each partition keeps its own dedup cache. A partition read from a wrongly guessed start is - /// discarded before any of its rows are yielded, but its cells have already passed through the parser's - /// converters and property setters, which must therefore be free of side effects. + /// since each partition keeps its own dedup cache. A partition read from a wrongly guessed start, which + /// takes a CSV outside the exactly-once conditions of ForEachAsync, is discarded before any of its + /// rows are yielded, but its cells have already passed through the parser's converters and property + /// setters, which must therefore be free of side effects. /// public static IAsyncEnumerable ParseAsync( IExcelSheet sheet, ExcelParser parser, SheetParallelOptions? options = null, CancellationToken ct = default) @@ -91,21 +92,23 @@ public static IAsyncEnumerable ParseAsync( /// several at once; the caller owns any synchronization. Records arrive in no particular order. /// /// - /// Delivery is at least once in general: may be invoked more than once for the + /// XLSB delivers every row exactly once. CSV does when every quote opens or closes a quoted field or is + /// doubled inside one, as RFC 4180 has it, and no quoted field runs past 64 KB: a partition's start is then + /// read off the quotes around it, and one whose quotes could open or close a field waits for its confirmed + /// start. XLSX delivers exactly once for a well-formed sheet. + /// + /// + /// Outside those, delivery is at least once: may be invoked more than once for the /// same record. On CSV, a partition whose start was guessed wrongly is read again from the confirmed /// offset, and the discarded speculative pass may already have delivered a whole partition's worth of /// records, which may also be misparsed: a wrong start shifts every field boundary. An exception - /// throws during a discarded pass is discarded with it. A start is only guessed - /// wrongly inside a quoted field, so a CSV in which the quote character never appears delivers every - /// record exactly once. A start inside a quoted field is read off the next quote, so a wrong guess also - /// needs a quoted field that begins with the delimiter or a line break, or that runs past 4 KB without a - /// quote. XLSB delivers every row exactly once. XLSX does too for a well-formed sheet; on - /// a malformed one, workers may already have called for rows that are then - /// dropped, so a row can be delivered twice or be one the sequential reader would not produce. + /// throws during a discarded pass is discarded with it. On a malformed XLSX sheet, + /// workers may already have called for rows that are then dropped, so a row can be + /// delivered twice or be one the sequential reader would not produce. /// /// - /// A with side effects must therefore be idempotent and tolerate records that - /// do not exist. For exactly-once on any source, use + /// A with side effects on such a source must therefore be idempotent and tolerate + /// records that do not exist. For exactly-once on any source, use /// , where the /// discarded partition's accumulator is thrown away, and apply the side effects once it returns. /// diff --git a/tests/ExcelReader.Tests/Parser/ParallelCsv/AggregateCsvParallelTests.cs b/tests/ExcelReader.Tests/Parser/ParallelCsv/AggregateCsvParallelTests.cs index 144f8c3c..bcf6156b 100644 --- a/tests/ExcelReader.Tests/Parser/ParallelCsv/AggregateCsvParallelTests.cs +++ b/tests/ExcelReader.Tests/Parser/ParallelCsv/AggregateCsvParallelTests.cs @@ -158,7 +158,8 @@ public async Task SweepsAChunkBoundaryAcrossEveryByteOffsetOfAQuotedNewlineFixtu [Fact] public async Task DiscardsExceptionsThrownWhileReadingAMisguessedPartition() { - byte[] csv = Encoding.UTF8.GetBytes("ok,\"one\nBOOM,x\"\nok,two\nok,\"three\nBOOM,y\"\nok,four\n"); + // Text after each closing quote keeps the start guess from seeing that a chunk begins inside the field. + byte[] csv = Encoding.UTF8.GetBytes("ok,\"one\nBOOM,x\"z\nok,two\nok,\"three\nBOOM,y\"z\nok,four\n"); List expected = Sequential(csv); int misguessedDeliveries = 0; var throwOnBoom = new RowAggregation> diff --git a/tests/ExcelReader.Tests/Parser/ParallelCsv/CsvBoundaryResolverTests.cs b/tests/ExcelReader.Tests/Parser/ParallelCsv/CsvBoundaryResolverTests.cs index de92b81a..0f145a66 100644 --- a/tests/ExcelReader.Tests/Parser/ParallelCsv/CsvBoundaryResolverTests.cs +++ b/tests/ExcelReader.Tests/Parser/ParallelCsv/CsvBoundaryResolverTests.cs @@ -98,14 +98,26 @@ public void FindsABoundaryThatLandsOnTheVeryLastByte() Assert.Equal(3, start); } + private static CsvQuoteParity? Infer(string window, char before = 'x', bool endsAtEof = false, byte delimiter = Delimiter) + { + return InferAfter(before.ToString(), window, endsAtEof, delimiter); + } + + private static CsvQuoteParity? InferAfter(string behind, string window, bool endsAtEof = false, byte delimiter = Delimiter) + { + byte[] bytes = Encoding.UTF8.GetBytes(behind + window); + return CsvBoundaryResolver.InferParity(bytes, Encoding.UTF8.GetByteCount(behind), startsAtSource: false, endsAtEof, Quote, delimiter); + } + [Theory] [InlineData("th\nree\",4\n")] [InlineData("ree\"\n")] [InlineData("ree\"\r\n")] [InlineData("a\"\"b\",4\n")] - public void AQuoteThatClosesAFieldMeansTheWindowStartsInside(string window) + [InlineData("ab\"\"\",4\n")] + public void ARunThatClosesAFieldMeansTheWindowStartsInside(string window) { - Assert.Equal(CsvQuoteParity.Inside, CsvBoundaryResolver.InferParity(Encoding.UTF8.GetBytes(window), Quote, Delimiter)); + Assert.Equal(CsvQuoteParity.Inside, Infer(window)); } [Theory] @@ -116,16 +128,59 @@ public void AQuoteThatClosesAFieldMeansTheWindowStartsInside(string window) [InlineData("plain,4\nplain,5\n")] [InlineData("")] [InlineData("ab\"")] - public void AnyOtherFirstQuoteOrNoneMeansOutside(string window) + public void ARunThatOpensAFieldOrNoRunMeansOutside(string window) + { + Assert.Equal(CsvQuoteParity.Outside, Infer(window)); + } + + [Theory] + [InlineData("a,b\"c,4\n")] + [InlineData("a\"x\",4\n")] + public void ARunThatBreaksTheQuotingRulesIsGuessedOutside(string window) + { + Assert.Equal(CsvQuoteParity.Outside, Infer(window)); + } + + [Theory] + [InlineData("x,\",b\",4\n")] + [InlineData("x\n\",b\",4\n")] + [InlineData("x,\"\n\",4\n")] + [InlineData("1,\"\"\",x\",2\n")] + public void ARunThatCouldOpenOrCloseIsUnknown(string window) + { + Assert.Null(Infer(window)); + } + + [Fact] + public void ARunAtTheWindowStartIsReadAgainstTheByteBeforeIt() + { + Assert.Equal(CsvQuoteParity.Outside, Infer("\"x\",4\n", before: ',')); + Assert.Equal(CsvQuoteParity.Inside, Infer("\",4\n", before: 'a')); + Assert.Null(Infer("\",4\n", before: ',')); + Assert.Null(Infer("\"x\",4\n", before: '"')); + } + + [Fact] + public void AStartInsideARunCountsTheQuotesBeforeIt() + { + Assert.Equal(CsvQuoteParity.Inside, InferAfter("x,\"", "\",4,\"abc\"\n")); + Assert.Equal(CsvQuoteParity.Outside, InferAfter("x,\"said \"", "\"hi\"\"\",5\n")); + Assert.Equal(CsvQuoteParity.Inside, InferAfter("x\n\"", "\"\"quoted\"\" start\",5\n")); + Assert.Null(InferAfter("\"", "\"x\",4\n")); + } + + [Fact] + public void ARunAtTheEndOfTheSourceCloses() { - Assert.Equal(CsvQuoteParity.Outside, CsvBoundaryResolver.InferParity(Encoding.UTF8.GetBytes(window), Quote, Delimiter)); + Assert.Equal(CsvQuoteParity.Inside, Infer("ree\"", endsAtEof: true)); + Assert.Equal(CsvQuoteParity.Outside, Infer("ree\"", endsAtEof: false)); } [Fact] public void HonoursTheDelimiter() { - Assert.Equal(CsvQuoteParity.Inside, CsvBoundaryResolver.InferParity("ree\";4\n"u8, Quote, (byte)';')); - Assert.Equal(CsvQuoteParity.Outside, CsvBoundaryResolver.InferParity("ree\";4\n"u8, Quote, Delimiter)); + Assert.Equal(CsvQuoteParity.Inside, Infer("ree\";4\n", delimiter: (byte)';')); + Assert.Equal(CsvQuoteParity.Outside, Infer("ree\";4\n")); } } } diff --git a/tests/ExcelReader.Tests/Parser/ParallelCsv/CsvChunkWorkerTests.cs b/tests/ExcelReader.Tests/Parser/ParallelCsv/CsvChunkWorkerTests.cs index 1bc8c719..116e50b0 100644 --- a/tests/ExcelReader.Tests/Parser/ParallelCsv/CsvChunkWorkerTests.cs +++ b/tests/ExcelReader.Tests/Parser/ParallelCsv/CsvChunkWorkerTests.cs @@ -102,13 +102,14 @@ public async Task SkipsANewlineHiddenInsideTheQuotedFieldItStartsIn() } [Fact] - public async Task GuessesWrongWhenAQuotedFieldStartsWithTheDelimiter() + public async Task LeavesAChunkWhoseFirstQuoteCouldOpenOrCloseToTheMerge() { byte[] csv = "Name,Age\nAda,0001\n\",b\",0002\nCid,0003\n"u8.ToArray(); CsvChunkResult result = await ParseAsync(csv, start: 10, end: 30); - Assert.NotEqual(18, result.ActualStart); + Assert.Equal(long.MaxValue, result.ActualStart); + Assert.Empty(result.Models); } [Fact] diff --git a/tests/ExcelReader.Tests/Parser/ParallelCsv/ForEachCsvParallelTests.cs b/tests/ExcelReader.Tests/Parser/ParallelCsv/ForEachCsvParallelTests.cs index 1940cfec..1c5c7a02 100644 --- a/tests/ExcelReader.Tests/Parser/ParallelCsv/ForEachCsvParallelTests.cs +++ b/tests/ExcelReader.Tests/Parser/ParallelCsv/ForEachCsvParallelTests.cs @@ -1,4 +1,5 @@ using System.Collections.Concurrent; +using System.Diagnostics.CodeAnalysis; using System.Globalization; using System.Text; using ExcelReader.Core.Parser; @@ -534,6 +535,7 @@ await ChunkedMapped( [InlineData("\"x\ny\",{0}\n")] [InlineData("\"x\",{0},\"y\"\n")] [InlineData("\"said \"\"hi\"\"\",{0}\n")] + [InlineData("\"\"\"quoted\"\" start\",{0}\n")] public async Task ForEachRecord_Chunked_QuotedFields_ReadsEveryChunkAndRecordOnce(string row) { const int rows = 4000; @@ -557,10 +559,51 @@ public async Task ForEachRecord_Chunked_QuotedFields_ReadsEveryChunkAndRecordOnc Assert.All(calls, pair => Assert.Equal(1, pair.Value)); } - // A quoted field that starts with the delimiter makes the opening quote look like a closing one, - // so a chunk that starts outside it is guessed to start inside and read twice. + [SuppressMessage("Security", "CA5394:Do not use insecure randomness", + Justification = "A seeded CSV generator, not randomness protecting anything.")] + private static string RandomRfcField(Random random) + { + const string alphabet = "ab ,\n\r\""; + var field = new StringBuilder(); + for (int n = random.Next(0, 12); n > 0; n--) + { + field.Append(alphabet[random.Next(alphabet.Length)]); + } + string text = field.ToString(); + bool needsQuotes = text.AsSpan().IndexOfAny(",\n\r\"") >= 0; + return needsQuotes || random.Next(3) == 0 ? "\"" + text.Replace("\"", "\"\"", StringComparison.Ordinal) + "\"" : text; + } + + [Fact] + [SuppressMessage("Security", "CA5394:Do not use insecure randomness", + Justification = "A seeded CSV generator, not randomness protecting anything.")] + public async Task ForEachRecord_Chunked_RandomRfcCsv_DeliversEveryRecordExactlyOnce() + { + const int rows = 300; + var random = new Random(20261020); + for (int file = 0; file < 25; file++) + { + var sb = new StringBuilder("Name,Units\n"); + for (int i = 1; i <= rows; i++) + { + sb.Append(RandomRfcField(random)).Append(',').Append(i.ToString(CultureInfo.InvariantCulture)).Append(random.Next(4) == 0 ? "\r\n" : "\n"); + } + byte[] csv = Encoding.UTF8.GetBytes(sb.ToString()); + foreach (int chunkSize in new[] { 7, 31, 64, 200, 1000 }) + { + var calls = new ConcurrentDictionary(); + await ChunkedRecord(csv, sale => calls.AddOrUpdate(sale.Units, 1, static (_, n) => n + 1), new ChunkProbe(), dop: 4, chunkSize); + + Assert.Equal(rows, calls.Count); + Assert.All(calls, pair => Assert.Equal(1, pair.Value)); + } + } + } + + // Quoted fields that start with the delimiter give runs that could open or close a field, so those + // chunks are left to the merge, which reads them from their confirmed start. [Fact] - public async Task ForEachRecord_Chunked_AmbiguousQuotes_InvokesBodyTwiceForSomeRecords() + public async Task ForEachRecord_Chunked_AmbiguousQuotes_DeliversEveryRecordOnce() { const int rows = 4000; var sb = new StringBuilder(); @@ -570,6 +613,27 @@ public async Task ForEachRecord_Chunked_AmbiguousQuotes_InvokesBodyTwiceForSomeR sb.Append(CultureInfo.InvariantCulture, $"\",x\ny\",{i}\n"); } byte[] csv = Encoding.UTF8.GetBytes(sb.ToString()); + var calls = new ConcurrentDictionary(); + + await ChunkedRecord(csv, sale => calls.AddOrUpdate(sale.Units, 1, static (_, n) => n + 1), new ChunkProbe(), dop: 4, chunkSize: 2048); + + Assert.Equal(rows, calls.Count); + Assert.All(calls, pair => Assert.Equal(1, pair.Value)); + } + + // Text after a closing quote breaks the quoting rules, which the parser reads leniently but the start + // guess cannot see, so a chunk that starts inside such a field is read twice. + [Fact] + public async Task ForEachRecord_Chunked_TextAfterAClosingQuote_InvokesBodyTwiceForSomeRecords() + { + const int rows = 4000; + var sb = new StringBuilder(); + sb.Append("Name,Units\n"); + for (int i = 1; i <= rows; i++) + { + sb.Append(CultureInfo.InvariantCulture, $"\"x\ny\"z,{i}\n"); + } + byte[] csv = Encoding.UTF8.GetBytes(sb.ToString()); var probe = new ChunkProbe(); var calls = new ConcurrentDictionary(); From 33fe908a7276b4a4c59110325d96f455ac22c37c Mon Sep 17 00:00:00 2001 From: Gabriel Matte Date: Tue, 20 Oct 2026 18:27:23 -0300 Subject: [PATCH 05/34] fix(reader): reject a sheet whose relationship the workbook rels do not define The XLSX and XLSB readers skipped a sheet whose r:id had no entry in the workbook's relationships part. When no sheet resolved, the empty list already threw, but in a workbook where only some sheets resolved the rest were dropped and the workbook opened as if they never existed. Both now throw InvalidDataException naming the sheet and the missing id, the exception the reader uses for any other corrupt package. The XLSB lookup goes through the dictionary's span lookup, so it no longer copies the id into a string first. --- .../Reader/Xlsb/XlsbWorkbookParts.cs | 10 +++--- .../Reader/Xlsx/XlsxWorkbook.Loading.cs | 7 ++-- .../Reader/DialectRobustnessTests.cs | 33 +++++++++++++++++++ 3 files changed, 43 insertions(+), 7 deletions(-) diff --git a/src/ExcelReader.Core/Reader/Xlsb/XlsbWorkbookParts.cs b/src/ExcelReader.Core/Reader/Xlsb/XlsbWorkbookParts.cs index 12c3eeea..a84c49d3 100644 --- a/src/ExcelReader.Core/Reader/Xlsb/XlsbWorkbookParts.cs +++ b/src/ExcelReader.Core/Reader/Xlsb/XlsbWorkbookParts.cs @@ -9,18 +9,19 @@ internal static (string Name, string Path, ExcelSheetVisibility Visibility)[] Pa Dictionary rels = XlsxXml.ParseRelationships(relsBytes); List<(string, string, ExcelSheetVisibility)> sheets = []; var reader = new Biff12RecordReader(workbookBin); + var alternateLookup = rels.GetAlternateLookup>(); while (reader.TryReadRecord(out int id, out ReadOnlySpan payload)) { if (id != Brt.BundleSh) { continue; } - AddSheet(payload, rels, sheets); + AddSheet(payload, alternateLookup, sheets); } return [.. sheets]; } - private static void AddSheet(ReadOnlySpan payload, Dictionary rels, List<(string, string, ExcelSheetVisibility)> sheets) + private static void AddSheet(ReadOnlySpan payload, Dictionary.AlternateLookup> rels, List<(string, string, ExcelSheetVisibility)> sheets) { if (payload.Length < 8) { @@ -34,10 +35,11 @@ private static void AddSheet(ReadOnlySpan payload, Dictionary"""); + WriteEntry(zip, "xl/workbook.xml", + $""""""); + WriteEntry(zip, "xl/_rels/workbook.xml.rels", + $""""""); + } + ms.Position = 0; + + InvalidDataException ex = Assert.Throws(() => Excel.FromXlsx(ms)); + Assert.Contains("Lost", ex.Message, StringComparison.Ordinal); + } + + [Fact] + public void XlsbSheetWithUnresolvedRelationshipThrows() + { + byte[] workbookBin = + [ + .. B.Record(Brt.BundleSh, [.. B.U32(0), .. B.U32(0), .. B.WideString("rId1"), .. B.WideString("Data")]), + .. B.Record(Brt.BundleSh, [.. B.U32(0), .. B.U32(0), .. B.WideString("rId2"), .. B.WideString("Lost")]), + ]; + byte[] rels = Encoding.UTF8.GetBytes(""""""); + + InvalidDataException ex = Assert.Throws(() => XlsbWorkbookParts.ParseSheets(workbookBin, rels)); + Assert.Contains("Lost", ex.Message, StringComparison.Ordinal); + } + [Fact] public void SingleQuotedStyleAttributesDetectDate() { From 71a5b2569ad3bdb609d2650dbaf1b0b76d55c4f6 Mon Sep 17 00:00:00 2001 From: Gabriel Matte Date: Tue, 20 Oct 2026 18:27:24 -0300 Subject: [PATCH 06/34] docs(native): state the thread-safety contract in excelreader.h The rules lived only in the native README, so a C consumer reading the header had no way to know them, and the Rust crate's Send/Sync impls cited a guarantee the header never made. The header now says that a workbook handle takes calls from several threads at once, that cursors, typed readers, Arrow streams and writer handles are used by one thread at a time, that a closed handle returns XL_INVALID_HANDLE, and that the last error is per thread. --- src/ExcelReader.Native/include/excelreader.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/ExcelReader.Native/include/excelreader.h b/src/ExcelReader.Native/include/excelreader.h index a556be64..4b14563b 100644 --- a/src/ExcelReader.Native/include/excelreader.h +++ b/src/ExcelReader.Native/include/excelreader.h @@ -18,6 +18,10 @@ extern "C" { #define XL_ABI_VERSION 6 +/* Threads: calls on one workbook handle may run on several threads at once. A row cursor, typed + reader, Arrow stream or writer handle is used by one thread at a time. A closed or unknown handle + returns XL_INVALID_HANDLE. xl_last_error reports the calling thread's last failure. */ + #define XL_FORMAT_AUTO 0 /* sniffs XLS/XLSX/XLSB; does NOT detect CSV */ #define XL_FORMAT_XLS 1 #define XL_FORMAT_XLSX 2 From f9039821c7a53559367ad83a1cb359e9a475d099 Mon Sep 17 00:00:00 2001 From: Gabriel Matte Date: Tue, 20 Oct 2026 18:27:24 -0300 Subject: [PATCH 07/34] ci(fuzz): run deflate and deflate-segments under libFuzzer Both targets were registered in the harness and seeded, but only the PR smoke check ran them. They take raw bytes, so they run without a dictionary, and the -dict flag is now passed only when a target has one. --- .github/workflows/fuzz.yml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/.github/workflows/fuzz.yml b/.github/workflows/fuzz.yml index d0dd6254..c17d96a5 100644 --- a/.github/workflows/fuzz.yml +++ b/.github/workflows/fuzz.yml @@ -53,7 +53,7 @@ jobs: strategy: fail-fast: false matrix: - target: [xlsx, xlsx-memory, xlsx-differential, xlsx-pipeline, xlsb, xlsb-memory, xlsb-differential, xlsb-pipeline, xls, encrypted, csv, csv-sniff, csv-parallel, inflate] + target: [xlsx, xlsx-memory, xlsx-differential, xlsx-pipeline, xlsb, xlsb-memory, xlsb-differential, xlsb-pipeline, xls, encrypted, csv, csv-sniff, csv-parallel, inflate, deflate, deflate-segments] steps: - name: Checkout uses: actions/checkout@v7 @@ -111,11 +111,12 @@ jobs: encrypted) dict=encrypted ;; csv*) dict=csv ;; inflate) dict=inflate ;; + deflate*) dict= ;; esac ./libfuzzer-dotnet \ --target_path=fuzz-out/ExcelReader.Fuzz \ corpus/${{ matrix.target }} \ - -dict="tests/ExcelReader.Fuzz/dictionaries/$dict.dict" \ + ${dict:+-dict="tests/ExcelReader.Fuzz/dictionaries/$dict.dict"} \ -max_total_time=${{ github.event.inputs.seconds || '600' }} \ -max_len=131072 \ -timeout=25 \ From 1dc170be9e89731483ab034ef888f173e4fc470c Mon Sep 17 00:00:00 2001 From: Gabriel Matte Date: Tue, 20 Oct 2026 18:27:24 -0300 Subject: [PATCH 08/34] ci: require 88% coverage for C#, Python and Rust on master codecov.yml sets an 88% project target on each language's flag, and the codecov/project/{csharp,python,rust} checks are now required on master. Codecov posts those checks only for a commit it received an upload for, and every upload is path-filtered, so a docs-only PR would wait on them forever. When the .NET tests are skipped, the Linux test job now sends an empty upload, which passes the checks on carried-forward coverage. Both uploads in ci.yml fail the job on error, so a lost upload shows as a red job instead of a check that never arrives. --- .github/workflows/ci.yml | 17 ++++++++++++++++- codecov.yml | 16 ++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ba27c934..9e16ff7b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -135,7 +135,22 @@ jobs: token: ${{ secrets.CODECOV_TOKEN }} files: ./TestResults/*.cobertura.xml flags: csharp - fail_ci_if_error: false + fail_ci_if_error: true + + # The codecov/project checks are required on master, and Codecov posts them only for a commit it + # has heard about; when nothing here ran, an empty upload passes them on carried-forward coverage. + - name: Checkout for empty Codecov upload + if: env.RUN != 'true' && matrix.os == 'ubuntu-latest' + uses: actions/checkout@v7 + + - name: Empty Codecov upload + if: env.RUN != 'true' && matrix.os == 'ubuntu-latest' + uses: codecov/codecov-action@v7 + with: + token: ${{ secrets.CODECOV_TOKEN }} + run_command: empty-upload + force: true + fail_ci_if_error: true isa-fallback: name: Test ISA fallback (${{ matrix.isa }}) diff --git a/codecov.yml b/codecov.yml index 203f0652..a5299547 100644 --- a/codecov.yml +++ b/codecov.yml @@ -1,3 +1,19 @@ +coverage: + status: + project: + csharp: + target: 88% + flags: + - csharp + python: + target: 88% + flags: + - python + rust: + target: 88% + flags: + - rust + # Each language uploads under its own flag from a path-filtered job, so a flag a commit did not run keeps its last report. flag_management: default_rules: From 02d0fca0e09f0d46f88a24a4226c1b0e3f5ed2f8 Mon Sep 17 00:00:00 2001 From: Gabriel Matte Date: Tue, 20 Oct 2026 18:37:57 -0300 Subject: [PATCH 09/34] docs: describe exactly-once CSV ForEachAsync in ARCHITECTURE.md The section on parallel CSV still said only quote-free sources were delivered exactly once and that GuessStart treated every chunk as starting outside quotes. Since 767db66 the worker infers the chunk's quote state from the runs of quotes around its start, and a chunk whose quotes fit both readings waits for its confirmed start. The section now describes that, and limits the caller obligation to the sources that break RFC 4180 quoting or carry a quoted field longer than 64 KB. --- ARCHITECTURE.md | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 09653c1f..bacdfde0 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -175,15 +175,25 @@ the seam. A wrong start also shifts every field boundary, so on a source with qu discarded pass can hand the callback values that appear nowhere in the file. For an accumulator both are invisible: the discarded instance is thrown away. -A guess is only wrong when the offset falls inside a quoted field. `CsvChunkWorker.GuessStart` first -reads the byte before the chunk start and returns the chunk start unchanged when it already sits on a -record boundary, because `CsvBoundaryResolver.FindRecordStart` only ever scans past a terminator and +A guess can only be wrong when the offset falls inside a quoted field, so `CsvChunkWorker.GuessStart` +first decides whether it does. `CsvBoundaryResolver.InferParity` reads the runs of quotes in a window +of up to 64 KB from the chunk start, reaching back over a run the start cuts and looking past the +chunk's end when it has to. Under RFC 4180 an odd run that opens a field follows a delimiter or a line +start, and one that closes a field precedes a delimiter, a line end or the end of the source, so the +first run that fits only one of the two decides. When a run fits both, the worker does not guess at +all: the chunk is left to the merge, which reads it from its confirmed start. + +When the chunk starts outside quotes and the byte before it is a terminator, `GuessStart` returns the +chunk start unchanged, because `CsvBoundaryResolver.FindRecordStart` only ever scans past a terminator and would otherwise overshoot by a whole record on every chunk that happens to be aligned — which, at a -1 MiB minimum chunk size, is every chunk of a fixed-width file. That leaves quote-free sources exact, -and the repeat documented as a caller obligation for the rest rather than designed out. Removing it -there too needs a quote-parity prefix pass over the bytes before any parsing starts; that pass is a -barrier and would cost roughly half the dop-16 runtime unless pipelined, which is more machinery than -the hazard currently warrants. +1 MiB minimum chunk size, is every chunk of a fixed-width file. + +`ForEachAsync` is therefore exactly once on a CSV that follows RFC 4180 quoting with no quoted field +longer than 64 KB. A stray quote inside an unquoted field, text after a closing quote or a longer quoted +field can still be misread, and those sources keep the repeat as a documented caller obligation. +Removing it there too would need a quote-parity prefix pass over the bytes before any parsing starts. +That pass is a barrier and would cost roughly half the dop-16 runtime unless pipelined, which is more +machinery than sources that break RFC 4180 warrant. ## Shared plumbing From bdd38dc47a145a5ef4763b9bc02d343924e18ca6 Mon Sep 17 00:00:00 2001 From: Gabriel Matte Date: Tue, 20 Oct 2026 18:37:58 -0300 Subject: [PATCH 10/34] docs: state in the README what the library reads and writes A reader coming from a document library had no way to tell, short of the API reference, that formula text, merged cells, comments, hyperlinks, charts, images and conditional formatting are out of scope. The README now says so, along with what is read (values, types, cached formula results, sheet names, visibility, tables) and the styling the writers support. --- README.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/README.md b/README.md index cb3bd6e6..a6de56a9 100644 --- a/README.md +++ b/README.md @@ -14,6 +14,12 @@ High-performance Excel reading and writing for .NET 10. Reads `.xlsx`, `.xlsb`, ExcelReader is built for streaming spreadsheet workloads where low allocations matter. It reads worksheet rows as lightweight `ref struct` values, resolves shared strings, recognizes date styles, and handles sparse cells. Workbooks open from a path, a `Stream`, or a caller-owned `ReadOnlyMemory` — the in-memory path decompresses parts directly, without a `ZipArchive` or an intermediate stream. +It moves data, not presentation. Reading gives each cell's value and type, with a formula cell read as +its last calculated result, plus sheet names, visibility and Excel tables. Formula text, merged cells, +comments, hyperlinks, charts, images and conditional formatting are not read. Writing covers values, +column widths and cell styles limited to bold, italic and a number format. To edit an existing workbook +or produce formatted reports, use a library built for document manipulation. + ## Install ```bash From 7a9c6c11d105d3e810699bbf279b23f5029d155e Mon Sep 17 00:00:00 2001 From: Gabriel Matte Date: Tue, 20 Oct 2026 18:37:59 -0300 Subject: [PATCH 11/34] test(benchmarks): compare reads against ExcelDataReader ExcelDataReader is the most-used reader on NuGet and the comparison a reader evaluating the library makes first, but no benchmark included it. ReadBenchmark now reads the XLSX and XLSB workbooks through it, and XlsReadBenchmark the XLS one, accumulating through the same IDataReader path as OfficeIMO. --- README.md | 2 +- .../ExcelReader.Benchmarks.csproj | 1 + .../Read/ReadBenchmark.cs | 18 ++++++++++++++++++ .../Read/XlsReadBenchmark.cs | 8 ++++++++ 4 files changed, 28 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index a6de56a9..313ac3de 100644 --- a/README.md +++ b/README.md @@ -51,7 +51,7 @@ foreach (Row row in workbook.FirstSheet) - [Reading untrusted files](docs/guide/untrusted-input.md) — the resource limits that bound what one hostile file can cost, and how to reject one cleanly. - [Migrating to v7](docs/guide/migrating-to-v7.md) — porting `CsvParallel` code to `SheetParallel`, which now reads XLSX and XLSB on several threads too. - [Migrating to v6](docs/guide/migrating-to-v6.md) — porting v5 reader code to workbooks and sheets, and reading sheets in parallel. -- [Benchmarks](docs/performance/benchmarks.md) — throughput and allocation against Sep, Sylvan, OfficeIMO and SpreadCheetah. [Live results](https://gabrielmarquezmatte.github.io/ExcelReader/dev/bench/). +- [Benchmarks](docs/performance/benchmarks.md) — throughput and allocation against Sep, Sylvan, ExcelDataReader, OfficeIMO and SpreadCheetah. [Live results](https://gabrielmarquezmatte.github.io/ExcelReader/dev/bench/). ## Command line diff --git a/tests/ExcelReader.Benchmarks/ExcelReader.Benchmarks.csproj b/tests/ExcelReader.Benchmarks/ExcelReader.Benchmarks.csproj index 00e45d00..aa391429 100644 --- a/tests/ExcelReader.Benchmarks/ExcelReader.Benchmarks.csproj +++ b/tests/ExcelReader.Benchmarks/ExcelReader.Benchmarks.csproj @@ -25,6 +25,7 @@ + diff --git a/tests/ExcelReader.Benchmarks/Read/ReadBenchmark.cs b/tests/ExcelReader.Benchmarks/Read/ReadBenchmark.cs index f17e01f1..d865c667 100644 --- a/tests/ExcelReader.Benchmarks/Read/ReadBenchmark.cs +++ b/tests/ExcelReader.Benchmarks/Read/ReadBenchmark.cs @@ -1,3 +1,4 @@ +using System.Text; using BenchmarkDotNet.Attributes; using ExcelReader.Core.Reader; using OfficeIMO.Excel; @@ -18,6 +19,7 @@ public class ReadBenchmark [GlobalSetup] public async Task SetupAsync() { + Encoding.RegisterProvider(CodePagesEncodingProvider.Instance); _workbook = await WorkbookGenerator.BuildAsync(Rows); _xlsbWorkbook = await WorkbookGenerator.BuildXlsbAsync(Rows); } @@ -82,6 +84,22 @@ public long Sylvan() return AccumulateSylvanExcel(reader); } + [Benchmark] + public long ExcelDataReader() + { + using var ms = new MemoryStream(_workbook, writable: false); + using var reader = global::ExcelDataReader.ExcelReaderFactory.CreateReader(ms); + return AccumulateDataReader(reader); + } + + [Benchmark] + public long ExcelDataReaderXlsb() + { + using var ms = new MemoryStream(_xlsbWorkbook, writable: false); + using var reader = global::ExcelDataReader.ExcelReaderFactory.CreateReader(ms); + return AccumulateDataReader(reader); + } + [Benchmark] public long OfficeIMO() { diff --git a/tests/ExcelReader.Benchmarks/Read/XlsReadBenchmark.cs b/tests/ExcelReader.Benchmarks/Read/XlsReadBenchmark.cs index f45f7902..c141ded6 100644 --- a/tests/ExcelReader.Benchmarks/Read/XlsReadBenchmark.cs +++ b/tests/ExcelReader.Benchmarks/Read/XlsReadBenchmark.cs @@ -51,6 +51,14 @@ public long Sylvan() return AccumulateSylvanExcel(reader); } + [Benchmark] + public long ExcelDataReader() + { + using var ms = new MemoryStream(_workbook, writable: false); + using var reader = global::ExcelDataReader.ExcelReaderFactory.CreateReader(ms); + return AccumulateDataReader(reader); + } + [Benchmark] public long OfficeIMO() { From 9780cd42a02e089459cfe93b71d15b4ecb5cb322 Mon Sep 17 00:00:00 2001 From: Gabriel Matte Date: Tue, 20 Oct 2026 19:05:17 -0300 Subject: [PATCH 12/34] ci: hold patch coverage to the same 88% as the project codecov/patch defaulted to the base commit's coverage as its target, so a PR had to be covered better than the whole project (91.3% today) to pass, and #149 failed at 88.9%. It now uses the fixed 88% the project targets use. --- codecov.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/codecov.yml b/codecov.yml index a5299547..42699370 100644 --- a/codecov.yml +++ b/codecov.yml @@ -13,6 +13,9 @@ coverage: target: 88% flags: - rust + patch: + default: + target: 88% # Each language uploads under its own flag from a path-filtered job, so a flag a commit did not run keeps its last report. flag_management: From 12686b02230ec9990412f42ddeb7306d24274d15 Mon Sep 17 00:00:00 2001 From: Gabriel Matte Date: Tue, 20 Oct 2026 19:24:25 -0300 Subject: [PATCH 13/34] fix(csv): guess a chunk at the source start to start there AlreadyAtRecordStart returned false for offset 0, so GuessStart on a chunk starting at the source start scanned past the first terminator and skipped the first record. The merge always hands chunk 0 its confirmed start, so no read reached this, but the guess itself was wrong; the start of the source is a record start. --- .../Parser/ParallelCsv/CsvChunkWorker.cs | 2 +- .../Parser/ParallelCsv/CsvChunkWorkerTests.cs | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/ExcelReader.Core/Parser/ParallelCsv/CsvChunkWorker.cs b/src/ExcelReader.Core/Parser/ParallelCsv/CsvChunkWorker.cs index 3fe18e5e..1e6c768a 100644 --- a/src/ExcelReader.Core/Parser/ParallelCsv/CsvChunkWorker.cs +++ b/src/ExcelReader.Core/Parser/ParallelCsv/CsvChunkWorker.cs @@ -301,7 +301,7 @@ private static bool AlreadyAtRecordStart(CsvChunkSource source, long start) { if (start <= 0) { - return false; + return true; } Span pair = stackalloc byte[2]; return source.ReadWindow(start - 1, pair) == pair.Length && CsvBoundaryResolver.StartsRecord(pair); diff --git a/tests/ExcelReader.Tests/Parser/ParallelCsv/CsvChunkWorkerTests.cs b/tests/ExcelReader.Tests/Parser/ParallelCsv/CsvChunkWorkerTests.cs index 116e50b0..053736b7 100644 --- a/tests/ExcelReader.Tests/Parser/ParallelCsv/CsvChunkWorkerTests.cs +++ b/tests/ExcelReader.Tests/Parser/ParallelCsv/CsvChunkWorkerTests.cs @@ -35,6 +35,16 @@ private static async Task> ParseAsync(byte[] csv, long start CancellationToken.None); } + [Fact] + public void AChunkAtTheSourceStartIsGuessedToStartThere() + { + byte[] csv = "Ada,0001\nBob,0002\n"u8.ToArray(); + + long start = CsvChunkWorker.GuessStart(new CsvChunkSource(csv.AsMemory()), new CsvChunk(0, 0, csv.Length), (byte)'"', (byte)','); + + Assert.Equal(0, start); + } + [Fact] public async Task ParsesOnlyTheRecordsStartingInsideItsChunk() { From 3a62fdea44d193b3fb79aa8b75f76ab689550098 Mon Sep 17 00:00:00 2001 From: Gabriel Matte Date: Tue, 20 Oct 2026 19:24:25 -0300 Subject: [PATCH 14/34] test: cover quote-run edge cases and a long XLSX recovery tail InferParity had no test for a run that the window cuts after a start inside it, for a run at index 0 with nothing behind it, or for a start inside a "" pair with no later quote; all three are unknown. The XLSX recovery tail was never long enough to reach its cancellation check, so a 5,000-row sheet now recovers early and reads the rest through it. --- .../Parser/ParallelCsv/CsvBoundaryResolverTests.cs | 3 +++ .../Parser/SheetPipeline/XlsxPipelineTests.cs | 13 +++++++++++++ 2 files changed, 16 insertions(+) diff --git a/tests/ExcelReader.Tests/Parser/ParallelCsv/CsvBoundaryResolverTests.cs b/tests/ExcelReader.Tests/Parser/ParallelCsv/CsvBoundaryResolverTests.cs index 0f145a66..2319d7d6 100644 --- a/tests/ExcelReader.Tests/Parser/ParallelCsv/CsvBoundaryResolverTests.cs +++ b/tests/ExcelReader.Tests/Parser/ParallelCsv/CsvBoundaryResolverTests.cs @@ -158,6 +158,7 @@ public void ARunAtTheWindowStartIsReadAgainstTheByteBeforeIt() Assert.Equal(CsvQuoteParity.Inside, Infer("\",4\n", before: 'a')); Assert.Null(Infer("\",4\n", before: ',')); Assert.Null(Infer("\"x\",4\n", before: '"')); + Assert.Null(InferAfter("", "\"x\",4\n")); } [Fact] @@ -167,6 +168,8 @@ public void AStartInsideARunCountsTheQuotesBeforeIt() Assert.Equal(CsvQuoteParity.Outside, InferAfter("x,\"said \"", "\"hi\"\"\",5\n")); Assert.Equal(CsvQuoteParity.Inside, InferAfter("x\n\"", "\"\"quoted\"\" start\",5\n")); Assert.Null(InferAfter("\"", "\"x\",4\n")); + Assert.Null(InferAfter("x,\"", "\"\"")); + Assert.Null(InferAfter("a\"", "\"b,4\n")); } [Fact] diff --git a/tests/ExcelReader.Tests/Parser/SheetPipeline/XlsxPipelineTests.cs b/tests/ExcelReader.Tests/Parser/SheetPipeline/XlsxPipelineTests.cs index e2621211..6e4f94ea 100644 --- a/tests/ExcelReader.Tests/Parser/SheetPipeline/XlsxPipelineTests.cs +++ b/tests/ExcelReader.Tests/Parser/SheetPipeline/XlsxPipelineTests.cs @@ -228,6 +228,19 @@ public async Task ARowLikeTagInsideARowMatchesSequential(string poison) } } + [Fact] + public async Task ALongReadAfterRecoveryMatchesSequential() + { + using MemoryStream ms = WorkbookBuilder.Build(Rows(5000, r => r <= 100 && r % 10 == 0 ? " ]]>" : "n")); + using XlsxWorkbook workbook = Excel.FromXlsx(ms); + List expected = Sequential(workbook, 0); + + foreach (int chunkBytes in new[] { 64, 90, 150, 400 }) + { + Assert.Equal(expected, await PipelineAsync(workbook, 0, 4, chunkBytes)); + } + } + [Fact] public async Task RowsAfterTheEndOfSheetDataAreIgnored() { From db56d4a8c38eec75318a18b3315332a3205c0328 Mon Sep 17 00:00:00 2001 From: Gabriel Matte Date: Wed, 21 Oct 2026 11:17:44 -0300 Subject: [PATCH 15/34] fix(python): refuse a column buffer that does not match its declared type _buffer_of discarded the byte length it measured, and NativeColumn.length took table.row_count on trust, so ten I64 rows over a single array('q') had the native writer read past an 8-byte copy, and an array('i') declared I64 was written as whatever two int32s packed into one int64. Each value buffer is now checked for a 1-D layout, the element size and signed or float format its ColumnType needs, and exactly row_count elements (string offsets: row_count + 1 int32s), and a validity bitmap for at least row_count bits, before any pointer to it exists. The offsets' contents were already validated natively. --- python/src/excelreader/writer.py | 71 ++++++++++++++++++++++---------- python/tests/test_writer.py | 57 +++++++++++++++++++++++++ 2 files changed, 106 insertions(+), 22 deletions(-) diff --git a/python/src/excelreader/writer.py b/python/src/excelreader/writer.py index 57d27c60..2f353d79 100644 --- a/python/src/excelreader/writer.py +++ b/python/src/excelreader/writer.py @@ -41,54 +41,81 @@ def _resolve_write_format(name: str | None, path: Path) -> int: ) from None -def _buffer_of(values: Any) -> tuple[Any, int]: - """Returns (ctypes-addressable buffer, byte length) for one column's values. - - NumPy arrays, `array.array` and `bytes` all expose the buffer protocol, so one `memoryview` reads - every input shape the same way. The bytes are then COPIED into a ctypes block rather than - borrowed: the copy is owned by this module and cannot be moved or freed out from under the native - side while the call runs. The caller must keep the returned object alive for as long as the - pointer is in use. +_SIGNED_FORMATS = frozenset("bhilqn") +_ELEMENTS = { + ColumnType.I64: (_SIGNED_FORMATS, 8), + ColumnType.TIME: (_SIGNED_FORMATS, 8), + ColumnType.TIMESTAMP: (_SIGNED_FORMATS, 8), + ColumnType.F64: (frozenset("d"), 8), + ColumnType.BOOL: (frozenset("bB?"), 1), + ColumnType.DATE: (_SIGNED_FORMATS, 4), +} + + +def _checked_view(values: Any, formats: frozenset[str], itemsize: int, count: int, what: str) -> memoryview: + """The native side trusts `length` and the column type, and cannot see how big a Python + allocation really is, so a buffer of the wrong shape must be refused here, before any pointer + to it exists. """ - if isinstance(values, (bytes, bytearray)): - raw: Any = values - elif isinstance(values, array): - raw = values - else: - raw = getattr(values, "data", values) - blob = memoryview(raw).tobytes() - return (ctypes.c_char * len(blob)).from_buffer_copy(blob), len(blob) + view = memoryview(getattr(values, "data", values)) + code = view.format.lstrip("@=") + if view.ndim != 1 or view.itemsize != itemsize or code not in formats: + raise TypeError( + f"{what} must be a 1-D buffer of {itemsize}-byte elements with format one of {sorted(formats)}; " + f"got format {view.format!r} with {view.itemsize}-byte elements" + ) + if len(view) != count: + raise ValueError(f"{what} has {len(view)} elements but {count} are needed") + return view + + +def _copy(view: memoryview) -> Any: + """COPIES the bytes into a ctypes block rather than borrowing them: the copy is owned by this + module and cannot be moved or freed out from under the native side while the call runs. The + caller must keep the returned object alive for as long as the pointer is in use. + """ + blob = view.tobytes() + return (ctypes.c_char * len(blob)).from_buffer_copy(blob) def _native_column(column: Any, column_type: ColumnType, validity: bytes | None, row_count: int, keepalive: list[Any]) -> _native.NativeColumn: if isinstance(column, StringColumn): - offsets, _ = _buffer_of(column.offsets) - data, data_len = _buffer_of(column.data) + offsets = _copy(_checked_view(column.offsets, _SIGNED_FORMATS, 4, row_count + 1, "a STRING column's offsets")) + data_view = memoryview(column.data) + data_len = data_view.nbytes + data = _copy(data_view) keepalive += [offsets, data] return _native.NativeColumn( type=int(ColumnType.STRING), length=row_count, values=ctypes.cast(offsets, ctypes.c_void_p), - validity=_validity_pointer(validity, keepalive), + validity=_validity_pointer(validity, row_count, keepalive), data=ctypes.cast(data, ctypes.POINTER(ctypes.c_uint8)) if data_len else None, data_len=data_len, ) - values, _ = _buffer_of(column) + if column_type is ColumnType.STRING: + raise TypeError("a STRING column must be a StringColumn") + formats, itemsize = _ELEMENTS[column_type] + values = _copy(_checked_view(column, formats, itemsize, row_count, f"a {column_type.name} column")) keepalive.append(values) return _native.NativeColumn( type=int(column_type), length=row_count, values=ctypes.cast(values, ctypes.c_void_p), - validity=_validity_pointer(validity, keepalive), + validity=_validity_pointer(validity, row_count, keepalive), data=None, data_len=0, ) -def _validity_pointer(validity: bytes | None, keepalive: list[Any]) -> Any: +def _validity_pointer(validity: bytes | None, row_count: int, keepalive: list[Any]) -> Any: if validity is None: return None + if len(validity) < (row_count + 7) // 8: + raise ValueError( + f"the validity bitmap is {len(validity)} bytes, but {row_count} rows need {(row_count + 7) // 8}" + ) buffer = (ctypes.c_uint8 * len(validity)).from_buffer_copy(validity) keepalive.append(buffer) return ctypes.cast(buffer, ctypes.POINTER(ctypes.c_uint8)) diff --git a/python/tests/test_writer.py b/python/tests/test_writer.py index 8c10c697..bd27962b 100644 --- a/python/tests/test_writer.py +++ b/python/tests/test_writer.py @@ -1,4 +1,5 @@ import io +from array import array import pytest from excelreader import ( @@ -6,6 +7,8 @@ ColumnType, ExcelReaderError, PasswordIncorrectError, + StringColumn, + TypedTable, WriteOptions, encrypt_package, open_workbook, @@ -390,3 +393,57 @@ def test_write_workbook_rejects_a_destination_it_cannot_write(source_csv): with pytest.raises(TypeError, match="destination"): write_workbook(42, table, _TYPES, format="xlsx") + + +def _one_column_table(column, rows=1, validity=None): + return TypedTable(row_count=rows, names=["n"], columns=[column], validity=[validity]) + + +@pytest.mark.parametrize( + ("column", "column_type", "rows", "message"), + [ + (array("q", [7]), ColumnType.I64, 10, "1 elements but 10"), + (array("i", [1, 2]), ColumnType.I64, 1, "8-byte"), + (array("d", [1.0]), ColumnType.I64, 1, "format"), + (array("Q", [1]), ColumnType.I64, 1, "format"), + (array("q", [1]), ColumnType.DATE, 1, "4-byte"), + (StringColumn(array("i", [0]), b""), ColumnType.STRING, 1, "offsets"), + (StringColumn(array("q", [0, 1]), b"a"), ColumnType.STRING, 1, "offsets"), + ], +) +def test_write_workbook_rejects_a_column_buffer_that_does_not_match(column, column_type, rows, message): + with pytest.raises((TypeError, ValueError), match=message): + write_workbook(io.BytesIO(), _one_column_table(column, rows), [column_type], format="xlsx") + + +def test_write_workbook_rejects_a_short_validity_bitmap(): + table = _one_column_table(array("q", range(9)), rows=9, validity=b"\xff") + + with pytest.raises(ValueError, match="validity"): + write_workbook(io.BytesIO(), table, [ColumnType.I64], format="xlsx") + + +def test_write_workbook_takes_numpy_columns(): + numpy = pytest.importorskip("numpy") + table = TypedTable( + row_count=2, + names=["qty", "price", "day", "ok"], + columns=[ + numpy.array([3, 7], dtype=numpy.int64), + numpy.array([1.5, 2.5]), + numpy.array([0, 1], dtype=numpy.int32), + numpy.array([True, False]), + ], + validity=[None, None, None, None], + ) + types = [ColumnType.I64, ColumnType.F64, ColumnType.DATE, ColumnType.BOOL] + buffer = io.BytesIO() + + write_workbook(buffer, table, types, format="xlsx") + + schema = [ColumnSpec(t, name=n) for n, t in zip(table.names, types)] + with open_workbook(buffer.getvalue(), format="xlsx") as workbook: + result = workbook.sheets[0].parse(schema) + assert list(result.columns[0]) == [3, 7] + assert list(result.columns[1]) == [1.5, 2.5] + assert list(result.columns[3]) == [1, 0] From c2a71080f615bf78080d917ce57092afd8eca5bb Mon Sep 17 00:00:00 2001 From: Gabriel Matte Date: Wed, 21 Oct 2026 11:19:12 -0300 Subject: [PATCH 16/34] fix(python): finish a write() that takes only part of the workbook write_workbook, encrypt_package and SheetWriter.close ignored what destination.write() returned, so a destination that took four bytes of a 2,286-byte XLSX left a four-byte file and the call still succeeded. All three now go through _write_all, which calls write() again for the rest. A write() returning None is taken to have written everything, as files that predate the int return convention do; one that returns 0, or more than was left, raises OSError instead of looping. SheetWriter.close still releases the native handle when that raises. --- python/src/excelreader/writer.py | 26 +++++++++++++--- python/src/excelreader/writer_handle.py | 7 +++-- python/tests/test_writer.py | 40 +++++++++++++++++++++++++ python/tests/test_writer_handle.py | 30 +++++++++++++++++++ 4 files changed, 96 insertions(+), 7 deletions(-) diff --git a/python/src/excelreader/writer.py b/python/src/excelreader/writer.py index 2f353d79..32324c3e 100644 --- a/python/src/excelreader/writer.py +++ b/python/src/excelreader/writer.py @@ -177,7 +177,8 @@ def write_workbook( `format` is one of xlsx/xlsb/xls/csv. For a path, None infers it from the extension; an object with `write()` has no extension, so it needs `format`. The whole workbook is built in memory and - handed to `write()` in one call, at the object's current position; the object is not closed. + handed to `write()` at the object's current position, again for the rest whenever `write()` + returns that it took only part; the object is not closed. The column names become the header row. `options` sets the sheet name and CSV dialect; see `WriteOptions`. @@ -201,7 +202,24 @@ def write_workbook( raise ValueError( f"format= is required when writing to an object with write(); one of {sorted(_FORMAT_NAMES)}" ) - destination.write(_write_memory(table, types, format, options)) + _write_all(destination, _write_memory(table, types, format, options)) + + +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. + """ + view = memoryview(data) + offset = 0 + while offset < len(data): + written = destination.write(data if offset == 0 else view[offset:]) + if written is None: + return + if not 0 < written <= len(data) - offset: + raise OSError( + f"write() made no progress: it returned {written} with {len(data) - offset} bytes left to write" + ) + offset += written def _write_file( @@ -398,7 +416,7 @@ def encrypt_package( `package` is a path to a complete file, or its bytes (`bytes`, `bytearray`, `memoryview`, or an `io.BytesIO` read whole). `destination` is a path, or a binary object with `write()` that - receives the encrypted workbook in one call and is not closed. Encryption parameters are fixed + receives the encrypted workbook, as `write_workbook` hands it over, and is not closed. Encryption parameters are fixed at Excel's own defaults; there are no options. """ password_encoded = password.encode("utf-8") @@ -441,4 +459,4 @@ def encrypt_package( if isinstance(destination, (str, os.PathLike)): Path(destination).write_bytes(encrypted) else: - destination.write(encrypted) + _write_all(destination, encrypted) diff --git a/python/src/excelreader/writer_handle.py b/python/src/excelreader/writer_handle.py index 0356d13b..1891f765 100644 --- a/python/src/excelreader/writer_handle.py +++ b/python/src/excelreader/writer_handle.py @@ -16,6 +16,7 @@ from excelreader import _native from excelreader.reader import _check from excelreader.types import ColumnType, WriteOptions +from excelreader.writer import _write_all _EPOCH_DATE = datetime.date(1970, 1, 1) _EPOCH_DATETIME = datetime.datetime(1970, 1, 1) @@ -131,8 +132,8 @@ def _finished_bytes(self, handle: ctypes.c_void_p) -> bytes: def close(self) -> None: """Finalizes and releases the writer. Safe to call more than once. - For a destination with `write()`, the finished workbook is written to it in one call, even - when sheets or rows were left open. + For a destination with `write()`, the finished workbook is written to it, even when sheets + or rows were left open; a `write()` that takes only part of it is called again for the rest. """ if self._handle is None: return @@ -140,7 +141,7 @@ def close(self) -> None: destination, self._destination = self._destination, None try: if destination is not None: - destination.write(self._finished_bytes(handle)) + _write_all(destination, self._finished_bytes(handle)) finally: _check(self._lib.xl_close_write_handle(handle)) diff --git a/python/tests/test_writer.py b/python/tests/test_writer.py index bd27962b..2e226bd8 100644 --- a/python/tests/test_writer.py +++ b/python/tests/test_writer.py @@ -395,6 +395,46 @@ def test_write_workbook_rejects_a_destination_it_cannot_write(source_csv): write_workbook(42, table, _TYPES, format="xlsx") +class _TrickleWriter: + def __init__(self, step): + self.step = step + self.data = bytearray() + + def write(self, chunk): + taken = bytes(chunk[: self.step]) + self.data += taken + return len(taken) + + +def test_write_workbook_finishes_a_partial_write(source_csv): + with open_workbook(source_csv) as workbook: + table = workbook.sheets[0].parse(_SCHEMA) + destination = _TrickleWriter(4) + + write_workbook(destination, table, _TYPES, format="xlsx") + + assert bytes(destination.data) == _to_bytes(table) + + +def test_write_workbook_fails_when_write_makes_no_progress(source_csv): + with open_workbook(source_csv) as workbook: + table = workbook.sheets[0].parse(_SCHEMA) + + with pytest.raises(OSError, match="no progress"): + write_workbook(_TrickleWriter(0), table, _TYPES, format="xlsx") + + +def test_encrypt_package_finishes_a_partial_write(source_csv): + with open_workbook(source_csv) as workbook: + table = workbook.sheets[0].parse(_SCHEMA) + destination = _TrickleWriter(7) + + encrypt_package(_to_bytes(table), destination, "hunter2") + + result = _parse_bytes(bytes(destination.data), "xlsx", password="hunter2") + assert list(result.columns[0]) == ["widget", "gadget"] + + def _one_column_table(column, rows=1, validity=None): return TypedTable(row_count=rows, names=["n"], columns=[column], validity=[validity]) diff --git a/python/tests/test_writer_handle.py b/python/tests/test_writer_handle.py index cbb95378..4078ad99 100644 --- a/python/tests/test_writer_handle.py +++ b/python/tests/test_writer_handle.py @@ -210,6 +210,36 @@ def test_a_writer_over_a_bytesio_finishes_a_sheet_left_open(): assert [row[0].value for row in workbook.sheets[0].rows()] == ["a"] +def test_a_writer_over_a_stream_finishes_a_partial_write(): + class Trickle: + def __init__(self): + self.data = bytearray() + + def write(self, chunk): + self.data += bytes(chunk[:5]) + return min(5, len(chunk)) + + destination = Trickle() + with open_writer(destination, "xlsx") as writer: + writer.start_sheet("Data") + writer.write_row(["widget", 7]) + + with open_workbook(bytes(destination.data)) as workbook: + assert [cell.value for cell in next(workbook.sheets[0].rows())] == ["widget", "7"] + + +def test_a_writer_over_a_stream_that_stalls_raises_and_still_closes(): + class Stalled: + def write(self, chunk): + return 0 + + writer = open_writer(Stalled(), "xlsx") + writer.start_sheet("Data") + with pytest.raises(OSError, match="no progress"): + writer.close() + writer.close() + + def test_a_writer_over_a_stream_needs_a_format_and_a_write_method(): import io From e5b6888f0f60ea486fca83b9755c05575066791a Mon Sep 17 00:00:00 2001 From: Gabriel Matte Date: Wed, 21 Oct 2026 11:19:12 -0300 Subject: [PATCH 17/34] perf(python): decode only the bytes xl_rows_next wrote for a row Sheet.rows handed the decoder buffer.raw, a copy of the whole buffer: 64 KiB per row at first, and more for every row after one that grew it. A slice to written.value copies only the row's own bytes. --- python/src/excelreader/reader.py | 2 +- python/tests/test_reader.py | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/python/src/excelreader/reader.py b/python/src/excelreader/reader.py index bb07f645..161f2602 100644 --- a/python/src/excelreader/reader.py +++ b/python/src/excelreader/reader.py @@ -249,7 +249,7 @@ def rows(self) -> Iterator[list[Cell]]: buffer = ctypes.create_string_buffer(capacity) continue _check(status) - yield _decode_row(buffer.raw, written.value, date1904) + yield _decode_row(buffer[: written.value], written.value, date1904) finally: self._close_rows(cursor) diff --git a/python/tests/test_reader.py b/python/tests/test_reader.py index e5f0be9a..b286b8fe 100644 --- a/python/tests/test_reader.py +++ b/python/tests/test_reader.py @@ -361,6 +361,16 @@ def test_open_options_rejects_an_out_of_range_value(csv_path): open_workbook(csv_path, format="csv", options=OpenOptions(csv_delimiter=999)) +def test_rows_after_one_that_grew_the_buffer_decode_only_their_own_bytes(tmp_path): + path = tmp_path / "grown.csv" + path.write_text("value\n" + ("x" * 200_000) + "\nação\n\n7\n", encoding="utf-8") + + with open_workbook(path, format="csv") as workbook: + rows = [[cell.value for cell in row] for row in workbook.sheets[0].rows()] + + assert rows[2:] == [["ação"], [""], ["7"]] + + def test_open_options_limit_actually_aborts_an_oversized_read(tmp_path): path = tmp_path / "wide-cell.csv" path.write_text("value\n" + ("x" * 200_000) + "\n", encoding="utf-8") From d29a65ea3b1346e87d7bc9c50f89ef6b2fa18814 Mon Sep 17 00:00:00 2001 From: Gabriel Matte Date: Wed, 21 Oct 2026 11:19:12 -0300 Subject: [PATCH 18/34] fix(rust): stop safe code from reading a hand-built column The column_* accessors are safe functions, and __private exported XlColumn with public fields, so code under #![forbid(unsafe_code)] could point a column at any address and read it. The accessors and ColumnBinding::assign now take a ColumnRef, whose field is private to this crate and which only TableView builds over a table the native library returned; XlColumn is no longer exported. The derive's output is unchanged and still compiles under forbid(unsafe_code). --- rust/excelreader/src/ffi.rs | 4 +- rust/excelreader/src/lib.rs | 3 +- rust/excelreader/src/workbook.rs | 40 ++++++++++++++----- .../tests/derive_ui/pass/forbid_unsafe.rs | 21 ++++++++++ 4 files changed, 55 insertions(+), 13 deletions(-) create mode 100644 rust/excelreader/tests/derive_ui/pass/forbid_unsafe.rs diff --git a/rust/excelreader/src/ffi.rs b/rust/excelreader/src/ffi.rs index 9b5c8a43..80a922e9 100644 --- a/rust/excelreader/src/ffi.rs +++ b/rust/excelreader/src/ffi.rs @@ -148,8 +148,8 @@ pub struct XlColumnSpec { pub nullable: i32, } -/// One column of a native table. Reachable from `__private` because the derive's generated -/// bindings read it through the `column_*` accessors. +/// One column of a native table. Crate-private: the derive's generated bindings only ever see it +/// wrapped in a `ColumnRef`, which nothing outside this crate can construct. #[repr(C)] pub struct XlColumn { pub r#type: i32, diff --git a/rust/excelreader/src/lib.rs b/rust/excelreader/src/lib.rs index d7804774..30b915ca 100644 --- a/rust/excelreader/src/lib.rs +++ b/rust/excelreader/src/lib.rs @@ -42,10 +42,9 @@ pub use self::arrow::ArrowBatches; /// follows semver. #[doc(hidden)] pub mod __private { - pub use crate::ffi::XlColumn; pub use crate::workbook::{ column_bool, column_date, column_f64, column_i64, column_str, column_time, - column_timestamp, ColumnBinding, + column_timestamp, ColumnBinding, ColumnRef, }; pub use crate::writer::{push_str, set_valid, OwnedColumn, OwnedColumnData}; diff --git a/rust/excelreader/src/workbook.rs b/rust/excelreader/src/workbook.rs index 4071abf6..aa511cea 100644 --- a/rust/excelreader/src/workbook.rs +++ b/rust/excelreader/src/workbook.rs @@ -614,7 +614,7 @@ pub struct ColumnBinding { /// Candidate header names, in priority order — the first one present in the header row wins. pub names: &'static [&'static str], pub column_type: ColumnType, - pub assign: fn(&mut T, &XlColumn, i64), + pub assign: fn(&mut T, ColumnRef<'_>, i64), } // Hand-written: `#[derive(Clone)]` would add a `T: Clone` bound the user's row struct need not meet. @@ -635,6 +635,21 @@ pub trait ExcelMapper: Sized { pub use excelreader_derive::ExcelMapper; +/// A column of a table the native library returned. Only this crate can build one, which is what +/// lets the safe `column_*` accessors trust its pointers: a hand-built `XlColumn` never reaches them. +/// +/// ```compile_fail +/// #![forbid(unsafe_code)] +/// let column = excelreader::__private::XlColumn { +/// r#type: 1, length: 1, +/// values: std::ptr::null(), validity: std::ptr::null(), +/// data: std::ptr::null(), data_len: 0, +/// }; +/// excelreader::__private::column_i64(excelreader::__private::ColumnRef(&column), 0); +/// ``` +#[derive(Clone, Copy)] +pub struct ColumnRef<'a>(&'a XlColumn); + fn is_valid(col: &XlColumn, row: i64) -> bool { if col.validity.is_null() { return true; @@ -674,7 +689,8 @@ fn check_access(col: &XlColumn, row: i64, expected_type: i32, accessor: &str) { /// # Panics /// If `col` is not a string column, if `row` is out of bounds, or if the native library returned /// bytes that are not valid UTF-8 (an ABI contract violation). -pub fn column_str(col: &XlColumn, row: i64) -> &str { +pub fn column_str<'a>(col: ColumnRef<'a>, row: i64) -> &'a str { + let col = col.0; check_access(col, row, ffi::XL_T_STRING, "column_str"); unsafe { let offsets = col.values as *const i32; @@ -689,28 +705,32 @@ pub fn column_str(col: &XlColumn, row: i64) -> &str { /// # Panics /// If `col` is not an `i64` column, or `row` is out of bounds. -pub fn column_i64(col: &XlColumn, row: i64) -> i64 { +pub fn column_i64(col: ColumnRef<'_>, row: i64) -> i64 { + let col = col.0; check_access(col, row, ffi::XL_T_I64, "column_i64"); unsafe { *(col.values as *const i64).offset(row as isize) } } /// # Panics /// If `col` is not an `f64` column, or `row` is out of bounds. -pub fn column_f64(col: &XlColumn, row: i64) -> f64 { +pub fn column_f64(col: ColumnRef<'_>, row: i64) -> f64 { + let col = col.0; check_access(col, row, ffi::XL_T_F64, "column_f64"); unsafe { *(col.values as *const f64).offset(row as isize) } } /// # Panics /// If `col` is not a bool column, or `row` is out of bounds. -pub fn column_bool(col: &XlColumn, row: i64) -> bool { +pub fn column_bool(col: ColumnRef<'_>, row: i64) -> bool { + let col = col.0; check_access(col, row, ffi::XL_T_BOOL, "column_bool"); unsafe { *(col.values as *const u8).offset(row as isize) != 0 } } /// # Panics /// If `col` is not a date column, or `row` is out of bounds. -pub fn column_date(col: &XlColumn, row: i64) -> Date { +pub fn column_date(col: ColumnRef<'_>, row: i64) -> Date { + let col = col.0; check_access(col, row, ffi::XL_T_DATE, "column_date"); Date { days_since_epoch: unsafe { *(col.values as *const i32).offset(row as isize) }, @@ -719,7 +739,8 @@ pub fn column_date(col: &XlColumn, row: i64) -> Date { /// # Panics /// If `col` is not a time column, or `row` is out of bounds. -pub fn column_time(col: &XlColumn, row: i64) -> Time { +pub fn column_time(col: ColumnRef<'_>, row: i64) -> Time { + let col = col.0; check_access(col, row, ffi::XL_T_TIME, "column_time"); Time { micros_since_midnight: unsafe { *(col.values as *const i64).offset(row as isize) }, @@ -728,7 +749,8 @@ pub fn column_time(col: &XlColumn, row: i64) -> Time { /// # Panics /// If `col` is not a timestamp column, or `row` is out of bounds. -pub fn column_timestamp(col: &XlColumn, row: i64) -> Timestamp { +pub fn column_timestamp(col: ColumnRef<'_>, row: i64) -> Timestamp { + let col = col.0; check_access(col, row, ffi::XL_T_TIMESTAMP, "column_timestamp"); Timestamp { micros_since_epoch: unsafe { *(col.values as *const i64).offset(row as isize) }, @@ -803,7 +825,7 @@ impl TableView { }; for (col, binding) in columns.iter().zip(self.bindings.iter()) { if is_valid(col, row) { - (binding.assign)(&mut instance, col, row); + (binding.assign)(&mut instance, ColumnRef(col), row); } } Some(instance) diff --git a/rust/excelreader/tests/derive_ui/pass/forbid_unsafe.rs b/rust/excelreader/tests/derive_ui/pass/forbid_unsafe.rs new file mode 100644 index 00000000..f7043913 --- /dev/null +++ b/rust/excelreader/tests/derive_ui/pass/forbid_unsafe.rs @@ -0,0 +1,21 @@ +#![forbid(unsafe_code)] + +use excelreader::{Date, ExcelMapper, Time, Timestamp}; + +#[derive(Default, ExcelMapper)] +struct Row { + #[excel(name = "Nome")] + nome: String, + #[excel(name = "Idade")] + idade: Option, + #[excel(name = "Dia")] + dia: Date, + #[excel(name = "Hora")] + hora: Time, + #[excel(name = "Instante")] + instante: Option, +} + +fn main() { + assert_eq!(Row::bindings().len(), 5); +} From c75f29c0bb1c684ab5c7cd098dbdfa29070c878e Mon Sep 17 00:00:00 2001 From: Gabriel Matte Date: Wed, 21 Oct 2026 11:19:12 -0300 Subject: [PATCH 19/34] fix(reader): infer a timestamp for date cells that carry a time of day ColumnStat.Observe marked every date-styled cell as a date, so a column of 2024-01-02 12:30 inferred DateColumn and a parse with that schema dropped the time. A date cell whose value has a time of day, read through TryGetDateTime with the workbook's date system, now marks a timestamp; midnight and an out-of-range serial still mark a date, and a mix promotes the column to TimestampColumn. --- .../Reader/Schema/SchemaInference.cs | 4 +- .../Reader/SchemaInferenceTests.cs | 61 +++++++++++++++++++ 2 files changed, 64 insertions(+), 1 deletion(-) diff --git a/src/ExcelReader.Core/Reader/Schema/SchemaInference.cs b/src/ExcelReader.Core/Reader/Schema/SchemaInference.cs index b5978362..2b2ca687 100644 --- a/src/ExcelReader.Core/Reader/Schema/SchemaInference.cs +++ b/src/ExcelReader.Core/Reader/Schema/SchemaInference.cs @@ -159,7 +159,9 @@ internal void Observe(in Cell cell, bool isDate1904, bool parseText) SawNonBinaryNumber |= !IsBinaryDigit(cell.Value); break; case CellType.Date: - SawDate = true; + bool hasTime = cell.TryGetDateTime(isDate1904, out DateTime date) && date.TimeOfDay != TimeSpan.Zero; + SawTimestamp |= hasTime; + SawDate |= !hasTime; break; case CellType.Boolean: SawBool = true; diff --git a/tests/ExcelReader.Tests/Reader/SchemaInferenceTests.cs b/tests/ExcelReader.Tests/Reader/SchemaInferenceTests.cs index d9ca2f9e..238019b8 100644 --- a/tests/ExcelReader.Tests/Reader/SchemaInferenceTests.cs +++ b/tests/ExcelReader.Tests/Reader/SchemaInferenceTests.cs @@ -2,6 +2,7 @@ using ExcelReader.Core.Reader.Csv; using ExcelReader.Core.Reader.Schema; using ExcelReader.Core.Reader.Xlsx; +using ExcelReader.Core.Writer.Xlsx; namespace ExcelReader.Tests.Reader { @@ -228,6 +229,66 @@ public void Should_GuessOneTypePerColumn_When_EverySampledCellAgrees() Assert.Equal(new ExcelColumnSchema { Index = 2, Name = "Ratio", Type = ExcelColumnType.Float64Column, IsNullable = false }, schema[2]); } + private static ExcelColumnType InferDateStyledColumn(bool date1904, params string[] values) + { + const string Styles = """"""; + string rows = string.Concat(values.Select((value, i) => $"""{value}""")); + using MemoryStream ms = WorkbookBuilder.Build(rows, styles: Styles, date1904: date1904); + using XlsxWorkbook reader = Excel.FromXlsx(ms.ToArray()); + return Excel.InferSchema(reader.FirstSheet, new SchemaInferenceOptions { HeaderRow = 0 })[0].Type; + } + + [Theory] + [InlineData(false)] + [InlineData(true)] + public void Should_KeepADateColumn_When_EverySampledDateIsAtMidnight(bool date1904) + { + Assert.Equal(ExcelColumnType.DateColumn, InferDateStyledColumn(date1904, "45293", "45294")); + } + + [Theory] + [InlineData(false)] + [InlineData(true)] + public void Should_InferATimestamp_When_ASampledDateCarriesATimeOfDay(bool date1904) + { + Assert.Equal(ExcelColumnType.TimestampColumn, InferDateStyledColumn(date1904, "45293.520833333336")); + } + + [Fact] + public void Should_PromoteToTimestamp_When_DatesAndTimestampsMix() + { + Assert.Equal(ExcelColumnType.TimestampColumn, InferDateStyledColumn(false, "45293", "45293.5", "45294")); + } + + [Fact] + public void Should_KeepADateColumn_When_ADateStyledSerialIsOutOfRange() + { + Assert.Equal(ExcelColumnType.DateColumn, InferDateStyledColumn(false, "45293", "1E+300")); + } + + [Fact] + public void Should_InferATimestamp_When_ADateTimeIsWrittenThroughTheFacade() + { + using var ms = new MemoryStream(); + using (XlsxWorkbookWriter wb = XlsxWorkbookWriter.Create(ms, leaveOpen: true)) + { + XlsxSheetWriter sheet = wb.AddSheet("S"); + using (XlsxRowWriter row = sheet.StartRow()) + { + row.Write("When"); + } + using (XlsxRowWriter row = sheet.StartRow()) + { + row.Write(new DateTime(2024, 1, 2, 12, 30, 0)); + } + sheet.End(); + wb.End(); + } + using XlsxWorkbook reader = Excel.FromXlsx(ms.ToArray()); + + Assert.Equal(ExcelColumnType.TimestampColumn, Excel.InferSchema(reader.FirstSheet)[0].Type); + } + [Fact] public void Should_FallBackToString_When_SampledCellsMixKinds() { From 57683c3ecf0405a86f81e90281186be1b106c156 Mon Sep 17 00:00:00 2001 From: Gabriel Matte Date: Wed, 21 Oct 2026 11:19:13 -0300 Subject: [PATCH 20/34] fix(reader): resolve the Column{i} names SheetDataReader synthesizes GetName returned Column{i} for a column without a header, but GetOrdinal only knew real headers, so GetOrdinal(GetName(0)) threw on a headerless sheet. BuildOrdinals now adds the synthesized names too; a real header of the same name keeps its ordinal. --- .../Reader/SheetDataReader.cs | 7 ++++++ .../Reader/SheetDataReaderTests.cs | 25 +++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/src/ExcelReader.Core/Reader/SheetDataReader.cs b/src/ExcelReader.Core/Reader/SheetDataReader.cs index 95cfee9c..d42127db 100644 --- a/src/ExcelReader.Core/Reader/SheetDataReader.cs +++ b/src/ExcelReader.Core/Reader/SheetDataReader.cs @@ -110,6 +110,13 @@ private static Dictionary BuildOrdinals(string?[] names) map[name] = i; } } + for (int i = 0; i < names.Length; i++) + { + if (names[i] is null) + { + map.TryAdd($"Column{i}", i); + } + } return map; } diff --git a/tests/ExcelReader.Tests/Reader/SheetDataReaderTests.cs b/tests/ExcelReader.Tests/Reader/SheetDataReaderTests.cs index e725b8f9..c9d6c178 100644 --- a/tests/ExcelReader.Tests/Reader/SheetDataReaderTests.cs +++ b/tests/ExcelReader.Tests/Reader/SheetDataReaderTests.cs @@ -89,6 +89,31 @@ public async Task HeaderRowBeyondSheetSizeIsAnEmptyResultSet() Assert.False(data.Read()); } + [Fact] + public async Task GetOrdinalResolvesTheNamesGetNameSynthesizes() + { + await using var ms = await TypedWorkbook.BuildAsync([7, "hello"]); + await using IExcelWorkbook reader = await Excel.OpenAsync(ms, ct: TestContext.Current.CancellationToken); + using var data = new SheetDataReader(reader.FirstSheet, new SheetDataReaderOptions { HeaderRow = 0 }); + + Assert.Equal(0, data.GetOrdinal(data.GetName(0))); + Assert.Equal(1, data.GetOrdinal("column1")); + } + + [Fact] + public async Task GetOrdinalPrefersARealHeaderOverASynthesizedNameItCollidesWith() + { + await using var ms = await TypedWorkbook.BuildAsync( + ["Id", null, "Column1", null, "Last"], + [1, 2, 3, 4, 5]); + await using IExcelWorkbook reader = await Excel.OpenAsync(ms, ct: TestContext.Current.CancellationToken); + using var data = new SheetDataReader(reader.FirstSheet); + + Assert.Equal("Column1", data.GetName(1)); + Assert.Equal(2, data.GetOrdinal("Column1")); + Assert.Equal(3, data.GetOrdinal(data.GetName(3))); + } + [Fact] public async Task GetOrdinalForUnknownNameThrows() { From 19dd52fb19a932ee4da06e45932893e9064df2b1 Mon Sep 17 00:00:00 2001 From: Gabriel Matte Date: Wed, 21 Oct 2026 11:19:50 -0300 Subject: [PATCH 21/34] fix(cpp): read a valid empty string as empty, not as the field's default assign_field only assigned a string when end > start, so a valid empty cell left std::string name = "fallback" holding "fallback". A valid empty value now assigns T{}; a null still leaves the default, as documented. --- cpp/tests/write.cpp | 33 +++++++++++++++++++ .../include/excelreader.hpp | 8 +++-- 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/cpp/tests/write.cpp b/cpp/tests/write.cpp index b1d8d0b9..f5350b71 100644 --- a/cpp/tests/write.cpp +++ b/cpp/tests/write.cpp @@ -45,6 +45,21 @@ struct xl::ExcelMapper } }; +struct NamedRow +{ + std::string name = "fallback"; + std::string_view view = "fallback"; +}; + +template <> +struct xl::ExcelMapper +{ + static constexpr auto get_bindings() + { + return std::make_tuple(xl::make_field("name", &NamedRow::name), xl::make_field("view", &NamedRow::view)); + } +}; + static std::filesystem::path temp_path(std::string_view name) { return std::filesystem::temp_directory_path() / @@ -711,8 +726,26 @@ static int test_writer_handle_class_rejects_auto_for_memory() return 0; } +static int test_a_valid_empty_string_replaces_the_default() +{ + const int32_t offsets[] = {0, 0}; + xl_column columns[] = { + xl_column{XL_T_STRING, 1, offsets, nullptr, nullptr, 0}, + xl_column{XL_T_STRING, 1, offsets, nullptr, nullptr, 0}, + }; + const xl_table table{2, 1, columns}; + const NamedRow row = xl::detail::row_at(table, 0); + CHECK(row.name.empty(), "an empty std::string must overwrite the default"); + CHECK(row.view.empty(), "an empty std::string_view must overwrite the default"); + return 0; +} + int main() { + if (test_a_valid_empty_string_replaces_the_default() != 0) + { + return 1; + } if (test_write_options() != 0) { return 1; diff --git a/src/ExcelReader.Native/include/excelreader.hpp b/src/ExcelReader.Native/include/excelreader.hpp index 447da5a7..4b1aba3a 100644 --- a/src/ExcelReader.Native/include/excelreader.hpp +++ b/src/ExcelReader.Native/include/excelreader.hpp @@ -1526,11 +1526,13 @@ namespace xl const int32_t *offsets = static_cast(col.values); int32_t start = offsets[row]; int32_t end = offsets[row + 1]; - if (end > start && col.data != nullptr) + if (end <= start || col.data == nullptr) { - const char *str_data = reinterpret_cast(col.data) + start; - instance.*(binding.member) = T(str_data, static_cast(end - start)); + instance.*(binding.member) = T{}; + return; } + const char *str_data = reinterpret_cast(col.data) + start; + instance.*(binding.member) = T(str_data, static_cast(end - start)); } else if constexpr (std::is_same_v) { From cde17b2f03d211e4e238394b9b1949e3c7b3356c Mon Sep 17 00:00:00 2001 From: Gabriel Matte Date: Wed, 21 Oct 2026 11:21:57 -0300 Subject: [PATCH 22/34] fix(cpp): refuse integers a field or an int64 cell cannot represent assign_field cast an int64 column value straight into the field, so 300 read into a uint8_t became 44, and the writers cast every integer to int64_t, so a uint64_t above INT64_MAX was written as a negative number. parse and TypedReader::next now check each integer field's column once, when the table is handed over, and return XL_INVALID_ARGUMENT naming the column and row; TableView::operator[] keeps returning T. write_sheet and WriterHandle::write refuse a value above INT64_MAX. --- cpp/tests/write.cpp | 98 +++++++++++++++++++ .../include/excelreader.hpp | 73 +++++++++++++- 2 files changed, 169 insertions(+), 2 deletions(-) diff --git a/cpp/tests/write.cpp b/cpp/tests/write.cpp index f5350b71..337decbe 100644 --- a/cpp/tests/write.cpp +++ b/cpp/tests/write.cpp @@ -45,6 +45,28 @@ struct xl::ExcelMapper } }; +struct NarrowRow +{ + uint8_t small; +}; + +template <> +struct xl::ExcelMapper +{ + static constexpr auto get_bindings() { return std::make_tuple(xl::make_field("small", &NarrowRow::small)); } +}; + +struct WideRow +{ + uint64_t big; +}; + +template <> +struct xl::ExcelMapper +{ + static constexpr auto get_bindings() { return std::make_tuple(xl::make_field("big", &WideRow::big)); } +}; + struct NamedRow { std::string name = "fallback"; @@ -726,6 +748,74 @@ static int test_writer_handle_class_rejects_auto_for_memory() return 0; } +static std::expected, xl::Error> parse_narrow(const std::vector &values, + std::vector &bytes) +{ + std::array columns{xl::i64_column("small", values)}; + if (!xl::write_columns(bytes, columns, {.format = XL_FORMAT_XLSX}).has_value()) + { + return std::unexpected(xl::Error{XL_ERROR, "write_columns failed"}); + } + auto workbook = xl::Workbook::open(std::span(bytes)); + if (!workbook.has_value()) + { + return std::unexpected(workbook.error()); + } + auto sheets = workbook->sheets(); + if (!sheets.has_value()) + { + return std::unexpected(sheets.error()); + } + return xl::parse(sheets->front()); +} + +static int test_narrow_field_rejects_a_value_it_cannot_hold() +{ + std::vector bytes; + auto too_big = parse_narrow({1, 300}, bytes); + CHECK(!too_big.has_value(), "300 must not be read into a uint8_t"); + CHECK(too_big.error().code == XL_INVALID_ARGUMENT, "an out-of-range value is an invalid argument"); + CHECK(too_big.error().message.find("small") != std::string::npos, "the error must name the column"); + + auto negative = parse_narrow({-1}, bytes); + CHECK(!negative.has_value(), "-1 must not be read into a uint8_t"); + + auto limits = parse_narrow({0, 255}, bytes); + CHECK(limits.has_value(), "values at the uint8_t limits must parse"); + CHECK((*limits)[0].small == 0 && (*limits)[1].small == 255, "values at the limits must round-trip"); + + (void)parse_narrow({300}, bytes); + auto workbook = xl::Workbook::open(std::span(bytes)); + CHECK(workbook.has_value(), "the written bytes must open"); + auto batches = xl::parse_batches(workbook->sheets()->front()); + CHECK(batches.has_value(), "parse_batches must open"); + auto batch = batches->next(); + CHECK(!batch.has_value(), "a batch holding 300 must fail for a uint8_t field"); + return 0; +} + +static int test_unsigned_values_past_int64_are_refused_on_write() +{ + std::vector bytes; + const std::vector rows{{static_cast(INT64_MAX) + 1}}; + auto written = xl::write_sheet(bytes, rows, {.format = XL_FORMAT_XLSX}); + CHECK(!written.has_value(), "a uint64_t above INT64_MAX must not be written as a negative number"); + + const std::vector limit{{static_cast(INT64_MAX)}}; + CHECK(xl::write_sheet(bytes, limit, {.format = XL_FORMAT_XLSX}).has_value(), "INT64_MAX must be written"); + auto workbook = xl::Workbook::open(std::span(bytes)); + CHECK(workbook.has_value(), "the written bytes must open"); + auto table = xl::parse(workbook->sheets()->front()); + CHECK(table.has_value() && (*table)[0].big == static_cast(INT64_MAX), "INT64_MAX must round-trip"); + + std::vector streamed; + auto handle = xl::WriterHandle::open(streamed, {.format = XL_FORMAT_XLSX}); + CHECK(handle.has_value(), "the writer handle must open"); + CHECK(handle->start_sheet("S").has_value() && handle->start_row().has_value(), "the sheet and row must start"); + CHECK(!handle->write(UINT64_MAX).has_value(), "WriterHandle::write must refuse a uint64_t above INT64_MAX"); + return 0; +} + static int test_a_valid_empty_string_replaces_the_default() { const int32_t offsets[] = {0, 0}; @@ -742,6 +832,14 @@ static int test_a_valid_empty_string_replaces_the_default() int main() { + if (test_narrow_field_rejects_a_value_it_cannot_hold() != 0) + { + return 1; + } + if (test_unsigned_values_past_int64_are_refused_on_write() != 0) + { + return 1; + } if (test_a_valid_empty_string_replaces_the_default() != 0) { return 1; diff --git a/src/ExcelReader.Native/include/excelreader.hpp b/src/ExcelReader.Native/include/excelreader.hpp index 4b1aba3a..e4736380 100644 --- a/src/ExcelReader.Native/include/excelreader.hpp +++ b/src/ExcelReader.Native/include/excelreader.hpp @@ -1502,6 +1502,10 @@ namespace xl return (col.validity[byte_idx] & (1 << bit_idx)) != 0; } + /// The standard integer type `std::in_range` accepts in place of a character type. + template + using standard_integer_t = std::conditional_t, std::make_signed_t, std::make_unsigned_t>; + template void assign_field(Class &instance, const xl_column &col, int64_t row, const FieldBinding &binding) { @@ -1590,6 +1594,43 @@ namespace xl return instance; } + template + std::optional check_range(const xl_column &col, int64_t rows, const FieldBinding &binding) + { + using Field = unwrap_optional_t; + if constexpr (std::is_integral_v && !std::is_same_v) + { + const int64_t *values = static_cast(col.values); + for (int64_t row = 0; row < rows; ++row) + { + if (is_valid(col, row) && !std::in_range>(values[row])) + { + return Error{XL_INVALID_ARGUMENT, "column '" + std::string(binding.column_names[0]) + "' holds " + + std::to_string(values[row]) + " at row " + std::to_string(row) + + ", which its field's type cannot represent"}; + } + } + } + return std::nullopt; + } + + template + std::optional check_ranges(const xl_table &table, const Tuple &bindings, std::index_sequence) + { + std::optional problem; + (void)(... || (problem = check_range(table.columns[Is], table.row_count, std::get(bindings))).has_value()); + return problem; + } + + /// TableView converts on access and returns `T`, so a value the row type cannot hold is + /// refused once, when the table is handed over, instead of wrapping on every read. + template + std::optional check_ranges(const xl_table &table) + { + static constexpr auto bindings = ExcelMapper::get_bindings(); + return check_ranges(table, bindings, std::make_index_sequence>{}); + } + template int32_t with_specs(const Sheet &sheet, Call &&call) { @@ -1785,7 +1826,12 @@ namespace xl { return std::unexpected(detail::make_error(status)); } - return detail::Access::make>(table); + TableView view = detail::Access::make>(table); + if (std::optional problem = detail::check_ranges(table)) + { + return std::unexpected(std::move(*problem)); + } + return view; } @@ -1829,7 +1875,12 @@ namespace xl { return std::unexpected(detail::make_error(status)); } - return std::optional>(TableView(table)); + TableView view(table); + if (std::optional problem = detail::check_ranges(table)) + { + return std::unexpected(std::move(*problem)); + } + return std::optional>(std::move(view)); } class iterator @@ -2112,6 +2163,7 @@ namespace xl typename ColumnStorage::type storage{}; std::vector validity{}; int64_t rows = 0; + bool out_of_range = false; void reserve(size_t count) { @@ -2214,6 +2266,7 @@ namespace xl } else if constexpr (std::is_integral_v) { + out_of_range |= !std::in_range(static_cast>(value)); storage.push_back(static_cast(value)); } else if constexpr (std::is_floating_point_v) @@ -2272,6 +2325,12 @@ namespace xl return (... || std::get(builders).overflowed()); } + template + bool any_out_of_range(const Builders &builders, std::index_sequence) + { + return (... || std::get(builders).out_of_range); + } + template std::array to_refs(const Builders &builders, const Tuple &bindings, std::index_sequence) @@ -2306,6 +2365,11 @@ namespace xl return std::unexpected(Error{XL_INVALID_ARGUMENT, "a string column exceeds 2 GiB, which int32 offsets cannot address."}); } + if (any_out_of_range(builders, indices)) + { + return std::unexpected(Error{XL_INVALID_ARGUMENT, "an integer field holds a value above INT64_MAX, " + "which an int64 column cannot represent."}); + } // refs borrows pointers into builders, so both must stay in the scope of the write call. const std::array refs = to_refs(builders, bindings, indices); @@ -2426,6 +2490,11 @@ namespace xl } else if constexpr (std::is_integral_v) { + if (!std::in_range(static_cast>(value))) + { + return std::unexpected(Error{XL_INVALID_ARGUMENT, "the value is above INT64_MAX, which an int64 " + "cell cannot represent."}); + } return status_result(xl_write_int64(handle_, static_cast(value))); } else if constexpr (std::is_floating_point_v) From 81af9bfb6107ec099eb0d2828a4b41b68b04362f Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Wed, 21 Oct 2026 20:06:43 +0000 Subject: [PATCH 23/34] build(deps): bump syn from 3.0.3 to 3.0.9 in /rust Bumps [syn](https://github.com/dtolnay/syn) from 3.0.3 to 3.0.9. - [Release notes](https://github.com/dtolnay/syn/releases) - [Commits](https://github.com/dtolnay/syn/compare/3.0.3...3.0.9) --- updated-dependencies: - dependency-name: syn dependency-version: 3.0.9 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] --- rust/Cargo.lock | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/rust/Cargo.lock b/rust/Cargo.lock index 926a59f3..7c402e45 100644 --- a/rust/Cargo.lock +++ b/rust/Cargo.lock @@ -553,7 +553,7 @@ version = "0.0.0" dependencies = [ "proc-macro2", "quote", - "syn 3.0.3", + "syn 3.0.9", ] [[package]] @@ -1034,7 +1034,7 @@ checksum = "e7a5d71263a5a7d47b41f6b3f06ba276f10cc18b0931f1799f710578e2309348" dependencies = [ "proc-macro2", "quote", - "syn 3.0.3", + "syn 3.0.9", ] [[package]] @@ -1090,9 +1090,9 @@ dependencies = [ [[package]] name = "syn" -version = "3.0.3" +version = "3.0.9" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "53e9bae58849f64dfa4f5d5ae372c8341f7305f82a3868709269343628b659a3" +checksum = "d78c8dee4c7bf0e14673097256fed6142ce9d3b85a408189d07482442145823b" dependencies = [ "proc-macro2", "quote", From ca5fb755d1c51a621af9c01d6bc8ff1541f30504 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Wed, 21 Oct 2026 20:06:47 +0000 Subject: [PATCH 24/34] build(deps): bump rust_xlsxwriter from 0.99.1 to 0.100.0 in /rust Bumps [rust_xlsxwriter](https://github.com/jmcnamara/rust_xlsxwriter) from 0.99.1 to 0.100.0. - [Changelog](https://github.com/jmcnamara/rust_xlsxwriter/blob/main/CHANGELOG.md) - [Commits](https://github.com/jmcnamara/rust_xlsxwriter/compare/v0.99.1...v0.100.0) --- updated-dependencies: - dependency-name: rust_xlsxwriter dependency-version: 0.100.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] --- rust/Cargo.lock | 41 +++++++++++++++++++++++++++++++++---- rust/excelreader/Cargo.toml | 2 +- 2 files changed, 38 insertions(+), 5 deletions(-) diff --git a/rust/Cargo.lock b/rust/Cargo.lock index 926a59f3..afcbab31 100644 --- a/rust/Cargo.lock +++ b/rust/Cargo.lock @@ -307,7 +307,7 @@ dependencies = [ "log", "quick-xml", "serde", - "zip", + "zip 8.6.0", ] [[package]] @@ -332,6 +332,12 @@ version = "1.0.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9330f8b2ff13f34540b44e946ef35111825727b38d33286ef986142615121801" +[[package]] +name = "cfg_aliases" +version = "0.2.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f079e83a288787bcd14a6aea84cee5c87a67c5a3e660c30f557a3d24761b3527" + [[package]] name = "chrono" version = "0.4.45" @@ -800,6 +806,18 @@ dependencies = [ "simd-adler32", ] +[[package]] +name = "nix" +version = "0.31.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cf20d2fde8ff38632c426f1165ed7436270b44f199fc55284c38276f9db47c3d" +dependencies = [ + "bitflags", + "cfg-if", + "cfg_aliases", + "libc", +] + [[package]] name = "num-bigint" version = "0.5.1" @@ -979,11 +997,11 @@ checksum = "d6f6ff9a378485b298a5286656da665ba74413d36db0979633275d2e708145d4" [[package]] name = "rust_xlsxwriter" -version = "0.99.1" +version = "0.100.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "34f2d2cda7223168a14fb86bc7411dbc3379bd7d49749ecc53b91ed98585b6b0" +checksum = "c44904ffb6c57b148ffcc8ce3011c28bc49b97e1a8343b2d632283cd566e1393" dependencies = [ - "zip", + "zip 9.0.5", ] [[package]] @@ -1430,6 +1448,21 @@ dependencies = [ "zopfli", ] +[[package]] +name = "zip" +version = "9.0.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6e04976da629d3b04204ba8105b396a193af8ad2583b2c79bfb3ab8af580a11e" +dependencies = [ + "crc32fast", + "flate2", + "indexmap", + "memchr", + "nix", + "typed-path", + "zopfli", +] + [[package]] name = "zlib-rs" version = "0.6.7" diff --git a/rust/excelreader/Cargo.toml b/rust/excelreader/Cargo.toml index f8c8b5fb..bf0e9942 100644 --- a/rust/excelreader/Cargo.toml +++ b/rust/excelreader/Cargo.toml @@ -26,7 +26,7 @@ cc = "1" trybuild = "1" criterion = "0.8" calamine = { version = "0.36.1", features = ["chrono"] } -rust_xlsxwriter = "0.99.1" +rust_xlsxwriter = "0.100.0" [[bench]] name = "parse_bench" From 11aec057e0e5d5b892c233464af44310c00a2bef Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Wed, 21 Oct 2026 20:06:51 +0000 Subject: [PATCH 25/34] build(deps): bump cc from 1.4.4 to 1.8.0 in /rust Bumps [cc](https://github.com/rust-lang/cc-rs) from 1.4.4 to 1.8.0. - [Release notes](https://github.com/rust-lang/cc-rs/releases) - [Changelog](https://github.com/rust-lang/cc-rs/blob/main/CHANGELOG.md) - [Commits](https://github.com/rust-lang/cc-rs/compare/cc-v1.4.4...cc-v1.8.0) --- updated-dependencies: - dependency-name: cc dependency-version: 1.8.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] --- rust/Cargo.lock | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/rust/Cargo.lock b/rust/Cargo.lock index 926a59f3..6911aad7 100644 --- a/rust/Cargo.lock +++ b/rust/Cargo.lock @@ -318,9 +318,9 @@ checksum = "37b2a672a2cb129a2e41c10b1224bb368f9f37a2b16b612598138befd7b37eb5" [[package]] name = "cc" -version = "1.4.4" +version = "1.8.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0ad534f4357a5264cce5019c989cf66a4f0dc4e0d1b1d15f8aacec0ff7360273" +checksum = "6651c9ed80effdc7db0ff72512157f901af5e3549e341e24b1dd4887d836d838" dependencies = [ "find-msvc-tools", "shlex", @@ -564,9 +564,9 @@ checksum = "c6e8948ce679d00a02a94739ea185595dca7118ed04feb991127e443bd3d761f" [[package]] name = "find-msvc-tools" -version = "0.1.11" +version = "0.1.15" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d45db016d36b838f563236e9193d0ee6ce38f3f68b6c94e914b4929c96bbb890" +checksum = "1c66915cc5391097de7a2b0b972e8505aa33c0df401fd3a6811dbdb32881ba48" [[package]] name = "flate2" From 631ff31a3d0078a7604e37a2e36ee3a7d4747b01 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Wed, 21 Oct 2026 20:06:56 +0000 Subject: [PATCH 26/34] build(deps): bump trybuild from 1.0.120 to 1.0.122 in /rust Bumps [trybuild](https://github.com/dtolnay/trybuild) from 1.0.120 to 1.0.122. - [Release notes](https://github.com/dtolnay/trybuild/releases) - [Commits](https://github.com/dtolnay/trybuild/compare/1.0.120...1.0.122) --- updated-dependencies: - dependency-name: trybuild dependency-version: 1.0.122 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] --- rust/Cargo.lock | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/rust/Cargo.lock b/rust/Cargo.lock index 926a59f3..c53bc5ec 100644 --- a/rust/Cargo.lock +++ b/rust/Cargo.lock @@ -1100,10 +1100,10 @@ dependencies = [ ] [[package]] -name = "target-triple" -version = "1.0.1" +name = "target-tuple" +version = "1.0.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c3a6bfce3d99adfa72d24750a61f782f3036a81e7f86d8841ee1326deaebd171" +checksum = "876fef147edbcbddc8ac5cbbba92c7b86519e314e86638596c09673b2ed01e7f" [[package]] name = "termcolor" @@ -1174,15 +1174,15 @@ checksum = "7d56353a2a665ad0f41a421187180aab746c8c325620617ad883a99a1cbe66d2" [[package]] name = "trybuild" -version = "1.0.120" +version = "1.0.122" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1e605bf6b39357663d8ba4e984f8be8da8df6bb32e81031d6889024ea8fd68e4" +checksum = "62db9c92d704393fbf2132041720cc80b689f2d3f28521015c2ac866223c11b8" dependencies = [ "glob", "serde", "serde_derive", "serde_json", - "target-triple", + "target-tuple", "termcolor", "toml", ] From 3ea5d83567b59b677691f1db128fd6d55177ffa4 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Wed, 21 Oct 2026 20:07:37 +0000 Subject: [PATCH 27/34] Bump the analyzers group with 1 update Bumps Meziantou.Analyzer from 3.0.303 to 3.0.315 --- updated-dependencies: - dependency-name: Meziantou.Analyzer dependency-version: 3.0.315 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: analyzers ... Signed-off-by: dependabot[bot] --- Directory.Build.props | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Directory.Build.props b/Directory.Build.props index 0e6a30ac..94839ae4 100644 --- a/Directory.Build.props +++ b/Directory.Build.props @@ -27,7 +27,7 @@ runtime; build; native; contentfiles; analyzers; buildtransitive all - + runtime; build; native; contentfiles; analyzers; buildtransitive all From f432029c10543906f68ce1085ae64bbfbcd31d3b Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Wed, 21 Oct 2026 20:08:46 +0000 Subject: [PATCH 28/34] Bump the benchmarks group with 1 update Bumps OfficeIMO.Excel from 3.4.5 to 3.5.1 --- updated-dependencies: - dependency-name: OfficeIMO.Excel dependency-version: 3.5.1 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: benchmarks ... Signed-off-by: dependabot[bot] --- tests/ExcelReader.Benchmarks/ExcelReader.Benchmarks.csproj | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/ExcelReader.Benchmarks/ExcelReader.Benchmarks.csproj b/tests/ExcelReader.Benchmarks/ExcelReader.Benchmarks.csproj index aa391429..c45e5851 100644 --- a/tests/ExcelReader.Benchmarks/ExcelReader.Benchmarks.csproj +++ b/tests/ExcelReader.Benchmarks/ExcelReader.Benchmarks.csproj @@ -26,7 +26,7 @@ - + From ef6d5fab613943f023821f98f4697512a2367b36 Mon Sep 17 00:00:00 2001 From: Gabriel Matte Date: Wed, 21 Oct 2026 18:06:03 -0300 Subject: [PATCH 29/34] feat(rust): read a sheet through a schema chosen at run time infer_schema returned columns nothing could consume: every read needed a compile-time ExcelMapper type. Sheet now takes a slice of InferredColumn in parse_with_schema, parse_batches_with_schema, to_arrow_with_schema and to_arrow_stream_with_schema. A column with a name is matched by it, one without by its index. The non-Arrow reads return a Table whose columns are the Column values write_columns takes, so a table writes back as it was read. --- rust/excelreader/README.md | 50 +++++ rust/excelreader/src/arrow.rs | 51 ++++- rust/excelreader/src/lib.rs | 4 +- rust/excelreader/src/workbook.rs | 267 ++++++++++++++++++++++++- rust/excelreader/tests/parse_arrow.rs | 68 +++++++ rust/excelreader/tests/parse_typed.rs | 100 ++++++++- rust/excelreader/tests/typed_reader.rs | 57 +++++- 7 files changed, 583 insertions(+), 14 deletions(-) diff --git a/rust/excelreader/README.md b/rust/excelreader/README.md index 7d88134d..4dc6833c 100644 --- a/rust/excelreader/README.md +++ b/rust/excelreader/README.md @@ -103,6 +103,34 @@ for column in sheet.infer_schema(1, 100, false)? { `infer_schema`'s last argument, `parse_text`, also types text cells (every CSV field, or numbers stored as text): integers, decimals, `true`/`false` and ISO dates. +With no row type, `parse_with_schema` reads the sheet through that schema into a `Table`. Its +columns are the `Column`s `write_columns` takes, so the table writes back as it was read: + +```rust,no_run +use excelreader::{write_columns, ColumnData, ReadOptions, Workbook, WriteOptions}; + +let workbook = Workbook::open("book.xlsx")?; +let sheet = workbook.sheet(0)?; +let schema = sheet.infer_schema(1, 100, false)?; +let table = sheet.parse_with_schema(&schema, &ReadOptions::new())?; +for column in table.columns() { + if let ColumnData::I64(values) = column.data { + println!("{:?}: {} integers", column.name, values.len()); + } +} +write_columns("copy.xlsx", &table.columns(), &WriteOptions::new())?; + +// Or a batch at a time: TableBatches yields one Table per batch. +for batch in sheet.parse_batches_with_schema(&schema, &ReadOptions::new())? { + println!("{} rows", batch?.len()); +} +# Ok::<(), excelreader::Error>(()) +``` + +A column matched by index has no name, and writing needs every column named or none. The schema is +a guess over a sample: a column inferred as `I64` that holds a fraction further down fails the read, +unless the column is `nullable`. + `Workbook` is `Send + Sync`, so sheets can be read in parallel over one open workbook, which loads the shared-string table once: @@ -249,6 +277,28 @@ Same `batch_size` semantics as `parse_batches` below. Unlike `TypedBatches`, `ArrowBatches` does not fuse after an error — it follows arrow-rs's own semantics — so break on the first `Err` rather than continuing the loop. +With no row type, `to_arrow_with_schema` and `to_arrow_stream_with_schema` take the columns at run +time instead, such as the ones `infer_schema` returned. A column with a name is matched by it, one +without by its index: + +```rust,no_run +# #[cfg(feature = "arrow")] +# fn main() -> Result<(), excelreader::Error> { +use excelreader::{Workbook, ReadOptions}; + +let workbook = Workbook::open("book.xlsx")?; +let sheet = workbook.sheet(0)?; +let schema = sheet.infer_schema(1, 100, false)?; +let batch = sheet.to_arrow_with_schema(&schema, &ReadOptions::new())?; +# Ok(()) +# } +# #[cfg(not(feature = "arrow"))] +# fn main() {} +``` + +The schema is a guess over a sample: a column inferred as `I64` that holds a fraction further down +fails the read, unless the column is `nullable`. + ## Writing `#[derive(ExcelMapper)]` generates both halves, so the same struct reads and writes - the field diff --git a/rust/excelreader/src/arrow.rs b/rust/excelreader/src/arrow.rs index cbf54060..c1c9b668 100644 --- a/rust/excelreader/src/arrow.rs +++ b/rust/excelreader/src/arrow.rs @@ -13,16 +13,43 @@ use arrow::ffi::{from_ffi, FFI_ArrowArray, FFI_ArrowSchema}; use arrow::ffi_stream::{ArrowArrayStreamReader, FFI_ArrowArrayStream}; use arrow::record_batch::RecordBatchReader; -use crate::ffi; -use crate::workbook::{build_specs, check}; -use crate::{Error, ErrorKind, ExcelMapper, ReadOptions, Sheet, Workbook}; +use crate::ffi::{self, XlColumnSpec}; +use crate::workbook::{build_specs, check, schema_specs}; +use crate::{Error, ErrorKind, ExcelMapper, InferredColumn, ReadOptions, Sheet, Workbook}; impl<'w> Sheet<'w> { /// [`parse`](Self::parse) into one Arrow [`RecordBatch`], with the same `#[derive(ExcelMapper)]` /// mapping and [`ReadOptions`]. pub fn to_arrow(&self, options: &ReadOptions) -> Result { - let arena = build_specs::(); + self.arrow_from_specs(&build_specs::().specs, options) + } + + /// [`to_arrow`](Self::to_arrow) over a schema chosen at run time, such as one + /// [`infer_schema`](Self::infer_schema) returned. A column with a name is matched by it, one + /// without by its index; an empty schema is an error. + pub fn to_arrow_with_schema( + &self, + schema: &[InferredColumn], + options: &ReadOptions, + ) -> Result { + self.arrow_from_specs(&schema_specs(schema).specs, options) + } + + /// [`to_arrow_stream`](Self::to_arrow_stream) over a schema chosen at run time, as in + /// [`to_arrow_with_schema`](Self::to_arrow_with_schema). + pub fn to_arrow_stream_with_schema( + &self, + schema: &[InferredColumn], + options: &ReadOptions, + ) -> Result, Error> { + self.arrow_stream_from_specs(&schema_specs(schema).specs, options) + } + fn arrow_from_specs( + &self, + specs: &[XlColumnSpec], + options: &ReadOptions, + ) -> Result { let mut array = FFI_ArrowArray::empty(); let mut schema = FFI_ArrowSchema::empty(); @@ -30,8 +57,8 @@ impl<'w> Sheet<'w> { ffi::xl_parse_arrow( self.handle(), self.index(), - arena.specs.as_ptr(), - arena.specs.len() as i32, + specs.as_ptr(), + specs.len() as i32, options.header_row, options.parallelism, &mut array as *mut FFI_ArrowArray as *mut c_void, @@ -55,16 +82,22 @@ impl<'w> Sheet<'w> { &self, options: &ReadOptions, ) -> Result, Error> { - let arena = build_specs::(); + self.arrow_stream_from_specs(&build_specs::().specs, options) + } + fn arrow_stream_from_specs( + &self, + specs: &[XlColumnSpec], + options: &ReadOptions, + ) -> Result, Error> { let mut stream = FFI_ArrowArrayStream::empty(); check(unsafe { ffi::xl_parse_arrow_stream( self.handle(), self.index(), - arena.specs.as_ptr(), - arena.specs.len() as i32, + specs.as_ptr(), + specs.len() as i32, options.header_row, options.batch_size, &mut stream as *mut FFI_ArrowArrayStream as *mut c_void, diff --git a/rust/excelreader/src/lib.rs b/rust/excelreader/src/lib.rs index 30b915ca..fed98411 100644 --- a/rust/excelreader/src/lib.rs +++ b/rust/excelreader/src/lib.rs @@ -26,8 +26,8 @@ pub use rows::{AllRows, CellIter, CellRef, CellType, RowCursor, RowRef}; pub use source::Source; pub use temporal::{Date, Time, Timestamp}; pub use workbook::{ - ColumnType, ExcelMapper, InferredColumn, Sheet, SheetVisibility, TableView, TableViewIter, - TypedBatches, Workbook, + ColumnType, ExcelMapper, InferredColumn, Sheet, SheetVisibility, Table, TableBatches, + TableView, TableViewIter, TypedBatches, Workbook, }; pub use writer::{ encrypt_package, write_columns, write_columns_to, write_sheet, write_sheet_to, Column, diff --git a/rust/excelreader/src/workbook.rs b/rust/excelreader/src/workbook.rs index aa511cea..6148fb51 100644 --- a/rust/excelreader/src/workbook.rs +++ b/rust/excelreader/src/workbook.rs @@ -3,7 +3,8 @@ use crate::ffi::{ XL_BUFFER_TOO_SMALL, XL_EOF, XL_OK, }; use crate::{ - AllRows, Date, Error, ErrorKind, OpenOptions, ReadOptions, RowCursor, RowRef, Time, Timestamp, + AllRows, Column, ColumnData, Date, Error, ErrorKind, OpenOptions, ReadOptions, RowCursor, + RowRef, Time, Timestamp, }; use std::marker::PhantomData; use std::path::Path; @@ -410,6 +411,59 @@ impl<'w> Sheet<'w> { TableView::new(table, arena.bindings, "xl_parse_typed") } + /// [`parse`](Self::parse) over a schema chosen at run time, such as one + /// [`infer_schema`](Self::infer_schema) returned. A column with a name is matched by it, one + /// without by its index; an empty schema is an error. + pub fn parse_with_schema( + &self, + schema: &[InferredColumn], + options: &ReadOptions, + ) -> Result { + let specs = schema_specs(schema); + let mut table = empty_table(); + check(unsafe { + ffi::xl_parse_typed( + self.workbook.handle, + self.index, + specs.specs.as_ptr(), + specs.specs.len() as i32, + options.header_row, + options.parallelism, + &mut table, + ) + })?; + Table::new(table, schema, "xl_parse_typed") + } + + /// [`parse_with_schema`](Self::parse_with_schema) delivered [`ReadOptions`]'s batch size rows + /// at a time, as [`parse_batches`](Self::parse_batches) does. `schema` is copied, so it need not + /// outlive the batches. + pub fn parse_batches_with_schema( + &self, + schema: &[InferredColumn], + options: &ReadOptions, + ) -> Result, Error> { + let specs = schema_specs(schema); + let mut reader: *mut ffi::XlTypedReader = std::ptr::null_mut(); + check(unsafe { + ffi::xl_typed_reader_open( + self.workbook.handle, + self.index, + specs.specs.as_ptr(), + specs.specs.len() as i32, + options.header_row, + options.batch_size, + &mut reader, + ) + })?; + Ok(TableBatches { + reader, + schema: schema.to_vec(), + done: false, + _workbook: PhantomData, + }) + } + /// [`parse`](Self::parse) delivered [`ReadOptions`]'s batch size rows at a time; a negative /// batch size is an error. The header row is consumed once, here, not re-read per batch. pub fn parse_batches( @@ -863,6 +917,126 @@ impl std::fmt::Debug for TableView { } } +/// Owns a [`Sheet::parse_with_schema`] result. Frees the native table on `Drop`. +/// +/// Its columns are the [`Column`]s [`write_columns`](crate::write_columns) takes, borrowed from +/// this table. A column matched by index has no name, and writing needs every column named or none. +pub struct Table { + table: XlTable, + names: Vec>, +} + +impl Table { + /// Takes ownership of `table`, freeing it and failing if it does not have one column of a known + /// type and of `row_count` rows per `schema` entry. + fn new(mut table: XlTable, schema: &[InferredColumn], source: &str) -> Result { + let columns = if table.columns.is_null() { + &[][..] + } else { + unsafe { std::slice::from_raw_parts(table.columns, table.column_count.max(0) as usize) } + }; + let consistent = table.row_count >= 0 + && columns.len() == schema.len() + && columns + .iter() + .all(|c| c.length == table.row_count && ColumnType::from_raw(c.r#type).is_some()); + if !consistent { + unsafe { ffi::xl_free_table(&mut table) }; + return Err(Error::new( + ErrorKind::Other, + format!( + "{source} returned a table that does not match its {} specs", + schema.len() + ), + )); + } + Ok(Table { + table, + names: schema.iter().map(|c| c.name.clone()).collect(), + }) + } + + pub fn len(&self) -> i64 { + self.table.row_count + } + + pub fn is_empty(&self) -> bool { + self.table.row_count == 0 + } + + pub fn num_columns(&self) -> usize { + self.names.len() + } + + /// Column `index`, or `None` past the last one. + pub fn column(&self, index: usize) -> Option> { + let name = self.names.get(index)?; + let raw = unsafe { &*self.table.columns.add(index) }; + Some(Column { + name: name.as_deref(), + data: unsafe { column_data(raw) }, + validity: (!raw.validity.is_null()) + .then(|| unsafe { native_slice(raw.validity, (raw.length as usize).div_ceil(8)) }), + }) + } + + /// Every column, in schema order. + pub fn columns(&self) -> Vec> { + (0..self.num_columns()) + .filter_map(|i| self.column(i)) + .collect() + } +} + +/// # Safety +/// `ptr` must be null or point to `len` live `E`s that outlive `'a`. +unsafe fn native_slice<'a, E>(ptr: *const E, len: usize) -> &'a [E] { + if ptr.is_null() || len == 0 { + return &[]; + } + std::slice::from_raw_parts(ptr, len) +} + +/// # Safety +/// `raw` must be a column of a native table whose type [`Table::new`] checked. +unsafe fn column_data(raw: &XlColumn) -> ColumnData<'_> { + let rows = raw.length as usize; + match ColumnType::from_raw(raw.r#type) { + Some(ColumnType::String) => ColumnData::Str { + offsets: native_slice(raw.values.cast::(), rows + 1), + data: native_slice(raw.data, raw.data_len as usize), + }, + Some(ColumnType::I64) => ColumnData::I64(native_slice(raw.values.cast(), rows)), + Some(ColumnType::F64) => ColumnData::F64(native_slice(raw.values.cast(), rows)), + Some(ColumnType::Bool) => ColumnData::Bool(native_slice(raw.values.cast(), rows)), + Some(ColumnType::Date) => ColumnData::Date(native_slice(raw.values.cast(), rows)), + Some(ColumnType::Time) => ColumnData::Time(native_slice(raw.values.cast(), rows)), + Some(ColumnType::Timestamp) => ColumnData::Timestamp(native_slice(raw.values.cast(), rows)), + None => unreachable!("Table::new rejects unknown column types"), + } +} + +impl Drop for Table { + fn drop(&mut self) { + unsafe { + ffi::xl_free_table(&mut self.table); + } + } +} + +// SAFETY: as for TableView. +unsafe impl Send for Table {} +unsafe impl Sync for Table {} + +impl std::fmt::Debug for Table { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("Table") + .field("rows", &self.table.row_count) + .field("columns", &self.names) + .finish_non_exhaustive() + } +} + /// Iterator over the batches of one chunked typed read. Closes the native reader on `Drop`. pub struct TypedBatches<'a, T: ExcelMapper> { reader: *mut ffi::XlTypedReader, @@ -917,6 +1091,57 @@ impl std::fmt::Debug for TypedBatches<'_, T> { } } +/// [`TypedBatches`]' counterpart for [`Sheet::parse_batches_with_schema`], yielding [`Table`]s. +pub struct TableBatches<'a> { + reader: *mut ffi::XlTypedReader, + schema: Vec, + done: bool, + _workbook: PhantomData<&'a Workbook>, +} + +// SAFETY: as for TypedBatches. +unsafe impl Send for TableBatches<'_> {} + +impl Iterator for TableBatches<'_> { + type Item = Result; + + fn next(&mut self) -> Option { + if self.done { + return None; + } + + let mut table = empty_table(); + let status = unsafe { ffi::xl_typed_reader_next(self.reader, &mut table) }; + if status == XL_EOF { + self.done = true; + return None; + } + if status != XL_OK { + self.done = true; + return Some(Err(last_error(status))); + } + + let batch = Table::new(table, &self.schema, "xl_typed_reader_next"); + self.done = batch.is_err(); + Some(batch) + } +} + +impl Drop for TableBatches<'_> { + fn drop(&mut self) { + unsafe { ffi::xl_typed_reader_close(self.reader) }; + } +} + +impl std::fmt::Debug for TableBatches<'_> { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("TableBatches") + .field("columns", &self.schema.len()) + .field("done", &self.done) + .finish_non_exhaustive() + } +} + pub struct TableViewIter<'a, T: ExcelMapper> { view: &'a TableView, row: i64, @@ -984,3 +1209,43 @@ pub(crate) fn build_specs() -> SpecArena { _name_lens: name_lens, } } + +/// [`SpecArena`] for a schema chosen at run time: `specs` points into `schema`'s names and into the +/// two `_name_*` vectors, so all three must outlive the native call. +pub(crate) struct SchemaSpecs<'a> { + pub(crate) specs: Vec, + _name_ptrs: Vec<*const u8>, + _name_lens: Vec, + _schema: PhantomData<&'a [InferredColumn]>, +} + +/// Lowers a runtime schema into `xl_column_spec`s: a column with a name is matched by it, one +/// without by its index. +pub(crate) fn schema_specs(schema: &[InferredColumn]) -> SchemaSpecs<'_> { + let name_ptrs: Vec<*const u8> = schema + .iter() + .map(|c| c.name.as_deref().map_or(std::ptr::null(), str::as_ptr)) + .collect(); + let name_lens: Vec = schema + .iter() + .map(|c| c.name.as_deref().map_or(0, |n| n.len() as i32)) + .collect(); + let specs: Vec = schema + .iter() + .enumerate() + .map(|(i, c)| XlColumnSpec { + names: &name_ptrs[i], + name_lens: &name_lens[i], + name_count: i32::from(c.name.is_some()), + index: c.index, + r#type: c.column_type.raw(), + nullable: i32::from(c.nullable), + }) + .collect(); + SchemaSpecs { + specs, + _name_ptrs: name_ptrs, + _name_lens: name_lens, + _schema: PhantomData, + } +} diff --git a/rust/excelreader/tests/parse_arrow.rs b/rust/excelreader/tests/parse_arrow.rs index 698d0b13..77af7f48 100644 --- a/rust/excelreader/tests/parse_arrow.rs +++ b/rust/excelreader/tests/parse_arrow.rs @@ -174,3 +174,71 @@ fn parallel_parse_arrow_matches_the_sequential_row_count() { drop(workbook); let _ = std::fs::remove_file(path); } + +#[test] +fn an_inferred_schema_reads_into_arrow_without_a_row_type() { + let workbook = Workbook::open(fixture_path()).expect("open must succeed"); + let sheet = workbook.sheet(0).expect("sheet 0"); + let schema = sheet + .infer_schema(1, 100, false) + .expect("infer_schema must succeed"); + + let batch = sheet + .to_arrow_with_schema(&schema, &ReadOptions::new()) + .expect("to_arrow_with_schema must succeed"); + + assert_eq!(batch.num_columns(), schema.len()); + assert_eq!(batch.num_rows(), 100); + assert_eq!(batch.schema().field(0).name(), "Coluna1"); + let coluna3 = batch.column_by_name("Coluna3").expect("Coluna3 is read"); + assert!(coluna3.as_any().downcast_ref::().is_some()); +} + +#[test] +fn a_schema_without_names_resolves_columns_by_index() { + let workbook = Workbook::open(fixture_path()).expect("open must succeed"); + let sheet = workbook.sheet(0).expect("sheet 0"); + let schema = sheet + .infer_schema(0, 100, false) + .expect("infer_schema must succeed"); + assert!(schema.iter().all(|c| c.name.is_none())); + + let batch = sheet + .to_arrow_with_schema(&schema, &ReadOptions::new().header_row(0)) + .expect("to_arrow_with_schema must succeed"); + + assert_eq!(batch.num_columns(), schema.len()); + assert_eq!( + batch.num_rows(), + 101, + "with no header the header row is data" + ); +} + +#[test] +fn an_empty_schema_is_rejected() { + let workbook = Workbook::open(fixture_path()).expect("open must succeed"); + assert!(workbook + .sheet(0) + .expect("sheet 0") + .to_arrow_with_schema(&[], &ReadOptions::new()) + .is_err()); +} + +#[test] +fn an_inferred_schema_streams_every_row() { + let workbook = Workbook::open(fixture_path()).expect("open must succeed"); + let sheet = workbook.sheet(0).expect("sheet 0"); + let schema = sheet + .infer_schema(1, 100, false) + .expect("infer_schema must succeed"); + + let stream = sheet + .to_arrow_stream_with_schema(&schema, &ReadOptions::new().batch_size(8)) + .expect("the stream must open"); + assert_eq!(stream.schema().fields().len(), schema.len()); + let rows: usize = stream + .map(|batch| batch.expect("a batch must read").num_rows()) + .sum(); + assert_eq!(rows, 100); +} diff --git a/rust/excelreader/tests/parse_typed.rs b/rust/excelreader/tests/parse_typed.rs index 00032d05..33c0381c 100644 --- a/rust/excelreader/tests/parse_typed.rs +++ b/rust/excelreader/tests/parse_typed.rs @@ -1,4 +1,4 @@ -use excelreader::ReadOptions; +use excelreader::{write_columns_to, ColumnData, ReadOptions, Table, WriteOptions}; use excelreader::{ColumnType, Date, ExcelMapper, Format, OpenOptions, Workbook}; #[derive(Default, ExcelMapper)] @@ -355,3 +355,101 @@ fn parallel_parse_matches_the_sequential_parse() { drop(workbook); let _ = std::fs::remove_file(path); } + +fn column_by_name<'t>(table: &'t Table, name: &str) -> ColumnData<'t> { + table + .columns() + .into_iter() + .find(|c| c.name == Some(name)) + .unwrap_or_else(|| panic!("{name} must be read")) + .data +} + +#[test] +fn an_inferred_schema_parses_without_a_row_type() { + let workbook = open_fixture(); + let sheet = workbook.sheet(0).expect("sheet 0"); + let schema = sheet + .infer_schema(1, 100, false) + .expect("infer_schema must succeed"); + + let table = sheet + .parse_with_schema(&schema, &ReadOptions::new()) + .expect("parse_with_schema must succeed"); + + assert_eq!(table.len(), 100); + assert_eq!(table.num_columns(), schema.len()); + assert!(table.column(schema.len()).is_none()); + let ColumnData::Str { offsets, data } = column_by_name(&table, "Coluna1") else { + panic!("Coluna1 must be a string column"); + }; + assert_eq!(&data[offsets[0] as usize..offsets[1] as usize], b"Valor1"); + let ColumnData::I64(coluna3) = column_by_name(&table, "Coluna3") else { + panic!("Coluna3 must be an i64 column"); + }; + assert_eq!(coluna3.len(), 100); + assert_eq!(coluna3[0], 1); +} + +#[test] +fn a_schema_without_names_parses_by_index() { + let workbook = open_fixture(); + let sheet = workbook.sheet(0).expect("sheet 0"); + let schema = sheet + .infer_schema(0, 100, false) + .expect("infer_schema must succeed"); + + let table = sheet + .parse_with_schema(&schema, &ReadOptions::new().header_row(0)) + .expect("parse_with_schema must succeed"); + + assert_eq!(table.len(), 101, "with no header the header row is data"); + assert!(table.columns().iter().all(|c| c.name.is_none())); +} + +#[test] +fn parse_with_schema_rejects_an_empty_schema() { + let workbook = open_fixture(); + assert!(workbook + .sheet(0) + .expect("sheet 0") + .parse_with_schema(&[], &ReadOptions::new()) + .is_err()); +} + +#[test] +fn a_parsed_table_writes_back_unchanged() { + let workbook = open_fixture(); + let sheet = workbook.sheet(0).expect("sheet 0"); + let schema = sheet + .infer_schema(1, 100, false) + .expect("infer_schema must succeed"); + let table = sheet + .parse_with_schema(&schema, &ReadOptions::new()) + .expect("parse_with_schema must succeed"); + + let mut bytes = Vec::new(); + write_columns_to( + &mut bytes, + &table.columns(), + &WriteOptions::new().format(Format::Xlsx), + ) + .expect("a parsed table must write"); + + let reopened = OpenOptions::new() + .open_bytes(&bytes) + .expect("the written workbook must open"); + let again = reopened + .sheet(0) + .expect("sheet 0") + .parse_with_schema(&schema, &ReadOptions::new()) + .expect("the written workbook must parse with the same schema"); + assert_eq!(again.len(), table.len()); + let (ColumnData::I64(before), ColumnData::I64(after)) = ( + column_by_name(&table, "Coluna3"), + column_by_name(&again, "Coluna3"), + ) else { + panic!("Coluna3 must stay an i64 column"); + }; + assert_eq!(before, after); +} diff --git a/rust/excelreader/tests/typed_reader.rs b/rust/excelreader/tests/typed_reader.rs index 0a340a29..c0962bb5 100644 --- a/rust/excelreader/tests/typed_reader.rs +++ b/rust/excelreader/tests/typed_reader.rs @@ -1,5 +1,5 @@ use excelreader::ReadOptions; -use excelreader::{ErrorKind, ExcelMapper, TypedBatches, Workbook}; +use excelreader::{ColumnData, ErrorKind, ExcelMapper, TableBatches, TypedBatches, Workbook}; #[derive(Default, ExcelMapper)] struct Record { @@ -123,3 +123,58 @@ fn two_readers_on_one_workbook_each_yield_every_row() { assert_eq!(count(one), 100); assert_eq!(count(two), 100); } + +fn schema_batches(workbook: &Workbook, batch_size: i64) -> TableBatches<'_> { + let sheet = workbook.sheet(0).expect("sheet 0"); + let schema = sheet + .infer_schema(1, 100, false) + .expect("infer_schema must succeed"); + sheet + .parse_batches_with_schema(&schema, &ReadOptions::new().batch_size(batch_size)) + .expect("the batches must open") +} + +#[test] +fn schema_batches_equal_the_whole_sheet() { + let expected: Vec = whole_sheet().into_iter().map(|(_, n)| n).collect(); + for batch_size in [1, 7, 8, 9, 1000] { + let workbook = open(); + let mut coluna3 = Vec::new(); + for batch in schema_batches(&workbook, batch_size) { + let batch = batch.expect("a batch must read"); + assert!(batch.len() <= batch_size); + let column = batch + .columns() + .into_iter() + .find(|c| c.name == Some("Coluna3")) + .expect("every batch keeps the schema's names"); + let ColumnData::I64(values) = column.data else { + panic!("Coluna3 must stay an i64 column"); + }; + coluna3.extend_from_slice(values); + } + assert_eq!(coluna3, expected, "batch size {batch_size}"); + } +} + +#[test] +fn schema_batches_with_batch_size_zero_yield_one_batch() { + let workbook = open(); + let batches: Vec<_> = schema_batches(&workbook, 0) + .map(|b| b.expect("a batch must read")) + .collect(); + assert_eq!(batches.len(), 1); + assert_eq!(batches[0].len(), 100); +} + +#[test] +fn schema_batches_reject_a_negative_batch_size() { + let workbook = open(); + let sheet = workbook.sheet(0).expect("sheet 0"); + let schema = sheet + .infer_schema(1, 100, false) + .expect("infer_schema must succeed"); + assert!(sheet + .parse_batches_with_schema(&schema, &ReadOptions::new().batch_size(-1)) + .is_err()); +} From e1c508762ea7f50aa5f6325bd1e08b8ec23658d4 Mon Sep 17 00:00:00 2001 From: Gabriel Matte Date: Wed, 21 Oct 2026 18:06:04 -0300 Subject: [PATCH 30/34] feat(cpp): read a sheet through a schema chosen at run time infer_schema returned columns nothing could consume: every read needed a compile-time ExcelMapper type. parse, parse_batches, to_arrow and to_arrow_stream gain overloads taking a span of InferredColumn. A column with a name is matched by it, one without by its index. parse returns an xl::Table and parse_batches an xl::TableReader of them; a Table's columns are the ColumnRefs write_columns takes, so it writes back as it was read. TypedReader's iterator moves into detail::BatchIterator, shared by both readers; TypedReader::iterator remains as an alias. --- cpp/README.md | 27 ++ cpp/tests/arrow.cpp | 37 +++ cpp/tests/batch.cpp | 140 +++++++++ .../include/excelreader.hpp | 293 +++++++++++++++--- .../include/excelreader_arrow.hpp | 43 +++ 5 files changed, 499 insertions(+), 41 deletions(-) diff --git a/cpp/README.md b/cpp/README.md index a9c7ecc6..33aa3951 100644 --- a/cpp/README.md +++ b/cpp/README.md @@ -106,8 +106,23 @@ for (xl::Sheet sheet : *workbook->sheets()) { std::println("{}", sheet.name().va for (const auto& column : *workbook->sheet(1).infer_schema(1, 100)) { // column.name is nullopt when the column must be resolved by column.index instead. } + +// With no row type, parse the sheet through that schema. The columns are the xl::ColumnRef +// write_columns takes, so the table writes back as it was read. +auto schema = workbook->sheet(1).infer_schema(); +auto table = xl::parse(workbook->sheet(1), *schema); +for (const xl::ColumnRef& column : table->columns()) { /* column.type, column.values, ... */ } +auto written = xl::write_columns("copy.xlsx", table->columns()); + +// Or a batch at a time: an xl::TableReader, whose batches are xl::Tables. +auto reader = xl::parse_batches(workbook->sheet(1), *schema, {.batch_size = 10'000}); +for (auto& batch : *reader) { if (!batch) { break; } /* batch->columns() */ } ``` +A column matched by index has no name, and writing needs every column named or none. The schema is +a guess over a sample: a column inferred as `XL_T_I64` that holds a fraction further down fails the +read, unless the column is `nullable`. + Every entry point returns `std::expected` — this header throws nothing. A workbook hands out sheets. An `xl::Sheet` is the workbook handle and an index: it owns nothing, @@ -299,6 +314,18 @@ while (true) Same `header_row`/`batch_size` meaning as `xl::parse_batches` above. +With no row type, both take the columns at run time instead, such as the ones `infer_schema` +returned. A column with a name is matched by it, one without by its index: + +```cpp +auto schema = workbook->sheet(0).infer_schema(); +auto table = xl::to_arrow(workbook->sheet(0), *schema); +auto stream = xl::to_arrow_stream(workbook->sheet(0), *schema, {.batch_size = 10'000}); +``` + +The schema is a guess over a sample: a column inferred as `XL_T_I64` that holds a fraction further +down fails the read, unless the column is `nullable`. + ## Writing Two layers, mirroring the two on the reading side. diff --git a/cpp/tests/arrow.cpp b/cpp/tests/arrow.cpp index 73639841..d7879a68 100644 --- a/cpp/tests/arrow.cpp +++ b/cpp/tests/arrow.cpp @@ -199,6 +199,43 @@ int main() CHECK(reader.has_value(), "a typed reader opens while an Arrow stream is live"); } + { + auto wb = xl::Workbook::open(EXCELREADER_FIXTURE_PATH, options); + CHECK(wb.has_value(), "opening a workbook for the inferred-schema test must succeed"); + auto schema = wb->sheet(0).infer_schema(); + CHECK(schema.has_value() && !schema->empty(), "infer_schema must find columns"); + + auto inferred = xl::to_arrow(wb->sheet(0), *schema); + CHECK(inferred.has_value(), "to_arrow over an inferred schema must succeed"); + CHECK(inferred->schema.n_children == static_cast(schema->size()), "one Arrow column per inferred column"); + CHECK(inferred->array.length == 100, "the inferred read holds every data row"); + CHECK(std::strcmp(inferred->schema.children[0]->name, "Coluna1") == 0, "columns keep their header names"); + + auto by_index = wb->sheet(0).infer_schema(0); + CHECK(by_index.has_value() && !(*by_index)[0].name.has_value(), "a schema without a header has no names"); + auto unnamed = xl::to_arrow(wb->sheet(0), *by_index, {.header_row = 0}); + CHECK(unnamed.has_value(), "to_arrow resolves unnamed columns by index"); + CHECK(unnamed->array.length == 101, "with no header the header row is data"); + + auto empty = xl::to_arrow(wb->sheet(0), std::span{}); + CHECK(!empty.has_value() && empty.error().code == XL_INVALID_ARGUMENT, "an empty schema is rejected"); + + auto stream = xl::to_arrow_stream(wb->sheet(0), *schema, {.batch_size = 8}); + CHECK(stream.has_value(), "to_arrow_stream over an inferred schema must open"); + int64_t rows = 0; + while (true) + { + auto batch = stream->next(); + CHECK(batch.has_value(), "an inferred-schema batch must read without error"); + if (!batch->has_value()) + { + break; + } + rows += (*batch)->array.length; + } + CHECK(rows == 100, "the inferred stream delivers every data row"); + } + std::printf("OK: C++ arrow test passed\n"); return 0; } diff --git a/cpp/tests/batch.cpp b/cpp/tests/batch.cpp index 67e2a6f7..e8c742ca 100644 --- a/cpp/tests/batch.cpp +++ b/cpp/tests/batch.cpp @@ -189,6 +189,144 @@ namespace auto after = reader->next(); check(after.has_value() && after->has_value(), "the reader keeps reading after a whole-sheet read"); } + + const xl::ColumnRef *find_column(const xl::Table &table, std::string_view name) + { + for (const xl::ColumnRef &column : table.columns()) + { + if (column.name == name) + { + return &column; + } + } + return nullptr; + } + + void test_runtime_schema_matches_the_typed_parse(const std::vector &expected) + { + auto workbook = xl::Workbook::open(EXCELREADER_FIXTURE_PATH); + check(workbook.has_value(), "open the fixture for the runtime-schema parse"); + if (!workbook.has_value()) + { + return; + } + auto schema = workbook->sheet(0).infer_schema(); + check(schema.has_value() && !schema->empty(), "infer_schema finds columns"); + if (!schema.has_value()) + { + return; + } + + auto table = xl::parse(workbook->sheet(0), *schema); + check(table.has_value(), "parse over an inferred schema succeeds"); + if (!table.has_value()) + { + return; + } + check(table->size() == static_cast(expected.size()), "the runtime-schema parse reads every row"); + check(table->columns().size() == schema->size(), "one column per inferred column"); + + const xl::ColumnRef *coluna1 = find_column(*table, "Coluna1"); + const xl::ColumnRef *coluna3 = find_column(*table, "Coluna3"); + check(coluna1 != nullptr && coluna1->type == XL_T_STRING, "Coluna1 is a string column"); + check(coluna3 != nullptr && coluna3->type == XL_T_I64, "Coluna3 is an i64 column"); + if (coluna1 == nullptr || coluna3 == nullptr || coluna1->type != XL_T_STRING || coluna3->type != XL_T_I64) + { + return; + } + const auto *offsets = static_cast(coluna1->values); + const auto *text = reinterpret_cast(coluna1->data); + const auto *numbers = static_cast(coluna3->values); + bool same = true; + for (size_t row = 0; row < expected.size(); ++row) + { + const std::string_view cell(text + offsets[row], static_cast(offsets[row + 1] - offsets[row])); + same = same && cell == expected[row].first && numbers[row] == expected[row].second; + } + check(same, "the runtime-schema parse matches the typed parse cell for cell"); + + std::vector bytes; + check(xl::write_columns(bytes, table->columns(), {.format = XL_FORMAT_XLSX}).has_value(), + "a runtime-schema table writes back as it was read"); + auto reopened = xl::Workbook::open(bytes); + check(reopened.has_value(), "the written workbook opens"); + if (!reopened.has_value()) + { + return; + } + auto again = xl::parse(reopened->sheet(0), *schema); + check(again.has_value() && again->size() == table->size(), "the written workbook parses with the same schema"); + + auto by_index = workbook->sheet(0).infer_schema(0); + check(by_index.has_value() && !(*by_index)[0].name.has_value(), "a schema without a header has no names"); + if (by_index.has_value()) + { + auto unnamed = xl::parse(workbook->sheet(0), *by_index, {.header_row = 0}); + check(unnamed.has_value() && unnamed->size() == static_cast(expected.size()) + 1, + "unnamed columns resolve by index, and with no header the header row is data"); + } + + auto empty = xl::parse(workbook->sheet(0), std::span{}); + check(!empty.has_value() && empty.error().code == XL_INVALID_ARGUMENT, "an empty schema is rejected"); + + xl::Table moved = std::move(*table); + check(find_column(moved, "Coluna3") != nullptr, "a moved table keeps its column names"); + } + + void test_runtime_schema_batches_equal_whole_sheet(const std::vector &expected) + { + auto workbook = xl::Workbook::open(EXCELREADER_FIXTURE_PATH); + check(workbook.has_value(), "open the fixture for the runtime-schema batched read"); + if (!workbook.has_value()) + { + return; + } + auto schema = workbook->sheet(0).infer_schema(); + check(schema.has_value(), "infer_schema succeeds for the batched read"); + if (!schema.has_value()) + { + return; + } + + auto reader = xl::parse_batches(workbook->sheet(0), *schema, {.batch_size = 8}); + check(reader.has_value(), "parse_batches over an inferred schema opens"); + if (!reader.has_value()) + { + return; + } + schema->clear(); + + std::vector coluna3; + int batches = 0; + for (auto &batch : *reader) + { + check(batch.has_value(), "a runtime-schema batch reads without error"); + if (!batch.has_value()) + { + return; + } + ++batches; + check(batch->size() <= 8, "no batch exceeds the requested batch size"); + const xl::ColumnRef *column = find_column(*batch, "Coluna3"); + check(column != nullptr && column->type == XL_T_I64, "every batch keeps the schema's names and types"); + if (column == nullptr || column->type != XL_T_I64) + { + return; + } + const auto *values = static_cast(column->values); + coluna3.insert(coluna3.end(), values, values + column->length); + } + check(batches == 13, "100 rows at batch size 8 is 13 batches"); + bool same = coluna3.size() == expected.size(); + for (size_t row = 0; same && row < expected.size(); ++row) + { + same = coluna3[row] == expected[row].second; + } + check(same, "the batches concatenate to the whole-sheet read"); + + auto rejected = xl::parse_batches(workbook->sheet(0), *schema, {.batch_size = 0}); + check(!rejected.has_value() && rejected.error().code == XL_INVALID_ARGUMENT, "a batch size below 1 is rejected"); + } } int main() @@ -200,6 +338,8 @@ int main() test_second_reader_is_allowed(); test_non_positive_batch_size_is_rejected(); test_foreign_read_leaves_the_reader_alone(); + test_runtime_schema_matches_the_typed_parse(expected); + test_runtime_schema_batches_equal_whole_sheet(expected); if (failures != 0) { diff --git a/src/ExcelReader.Native/include/excelreader.hpp b/src/ExcelReader.Native/include/excelreader.hpp index e4736380..ad5b2b0e 100644 --- a/src/ExcelReader.Native/include/excelreader.hpp +++ b/src/ExcelReader.Native/include/excelreader.hpp @@ -2,6 +2,7 @@ #include "excelreader.h" +#include #include #include #include @@ -1642,6 +1643,27 @@ namespace xl return call(Access::handle(sheet), sheet.index(), specs.data(), static_cast(num_fields)); } + template + int32_t with_specs(const Sheet &sheet, std::span columns, Call &&call) + { + const size_t count = columns.size(); + std::vector name_slots(count); + std::vector name_lens(count); + std::vector specs(count); + for (size_t i = 0; i < count; ++i) + { + const InferredColumn &column = columns[i]; + if (column.name.has_value()) + { + name_slots[i] = reinterpret_cast(column.name->data()); + name_lens[i] = static_cast(column.name->size()); + } + specs[i] = xl_column_spec{&name_slots[i], &name_lens[i], column.name.has_value() ? 1 : 0, + column.index, column.type, column.nullable ? 1 : 0}; + } + return call(Access::handle(sheet), sheet.index(), specs.data(), static_cast(count)); + } + inline std::optional check_batch_size(int64_t batch_size) { if (batch_size > 0) @@ -1834,68 +1856,83 @@ namespace xl return view; } - - /// @brief parse delivered a batch at a time. + /// @brief The columns of a parse over a runtime schema, held in native columnar memory. /// - /// An input range of `std::expected, Error>` that stops after the first error. Each - /// batch owns its memory and outlives the reader. Use from one thread at a time. - /// @tparam T Row type, mapped by `ExcelMapper`. - template - class TypedReader + /// Owns that memory. Its columns are the ColumnRefs write_columns takes, borrowed from this + /// table. A column matched by index has no name, and writing needs every column named or none. + class Table { public: - TypedReader(const TypedReader &) = delete; - TypedReader &operator=(const TypedReader &) = delete; + Table(const Table &) = delete; + Table &operator=(const Table &) = delete; - TypedReader(TypedReader &&other) noexcept : reader_(std::exchange(other.reader_, nullptr)) {} + Table(Table &&other) noexcept + : table_(std::exchange(other.table_, xl_table{})), names_(std::move(other.names_)), + columns_(std::move(other.columns_)) + { + } - TypedReader &operator=(TypedReader &&other) noexcept + Table &operator=(Table &&other) noexcept { if (this != &other) { - xl_typed_reader_close(reader_); - reader_ = std::exchange(other.reader_, nullptr); + xl_free_table(&table_); + table_ = std::exchange(other.table_, xl_table{}); + names_ = std::move(other.names_); + columns_ = std::move(other.columns_); } return *this; } - ~TypedReader() { xl_typed_reader_close(reader_); } + ~Table() { xl_free_table(&table_); } - /// @brief Reads the next batch. - /// @return The batch, or empty at the end of the sheet. - std::expected>, Error> next() + int64_t size() const noexcept { return table_.row_count; } + bool empty() const noexcept { return table_.row_count == 0; } + + /// @brief Every column, in schema order. + std::span columns() const noexcept { return columns_; } + + private: + friend struct detail::Access; + + // A moved vector keeps its elements in place, so the names columns_ views stay valid. + Table(xl_table table, std::span schema) : table_(table) { - xl_table table{}; - int32_t status = xl_typed_reader_next(reader_, &table); - if (status == XL_EOF) + names_.reserve(schema.size()); + columns_.reserve(schema.size()); + for (size_t i = 0; i < schema.size(); ++i) { - return std::optional>{}; + const std::string &name = names_.emplace_back(schema[i].name.value_or(std::string{})); + const xl_column &raw = table_.columns[i]; + const int64_t validity_len = raw.validity == nullptr ? 0 : (raw.length + 7) / 8; + columns_.push_back(ColumnRef{name, raw.type, raw.length, raw.values, raw.validity, validity_len, + raw.data, raw.data_len}); } - if (status != XL_OK) - { - return std::unexpected(detail::make_error(status)); - } - TableView view(table); - if (std::optional problem = detail::check_ranges(table)) - { - return std::unexpected(std::move(*problem)); - } - return std::optional>(std::move(view)); } - class iterator + xl_table table_{}; + std::vector names_; + std::vector columns_; + }; + + namespace detail + { + /// The input iterator of a batched read: `Reader::next()` yields `Batch`es until it is empty + /// or fails, and the failure is the last element. + template + class BatchIterator { public: using iterator_category = std::input_iterator_tag; using iterator_concept = std::input_iterator_tag; - using value_type = std::expected, Error>; + using value_type = std::expected; using difference_type = std::ptrdiff_t; - iterator() = default; + BatchIterator() = default; value_type &operator*() { return *current_; } - iterator &operator++() + BatchIterator &operator++() { advance(); return *this; @@ -1903,15 +1940,15 @@ namespace xl void operator++(int) { advance(); } - friend bool operator==(const iterator &it, std::default_sentinel_t) noexcept + friend bool operator==(const BatchIterator &it, std::default_sentinel_t) noexcept { return !it.current_.has_value(); } private: - friend class TypedReader; + friend struct Access; - explicit iterator(TypedReader *reader) : reader_(reader) { advance(); } + explicit BatchIterator(Reader *reader) : reader_(reader) { advance(); } void advance() { @@ -1921,7 +1958,7 @@ namespace xl return; } current_.reset(); - std::expected>, Error> batch = reader_->next(); + std::expected, Error> batch = reader_->next(); if (!batch.has_value()) { current_ = value_type(std::unexpect, std::move(batch.error())); @@ -1937,11 +1974,97 @@ namespace xl current_ = value_type(std::move(**batch)); } - TypedReader *reader_ = nullptr; + Reader *reader_ = nullptr; std::optional current_{}; }; - iterator begin() { return iterator(this); } + inline std::expected adopt_table(xl_table table, std::span schema, + const char *source) + { + const bool consistent = table.row_count >= 0 && table.column_count >= 0 && + static_cast(table.column_count) == schema.size() && + std::all_of(table.columns, table.columns + table.column_count, + [&](const xl_column &c) { return c.length == table.row_count; }); + if (!consistent) + { + xl_free_table(&table); + return std::unexpected(Error{XL_ERROR, std::string(source) + " returned a table that does not match its specs"}); + } + return Access::make(table, schema); + } + } + + /// @brief parse over a schema chosen at run time, such as one Sheet::infer_schema returned. + /// @param sheet The sheet to read. + /// @param columns One per output column: matched by `name` when it has one, else by `index`. + /// Must not be empty. + /// @param options Header row and parallelism. + /// @return Every row after the header. + inline std::expected parse(Sheet sheet, std::span columns, + const ReadOptions &options = {}) + { + xl_table table{}; + const int32_t status = detail::with_specs( + sheet, columns, [&](xl_workbook *handle, int32_t index, const xl_column_spec *specs, int32_t count) + { return xl_parse_typed(handle, index, specs, count, options.header_row, options.parallelism, &table); }); + if (status != XL_OK) + { + return std::unexpected(detail::make_error(status)); + } + return detail::adopt_table(table, columns, "xl_parse_typed"); + } + + /// @brief parse delivered a batch at a time. + /// + /// An input range of `std::expected, Error>` that stops after the first error. Each + /// batch owns its memory and outlives the reader. Use from one thread at a time. + /// @tparam T Row type, mapped by `ExcelMapper`. + template + class TypedReader + { + public: + TypedReader(const TypedReader &) = delete; + TypedReader &operator=(const TypedReader &) = delete; + + TypedReader(TypedReader &&other) noexcept : reader_(std::exchange(other.reader_, nullptr)) {} + + TypedReader &operator=(TypedReader &&other) noexcept + { + if (this != &other) + { + xl_typed_reader_close(reader_); + reader_ = std::exchange(other.reader_, nullptr); + } + return *this; + } + + ~TypedReader() { xl_typed_reader_close(reader_); } + + /// @brief Reads the next batch. + /// @return The batch, or empty at the end of the sheet. + std::expected>, Error> next() + { + xl_table table{}; + int32_t status = xl_typed_reader_next(reader_, &table); + if (status == XL_EOF) + { + return std::optional>{}; + } + if (status != XL_OK) + { + return std::unexpected(detail::make_error(status)); + } + TableView view(table); + if (std::optional problem = detail::check_ranges(table)) + { + return std::unexpected(std::move(*problem)); + } + return std::optional>(std::move(view)); + } + + using iterator = detail::BatchIterator>; + + iterator begin() { return detail::Access::make(this); } std::default_sentinel_t end() const noexcept { return {}; } private: @@ -1974,6 +2097,94 @@ namespace xl return detail::Access::make>(reader); } + /// @brief parse over a runtime schema, delivered a batch at a time. + /// + /// TypedReader's counterpart, whose batches are Tables. + class TableReader + { + public: + TableReader(const TableReader &) = delete; + TableReader &operator=(const TableReader &) = delete; + + TableReader(TableReader &&other) noexcept + : reader_(std::exchange(other.reader_, nullptr)), schema_(std::move(other.schema_)) + { + } + + TableReader &operator=(TableReader &&other) noexcept + { + if (this != &other) + { + xl_typed_reader_close(reader_); + reader_ = std::exchange(other.reader_, nullptr); + schema_ = std::move(other.schema_); + } + return *this; + } + + ~TableReader() { xl_typed_reader_close(reader_); } + + /// @brief Reads the next batch. + /// @return The batch, or empty at the end of the sheet. + std::expected, Error> next() + { + xl_table table{}; + int32_t status = xl_typed_reader_next(reader_, &table); + if (status == XL_EOF) + { + return std::optional
{}; + } + if (status != XL_OK) + { + return std::unexpected(detail::make_error(status)); + } + std::expected batch = detail::adopt_table(table, schema_, "xl_typed_reader_next"); + if (!batch.has_value()) + { + return std::unexpected(std::move(batch.error())); + } + return std::optional
(std::move(*batch)); + } + + using iterator = detail::BatchIterator; + + iterator begin() { return detail::Access::make(this); } + std::default_sentinel_t end() const noexcept { return {}; } + + private: + friend struct detail::Access; + + TableReader(xl_typed_reader *reader, std::span schema) + : reader_(reader), schema_(schema.begin(), schema.end()) + { + } + + xl_typed_reader *reader_ = nullptr; + std::vector schema_; + }; + + /// @brief Opens a TableReader over a sheet. + /// @param sheet The sheet to read. + /// @param columns As for the runtime-schema parse; copied, so it need not outlive the reader. + /// @param options Header row and batch size. + inline std::expected parse_batches(Sheet sheet, std::span columns, + const ReadOptions &options = {}) + { + if (std::optional problem = detail::check_batch_size(options.batch_size)) + { + return std::unexpected(std::move(*problem)); + } + xl_typed_reader *reader = nullptr; + const int32_t status = detail::with_specs( + sheet, columns, [&](xl_workbook *handle, int32_t index, const xl_column_spec *specs, int32_t count) + { return xl_typed_reader_open(handle, index, specs, count, options.header_row, options.batch_size, &reader); }); + if (status != XL_OK) + { + return std::unexpected(detail::make_error(status)); + } + return detail::Access::make(reader, columns); + } + namespace detail { diff --git a/src/ExcelReader.Native/include/excelreader_arrow.hpp b/src/ExcelReader.Native/include/excelreader_arrow.hpp index 00a2fb0e..78448ee2 100644 --- a/src/ExcelReader.Native/include/excelreader_arrow.hpp +++ b/src/ExcelReader.Native/include/excelreader_arrow.hpp @@ -83,6 +83,26 @@ namespace xl return result; } + /// @brief to_arrow over a schema chosen at run time, such as one Sheet::infer_schema returned. + /// @param sheet The sheet to read. + /// @param columns One per output column: matched by `name` when it has one, else by `index`. + /// Must not be empty. + /// @param options Header row and parallelism. + inline std::expected to_arrow(Sheet sheet, std::span columns, + const ReadOptions &options = {}) + { + ArrowTable result; + const int32_t status = detail::with_specs( + sheet, columns, [&](xl_workbook *handle, int32_t index, const xl_column_spec *specs, int32_t count) + { return xl_parse_arrow(handle, index, specs, count, options.header_row, options.parallelism, + &result.array, &result.schema); }); + if (status != XL_OK) + { + return std::unexpected(detail::make_error(status)); + } + return result; + } + /// @brief parse_batches's Arrow counterpart: record batches pulled one at a time. /// /// Use from one thread at a time. @@ -199,6 +219,29 @@ namespace xl } return detail::Access::make(stream); } + + /// @brief to_arrow_stream over a schema chosen at run time, as for the to_arrow overload. + /// @param sheet The sheet to read. + /// @param columns One per output column: matched by `name` when it has one, else by `index`. + /// Must not be empty. + /// @param options Header row and batch size. + inline std::expected to_arrow_stream(Sheet sheet, std::span columns, + const ReadOptions &options = {}) + { + if (std::optional problem = detail::check_batch_size(options.batch_size)) + { + return std::unexpected(std::move(*problem)); + } + ArrowArrayStream stream{}; + const int32_t status = detail::with_specs( + sheet, columns, [&](xl_workbook *handle, int32_t index, const xl_column_spec *specs, int32_t count) + { return xl_parse_arrow_stream(handle, index, specs, count, options.header_row, options.batch_size, &stream); }); + if (status != XL_OK) + { + return std::unexpected(detail::make_error(status)); + } + return detail::Access::make(stream); + } } #endif /* XL_EXCELREADER_ARROW_HPP */ From aee4fd52d7d029a7e6ccd6f1a0db7fd7785d5256 Mon Sep 17 00:00:00 2001 From: Gabriel Matte Date: Thu, 22 Oct 2026 09:59:15 -0300 Subject: [PATCH 31/34] feat(ci): add C++ coverage testing and upload to Codecov --- .github/workflows/native-bindings.yml | 20 ++++++++++++++++++++ codecov.yml | 11 +++++++++++ 2 files changed, 31 insertions(+) diff --git a/.github/workflows/native-bindings.yml b/.github/workflows/native-bindings.yml index 7cdea891..a32e7a25 100644 --- a/.github/workflows/native-bindings.yml +++ b/.github/workflows/native-bindings.yml @@ -351,3 +351,23 @@ jobs: - name: Run the consumer run: ctest --test-dir build/consumer --output-on-failure -C Release + + # A separate Debug build without the benchmarks, so only the tests count towards coverage. + - name: Test with coverage (Linux) + if: runner.os == 'Linux' + env: + EXCELREADER_NATIVE_LIB: ${{ steps.lib.outputs.path }} + run: | + cmake -S cpp -B build/cpp-coverage -DEXCELREADER_BUILD_TESTS=ON -DCMAKE_BUILD_TYPE=Debug -DCMAKE_CXX_FLAGS=--coverage + cmake --build build/cpp-coverage + ctest --test-dir build/cpp-coverage --output-on-failure + pipx run gcovr --root . --filter 'cpp/include/xl/' --exclude-throw-branches --cobertura coverage-cpp.xml --txt + + - name: Upload C++ coverage to Codecov + if: runner.os == 'Linux' + uses: codecov/codecov-action@v7 + with: + token: ${{ secrets.CODECOV_TOKEN }} + files: ./coverage-cpp.xml + flags: cpp + fail_ci_if_error: false diff --git a/codecov.yml b/codecov.yml index 42699370..acbd864c 100644 --- a/codecov.yml +++ b/codecov.yml @@ -13,10 +13,18 @@ coverage: target: 88% flags: - rust + cpp: + target: 88% + flags: + - cpp patch: default: target: 88% +# gcov reports the C++ headers through the cpp/include/xl symlink; git tracks them at their real path. +fixes: + - "cpp/include/xl/::src/ExcelReader.Native/include/" + # Each language uploads under its own flag from a path-filtered job, so a flag a commit did not run keeps its last report. flag_management: default_rules: @@ -31,3 +39,6 @@ flag_management: - name: python paths: - python/ + - name: cpp + paths: + - src/ExcelReader.Native/include/ From c1ef50c301abc10d27ebe6abb2c57124e1c660b6 Mon Sep 17 00:00:00 2001 From: Gabriel Matte Date: Thu, 22 Oct 2026 11:00:47 -0300 Subject: [PATCH 32/34] feat(tests): add tests for handling sheets past the end and error reporting in various sources --- .github/workflows/native-bindings.yml | 9 +- cpp/tests/aggregate.cpp | 36 ++++++ cpp/tests/arrow.cpp | 14 ++ cpp/tests/batch.cpp | 52 ++++++++ cpp/tests/numbers.cpp | 3 + cpp/tests/sheets.cpp | 19 +++ cpp/tests/sources.cpp | 176 ++++++++++++++++++++++++++ cpp/tests/write.cpp | 1 + 8 files changed, 306 insertions(+), 4 deletions(-) diff --git a/.github/workflows/native-bindings.yml b/.github/workflows/native-bindings.yml index a32e7a25..0918baca 100644 --- a/.github/workflows/native-bindings.yml +++ b/.github/workflows/native-bindings.yml @@ -352,16 +352,17 @@ jobs: - name: Run the consumer run: ctest --test-dir build/consumer --output-on-failure -C Release - # A separate Debug build without the benchmarks, so only the tests count towards coverage. - - name: Test with coverage (Linux) + - name: Test with coverage and sanitizers (Linux) if: runner.os == 'Linux' env: EXCELREADER_NATIVE_LIB: ${{ steps.lib.outputs.path }} + ASAN_OPTIONS: detect_leaks=0 + UBSAN_OPTIONS: print_stacktrace=1 run: | - cmake -S cpp -B build/cpp-coverage -DEXCELREADER_BUILD_TESTS=ON -DCMAKE_BUILD_TYPE=Debug -DCMAKE_CXX_FLAGS=--coverage + cmake -S cpp -B build/cpp-coverage -DEXCELREADER_BUILD_TESTS=ON -DCMAKE_BUILD_TYPE=Debug "-DCMAKE_CXX_FLAGS=--coverage -fprofile-update=atomic -fsanitize=address,undefined -fno-sanitize-recover=all -fno-omit-frame-pointer" cmake --build build/cpp-coverage ctest --test-dir build/cpp-coverage --output-on-failure - pipx run gcovr --root . --filter 'cpp/include/xl/' --exclude-throw-branches --cobertura coverage-cpp.xml --txt + pipx run gcovr --root . --filter 'src/ExcelReader.Native/include/' --exclude-throw-branches --cobertura coverage-cpp.xml --txt - name: Upload C++ coverage to Codecov if: runner.os == 'Linux' diff --git a/cpp/tests/aggregate.cpp b/cpp/tests/aggregate.cpp index 366da363..76a1e9e9 100644 --- a/cpp/tests/aggregate.cpp +++ b/cpp/tests/aggregate.cpp @@ -473,6 +473,34 @@ static int test_sheet_aggregate_reports_a_throwing_accumulate() return 0; } +static int test_sheet_aggregate_fails_on_a_sheet_past_the_end() +{ + const auto bytes = generate(XL_FORMAT_XLSX, 10); + auto workbook = open_generated(bytes, XL_FORMAT_XLSX); + CHECK(workbook.has_value(), "open the generated workbook"); + auto result = workbook->sheet(99).aggregate(RowCounter{}, 1, 1); + CHECK(!result.has_value() && result.error().code < 0, "a sheet past the end fails with the library's status"); + return 0; +} + +static int test_aggregate_reports_a_seed_copy_that_throws() +{ + struct ThrowsOnCopy + { + ThrowsOnCopy() = default; + ThrowsOnCopy(const ThrowsOnCopy &) { throw std::runtime_error("no copies"); } + ThrowsOnCopy(ThrowsOnCopy &&) noexcept = default; + int32_t accumulate(xl::RowView) noexcept { return XL_OK; } + int32_t combine(ThrowsOnCopy &) noexcept { return XL_OK; } + }; + + std::span data(reinterpret_cast(kCsv.data()), kCsv.size()); + auto result = xl::aggregate_csv(data, ThrowsOnCopy{}); + CHECK(!result.has_value() && std::string_view(result.error().message).find("threw") != std::string_view::npos, + "a seed copy that throws must be reported as a callback that threw"); + return 0; +} + static int test_existing_csv_accumulator_works_unchanged() { const auto bytes = generate(XL_FORMAT_CSV, 10'000); @@ -508,6 +536,14 @@ int main() { return failed; } + if (int failed = test_sheet_aggregate_fails_on_a_sheet_past_the_end()) + { + return failed; + } + if (int failed = test_aggregate_reports_a_seed_copy_that_throws()) + { + return failed; + } if (int failed = test_aggregate_csv_memory()) { return failed; diff --git a/cpp/tests/arrow.cpp b/cpp/tests/arrow.cpp index d7879a68..58c10e26 100644 --- a/cpp/tests/arrow.cpp +++ b/cpp/tests/arrow.cpp @@ -93,6 +93,20 @@ int main() auto second = xl::to_arrow_stream(stream_workbook->sheet(0), {.batch_size = 8}); CHECK(second.has_value(), "a second stream on one workbook opens"); + + xl::ArrowStream moved = std::move(*second); + CHECK(!second->schema().has_value(), "a moved-from stream has no schema"); + CHECK(!second->next().has_value(), "a moved-from stream reads nothing"); + CHECK(moved.schema().has_value(), "the moved-to stream still reports its schema"); + + const xl::InferredColumn columns[] = {{"Coluna3", 0, XL_T_I64, false}}; + CHECK(!xl::to_arrow_stream(stream_workbook->sheet(99)).has_value(), + "a stream over a sheet past the end fails to open"); + CHECK(!xl::to_arrow_stream(stream_workbook->sheet(99), columns).has_value(), + "a runtime-schema stream over a sheet past the end fails to open"); + auto zero_batch = xl::to_arrow_stream(stream_workbook->sheet(0), columns, {.batch_size = 0}); + CHECK(!zero_batch.has_value() && zero_batch.error().code == XL_INVALID_ARGUMENT, + "a runtime-schema stream rejects a batch size below 1"); } { diff --git a/cpp/tests/batch.cpp b/cpp/tests/batch.cpp index e8c742ca..049d6b0d 100644 --- a/cpp/tests/batch.cpp +++ b/cpp/tests/batch.cpp @@ -271,6 +271,56 @@ namespace xl::Table moved = std::move(*table); check(find_column(moved, "Coluna3") != nullptr, "a moved table keeps its column names"); + + auto replacement = xl::parse(workbook->sheet(0), *schema); + check(replacement.has_value(), "parse a second table to move-assign"); + if (replacement.has_value()) + { + moved = std::move(*replacement); + check(moved.size() == static_cast(expected.size()) && find_column(moved, "Coluna1") != nullptr, + "a move-assigned table holds the other table's rows and names"); + } + } + + void test_move_assigned_view_holds_the_other_rows(const std::vector &expected) + { + auto workbook = xl::Workbook::open(EXCELREADER_FIXTURE_PATH); + check(workbook.has_value(), "open the fixture for the move-assigned view"); + if (!workbook.has_value()) + { + return; + } + auto whole = xl::parse(workbook->sheet(0)); + auto reader = xl::parse_batches(workbook->sheet(0), {.batch_size = 8}); + check(whole.has_value() && reader.has_value(), "parse a view and open a reader to move-assign from"); + if (!whole.has_value() || !reader.has_value()) + { + return; + } + auto batch = reader->next(); + check(batch.has_value() && batch->has_value(), "read a batch to move-assign over the whole view"); + if (!batch.has_value() || !batch->has_value()) + { + return; + } + *whole = std::move(**batch); + check(whole->size() == 8 && std::string((*whole)[0].Coluna1) == expected[0].first, + "a move-assigned view holds the batch's rows"); + } + + void test_a_sheet_past_the_end_fails_every_read() + { + auto workbook = xl::Workbook::open(EXCELREADER_FIXTURE_PATH); + check(workbook.has_value(), "open the fixture for the past-the-end sheet"); + if (!workbook.has_value()) + { + return; + } + const xl::Sheet missing = workbook->sheet(99); + const xl::InferredColumn schema[] = {{"Coluna3", 0, XL_T_I64, false}}; + check(!xl::parse(missing).has_value(), "parse fails on a sheet past the end"); + check(!xl::parse_batches(missing).has_value(), "parse_batches fails on a sheet past the end"); + check(!xl::parse_batches(missing, schema).has_value(), "a runtime-schema parse_batches fails on a sheet past the end"); } void test_runtime_schema_batches_equal_whole_sheet(const std::vector &expected) @@ -340,6 +390,8 @@ int main() test_foreign_read_leaves_the_reader_alone(); test_runtime_schema_matches_the_typed_parse(expected); test_runtime_schema_batches_equal_whole_sheet(expected); + test_move_assigned_view_holds_the_other_rows(expected); + test_a_sheet_past_the_end_fails_every_read(); if (failures != 0) { diff --git a/cpp/tests/numbers.cpp b/cpp/tests/numbers.cpp index 152e2123..1750915d 100644 --- a/cpp/tests/numbers.cpp +++ b/cpp/tests/numbers.cpp @@ -8,6 +8,7 @@ #include #include +#include #include namespace @@ -61,6 +62,8 @@ int main() check(xl::CellView{0, xl::CellType::Number, {}, 0.0125}.text() == "0.0125", "a fraction places its point"); check(xl::CellView{0, xl::CellType::Number, {}, -2.5}.text() == "-2.5", "a negative keeps its sign"); check(xl::CellView{0, xl::CellType::Number, {}, 0.0}.text() == "0", "zero is 0"); + check(xl::CellView{0, xl::CellType::Number, {}, std::numeric_limits::infinity()}.text() == "inf", + "infinity has no exponent to place"); round_trips(0.1); round_trips(1.0 / 3.0); round_trips(1e21); diff --git a/cpp/tests/sheets.cpp b/cpp/tests/sheets.cpp index 9942daef..f4d682f7 100644 --- a/cpp/tests/sheets.cpp +++ b/cpp/tests/sheets.cpp @@ -113,6 +113,25 @@ int main() check(decoded.has_value() && decoded->size() == 3, "read_all reads the whole named sheet"); auto schema = workbook->sheet(1).infer_schema(0, 10); check(schema.has_value() && schema->size() == 1, "infer_schema samples the named sheet"); + + const xl::Sheet beyond = workbook->sheet(2); + check(!beyond.name().has_value(), "a sheet past the end has no name"); + check(!beyond.visibility().has_value(), "a sheet past the end has no visibility"); + check(!beyond.read_all().has_value(), "read_all fails on a sheet past the end"); + check(!beyond.infer_schema().has_value(), "infer_schema fails on a sheet past the end"); + } + } + + { + auto workbook = xl::Workbook::open(two); + check(workbook.has_value(), "open for the moved-from workbook"); + if (workbook.has_value()) + { + xl::Workbook moved = std::move(*workbook); + check(!workbook->sheets().has_value(), "a moved-from workbook lists no sheets"); + check(!workbook->sheet_by_name("sheet0").has_value(), "a moved-from workbook finds no sheet by name"); + check(!workbook->is_date1904().has_value(), "a moved-from workbook has no date system"); + check(moved.is_date1904().has_value(), "the moved-to workbook still answers"); } } diff --git a/cpp/tests/sources.cpp b/cpp/tests/sources.cpp index 76c265d9..11024e1b 100644 --- a/cpp/tests/sources.cpp +++ b/cpp/tests/sources.cpp @@ -1,4 +1,5 @@ #include +#include #include #include @@ -7,12 +8,25 @@ #include #include #include +#include #include +#include #include #include #include #include +struct Quantity +{ + int64_t qty; +}; + +template <> +struct xl::ExcelMapper +{ + static constexpr auto get_bindings() { return std::make_tuple(xl::make_field("qty", &Quantity::qty)); } +}; + namespace { int failures = 0; @@ -98,6 +112,50 @@ namespace Counters &counters_; }; + class OpaqueThrowingSource final : public xl::Source + { + public: + std::uint64_t size() const override { return 1024; } + std::expected read_at(std::uint64_t, std::span) const override + { + throw 42; + } + }; + + class HugeSource final : public xl::Source + { + public: + std::uint64_t size() const override { return std::numeric_limits::max(); } + std::expected read_at(std::uint64_t, std::span) const override + { + return std::size_t{0}; + } + }; + + /// Serves the first `trip` bytes and fails every read past them, so a read fails partway. + class TrippingSource final : public xl::Source + { + public: + TrippingSource(std::vector bytes, std::uint64_t trip) : bytes_(std::move(bytes)), trip_(trip) {} + + std::uint64_t size() const override { return bytes_.size(); } + + std::expected read_at(std::uint64_t offset, std::span buffer) const override + { + if (offset >= trip_) + { + return std::unexpected(std::string("disk failed mid-read")); + } + const std::size_t count = std::min(buffer.size(), bytes_.size() - offset); + std::memcpy(buffer.data(), bytes_.data() + offset, count); + return count; + } + + private: + std::vector bytes_; + std::uint64_t trip_; + }; + class ErrorStream final : public xl::InputStream { public: @@ -172,6 +230,108 @@ namespace auto decoded = workbook.sheet(0).read_all(); return decoded.has_value() ? decoded->size() : 0; } + + bool mentions(const xl::Error &error, std::string_view text) + { + return error.message.find(text) != std::string::npos; + } + + std::expected open_tripping() + { + std::string csv = "qty\n"; + for (int i = 0; i < 50'000; ++i) + { + csv += std::to_string(i) + "\n"; + } + std::vector bytes(csv.size()); + std::memcpy(bytes.data(), csv.data(), csv.size()); + const std::uint64_t trip = bytes.size() / 2; + return xl::Workbook::open(std::make_unique(std::move(bytes), trip), + {.format = XL_FORMAT_CSV, .source_block_size = -1}); + } + + /// Drains `next()` and returns the error it stopped on, or nothing if the sheet ended cleanly. + template + std::optional first_error(Reader &reader) + { + while (true) + { + auto batch = reader.next(); + if (!batch.has_value()) + { + return batch.error(); + } + if (!batch->has_value()) + { + return std::nullopt; + } + } + } + + const xl::InferredColumn kQtySchema[] = {{"qty", 0, XL_T_I64, false}}; + + void test_a_typed_range_ends_on_a_failed_read() + { + auto workbook = open_tripping(); + check(workbook.has_value(), "a tripping source opens"); + if (!workbook.has_value()) + { + return; + } + auto reader = xl::parse_batches(workbook->sheet(0), {.batch_size = 100}); + check(reader.has_value(), "a typed reader opens before the failure"); + if (!reader.has_value()) + { + return; + } + int64_t rows = 0; + std::optional last; + for (auto &batch : *reader) + { + if (!batch.has_value()) + { + last = batch.error(); + break; + } + rows += batch->size(); + } + check(rows > 0, "batches before the failure arrive"); + check(last.has_value() && mentions(*last, "disk failed mid-read"), "the range ends on the source's error"); + } + + void test_a_table_reader_reports_a_failed_read() + { + auto workbook = open_tripping(); + if (!workbook.has_value()) + { + return; + } + auto reader = xl::parse_batches(workbook->sheet(0), kQtySchema, {.batch_size = 100}); + check(reader.has_value(), "a table reader opens before the failure"); + if (!reader.has_value()) + { + return; + } + const std::optional error = first_error(*reader); + check(error.has_value() && mentions(*error, "disk failed mid-read"), "a table reader reports the source's error"); + } + + void test_an_arrow_stream_reports_a_failed_read() + { + auto workbook = open_tripping(); + if (!workbook.has_value()) + { + return; + } + auto stream = xl::to_arrow_stream(workbook->sheet(0), kQtySchema, {.batch_size = 100}); + check(stream.has_value(), "an Arrow stream opens before the failure"); + if (!stream.has_value()) + { + return; + } + const std::optional error = first_error(*stream); + check(error.has_value() && mentions(*error, "disk failed mid-read"), "an Arrow stream reports the source's error"); + } } int main() @@ -215,6 +375,22 @@ int main() "an exception from the source becomes an error"); check(throwing.destroyed.load() == 1, "a source that threw is destroyed exactly once"); } + { + auto workbook = xl::Workbook::open(std::make_unique(), {.format = XL_FORMAT_XLSX}); + check(!workbook.has_value() && mentions(workbook.error(), "the source threw an exception"), + "a throw that is not a std::exception becomes an error"); + } + { + auto null_source = xl::Workbook::open(std::unique_ptr{}); + check(!null_source.has_value() && null_source.error().code == XL_INVALID_ARGUMENT, "a null source is rejected"); + auto null_stream = xl::Workbook::open(std::unique_ptr{}); + check(!null_stream.has_value() && null_stream.error().code == XL_INVALID_ARGUMENT, "a null stream is rejected"); + auto huge = xl::Workbook::open(std::make_unique()); + check(!huge.has_value() && huge.error().code == XL_INVALID_ARGUMENT, "a source larger than INT64_MAX is rejected"); + } + test_a_typed_range_ends_on_a_failed_read(); + test_a_table_reader_reports_a_failed_read(); + test_an_arrow_stream_reports_a_failed_read(); { auto workbook = xl::Workbook::open(std::make_unique(), {.format = XL_FORMAT_XLSX}); check(!workbook.has_value() && workbook.error().message.find("pipe broke") != std::string::npos, diff --git a/cpp/tests/write.cpp b/cpp/tests/write.cpp index 337decbe..3db2cf95 100644 --- a/cpp/tests/write.cpp +++ b/cpp/tests/write.cpp @@ -339,6 +339,7 @@ static int test_format_from_extension() } CHECK(!xl::write_columns(temp_path("no-extension").string(), columns).has_value(), "a path without an extension must be rejected"); + CHECK(!xl::write_columns("ab", columns).has_value(), "a name shorter than any extension must be rejected"); return 0; } From c9d0033e390a4da302b9f6c0f94c334de40ab2de Mon Sep 17 00:00:00 2001 From: Gabriel Matte Date: Thu, 22 Oct 2026 11:08:22 -0300 Subject: [PATCH 33/34] fix(ci): update gcovr command to merge lines for C++ coverage reporting --- .github/workflows/native-bindings.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/native-bindings.yml b/.github/workflows/native-bindings.yml index 0918baca..1052889f 100644 --- a/.github/workflows/native-bindings.yml +++ b/.github/workflows/native-bindings.yml @@ -362,7 +362,7 @@ jobs: cmake -S cpp -B build/cpp-coverage -DEXCELREADER_BUILD_TESTS=ON -DCMAKE_BUILD_TYPE=Debug "-DCMAKE_CXX_FLAGS=--coverage -fprofile-update=atomic -fsanitize=address,undefined -fno-sanitize-recover=all -fno-omit-frame-pointer" cmake --build build/cpp-coverage ctest --test-dir build/cpp-coverage --output-on-failure - pipx run gcovr --root . --filter 'src/ExcelReader.Native/include/' --exclude-throw-branches --cobertura coverage-cpp.xml --txt + pipx run gcovr --root . --filter 'src/ExcelReader.Native/include/' --exclude-throw-branches --merge-lines --cobertura coverage-cpp.xml --txt - name: Upload C++ coverage to Codecov if: runner.os == 'Linux' From a3f53f69c9deee2e4f663141d6b95cbd241bec19 Mon Sep 17 00:00:00 2001 From: Gabriel Matte Date: Thu, 22 Oct 2026 11:27:43 -0300 Subject: [PATCH 34/34] test: improve error handling in ForEachRecord_Cancelled test --- .../Parser/ParallelCsv/ForEachCsvParallelTests.cs | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/tests/ExcelReader.Tests/Parser/ParallelCsv/ForEachCsvParallelTests.cs b/tests/ExcelReader.Tests/Parser/ParallelCsv/ForEachCsvParallelTests.cs index 1c5c7a02..9c399824 100644 --- a/tests/ExcelReader.Tests/Parser/ParallelCsv/ForEachCsvParallelTests.cs +++ b/tests/ExcelReader.Tests/Parser/ParallelCsv/ForEachCsvParallelTests.cs @@ -166,23 +166,32 @@ public async Task ForEachRecord_BodyThrows_LaterChunksStopDelivering() { const int rows = 20_000; byte[] csv = SalesCsv(rows); - int delivered = 0; + int failed = 0; + int afterFailure = 0; await Assert.ThrowsAsync(() => ChunkedRecord( csv, sale => { + if (Volatile.Read(ref failed) == 1) + { + if (Interlocked.Increment(ref afterFailure) == 1) + { + Thread.Sleep(100); + } + return; + } if (sale.Units == 1) { + Volatile.Write(ref failed, 1); throw new InvalidOperationException("boom"); } - Interlocked.Increment(ref delivered); }, new ChunkProbe(), dop: 2, chunkSize: 2048)); - Assert.True(delivered < rows / 4, $"{delivered} of {rows} records were delivered after the first one failed"); + Assert.True(afterFailure <= 1, $"{afterFailure} records were delivered after the first one failed"); } [Fact]