Chapter 9: The last four, and a harness that had stopped measuring

Chapter 8 ended by sorting the fourteen remaining corpus disagreements into piles: six elapsed-time cells, three strict-ISO timezone cases, one whitespace argument I believe I am right about, and four I had not looked at. "Four are untriaged" is a fine place to stop for a day. It is not a fine place to stop.

So I went back for them. All four were real defects in this reader, in three distinct classes, and none of them were the kind of thing I would have found by thinking harder about the code.

First, the harness had rotted

Before any of that. The corpus checkout is not vendored, so re-running the comparison means cloning ExcelDataReader again and rebuilding both dumpers. I did, pointed it at the committed baseline, and got two files that had been recorded IDENTICAL coming back DIFFER.

That is the alarming direction. Either the reader had regressed without a test noticing, or the baseline was recorded against something I could no longer reproduce.

It was the second, and the cause is embarrassing in a specific way. Chapter 8 describes fixing the C# dumper so that DateTimeOffset renders as UTC instead of falling through a default: branch into Convert.ToString. That fix is real; it is why those two files matched. It was never committed. The journey/edrdump.cs in the repository is the version from before the fix.

So the committed harness could not reproduce the committed baseline, and had not been able to since the day both were written. Nothing failed, because the test that runs without .NET checks hashes of our output, and our output had not changed. The parity number was still true. The thing that proved it was not in the repository.

Committing the dumper fix makes HEAD reproduce the baseline exactly: 229, 33, 27, 14, file for file. That took an hour and produced no improvement to the library whatsoever, which is the correct amount of progress for discovering that your instrument was not in the box.

The lesson is the same one this project keeps teaching, one level further out. I had checked that the reader was correct. I had not checked that the thing checking the reader was the thing I had checked with.

The four

With the harness honest again, the four files.

Two of them were one bug. Issue467_SstEmptyContinue.xls and Issue467_EmptyContinueLeftoverbytes.xls are named, helpfully, after their own cause. Both produce one corrupted string in a 627-line dump: the reference reads However theCCsion, and this read However the䌁sion. It then dropped the final character of the string, 8,000 characters later.

A BIFF8 string split across a CONTINUE record restates its compression flag in the first byte of the continuation. Chapter 5 already got that right, and there is a test for it. What neither covered:

• A CONTINUE may carry its flags byte and no characters at all. The next byte then begins another CONTINUE with another flags byte. Checking for a boundary once per character consumes the first flag and reads the second as character data. 01 43 becomes U+4301 instead of C.
• A CONTINUE may end with fewer bytes than a character needs. A UTF-16 unit never straddles a boundary, so that trailing byte belongs to no character and must be discarded rather than paired with the next segment's flags byte.

Both collapse into one rule. Before every character, settle the boundary, and keep settling it until the position is somewhere a character can actually start. The if became a for, which is the entire fix:

for {
        if c.atContinuation() {
                flags, ok := c.u8()
                ...
                high = flags&0x01 != 0
                continue
        }
        width := 1
        if high {
                width = 2
        }
        if end := c.segmentEnd(); end < len(c.data) && end-c.pos < width {
                c.pos = end
                continue
        }
        break
}

I want to be honest about how this was found, because it was not by reading. It was by dumping the byte offsets of every continuation boundary in the record and staring at 01 01 43 00 43 00 01 73 until the two adjacent boundaries at 8210 and 8211 stopped looking like a mistake in my probe.

The third was a chart. Issue321.xls produced thirty-three numeric cells that appear nowhere in the reference's output, and lost two strings that do. The invented values were an 11×3 block at the top-left of the sheet.

A worksheet that carries a chart stores the chart as its own BOF..EOF substream in the middle of the worksheet's cell records, and that substream contains NUMBER records for the chart's cached series values. My reader skipped BOF as "not a cell record" and carried on, so the chart's cached data was merged into the sheet as real cells, overwriting the two strings that happened to share their addresses. Then the chart's EOF ended the sheet early, twenty-three records before the real one.

The fix is a depth counter. A nested BOF opens a substream that is not ours; its records are skipped and its EOF decrements rather than closing the sheet.

This is the third time this format has punished me for assuming a record stream is flat. Chapter 8 was cells arriving out of row order; this is records arriving from a different document entirely.

The fourth was the same bug I had already fixed, in the other reader. Issue324.xlsx came out as 289 rows of one cell each, arranged on a diagonal. The sheet is written like this, once per cell:

<row r="1" spans="1:13" ...><c r="A1" s="1" t="str"><v>London Metal Exchange</v></c></row>
<row r="1" spans="1:13" ...><c r="B1" s="1" t="str"><v></v></c></row>
<row r="1" spans="1:13" ...><c r="C1" s="1" t="str"><v></v></c></row>

Every <row> element carries r="1". They are one row. The scanner read the r attribute correctly and then emitted one row per element, so a 13-column sheet became 13 rows that all claimed to be row 1.

Chapter 8's whole subject was placing cells by absolute index instead of by arrival order, in the .xls reader. I fixed it there, wrote a page about why it mattered, and did not go and look at whether the .xlsx reader made the same assumption. It did. The two readers were written days apart and share nothing but a mental model, and the mental model was the bug.

After merging rows by index, one difference remained: 117 cells where the file writes <c t="str"><v></v></c> and I returned "" where the reference returns nothing. D15 already settled the shape of this question for inline strings: empty text written into the cell means absent, empty text interned in the shared table means a value. A formula's cached empty result belongs on the absent side. Applying it fixed the file and moved nothing else in the corpus, which is the test D15 used and the only reason to trust it.

Result

	Chapter 8	Now
Byte-identical	229	233
Both read it, output differs	14	10
Both refuse	33	33
Only ExcelDataReader reads	27	27
Only this library reads	0	0

96% of the files both readers accept. Four fixed, nothing regressed. I checked that by re-running the sweep against a build of HEAD as well as of the fix, so that "no regressions" is a measurement and not an assumption. The two files that had looked like regressions differ at HEAD too; they were the harness.

The remaining ten, now with none untriaged:

• Five are elapsed-time cells, where the reference returns a TimeSpan and this returns a time. A duration and an instant are different things and the interface specifies time.Time.
• Three are OOXML strict cells holding a time with no date. The reference resolves those against today's date, so its output for them changes overnight. That is the one finding here worth sending upstream. Our own output was worse, and the first afterword below is what it turned out to be.
• One is Issue425.xlsx, whitespace, and I had this one backwards for two days. See the second afterword.
• One is Issue158.xls, a single extra empty cell, attempted in chapter 8, broken worse by the guess, reverted, and still open.

Two of those ten are things I would change about this library. Neither is a mystery any more, which is the whole point of spending a day on four files that nobody had complained about.

Afterword: the bug I described wrongly

Above I wrote that this library "renders 10:59:59.9999999999984025 where it means 11:00:00", and called it a float-precision bug on our side. Going to fix it, I opened the file:

<c r="B2" s="4" t="d"><v>01:34:00.00000000000154875</v></c>

We were not rendering anything. That is the text in the file, and we were handing it back verbatim as a string, because parseISODate had layouts for a date and for a date and time but none for a time on its own. Every layout failed and the value fell through to the branch that returns the raw text.

So the diagnosis was wrong in a way worth recording. I had read our own output, seen a number that looked like floating point error, and concluded we had done the arithmetic badly. We had not done any arithmetic at all. The noise belongs to whatever produced the file, which converted an Excel serial into ISO text and wrote the serial's error into the string.

The fix is to parse the time forms, and to resolve everything a t="d" cell holds to the millisecond. That second half is not new policy. SerialToTime has rounded to the millisecond since chapter 1 and says why in a comment: the values that arrive are the result of Excel's own decimal rounding, and an exact conversion produces times like 09:59:59.9999997. A serial that took the scenic route through text is the same value with the same problem.

A time with no date is placed on the workbook's epoch day, which is where a serial holding only a fraction already lands, so the two spellings of "a time and no date" now decode to the same instant.

This costs a file. strict/BigFormatted.xlsx contains 2014-01-16T15:59:59.99999979045242400, and ExcelDataReader keeps that noise; its dump only looks clean because .NET's .fff truncates it. Rounding gives 16:00:00.000, which is the value the file means and the one Excel displays, and byte-identical parity says that is a difference. So the count goes 233 to 232 and the trade is written into the table of intentional differences.

I would rather be right than matching, but only when I can say which one I am choosing. That is the whole reason the table exists.

Second afterword: two things I said about the reference that were not true

Drafting an upstream issue meant writing a reproduction that did not go through my own harness, and two claims did not survive contact with it.

The timezone finding does not exist. Chapter 8 has a memorable little table showing strict/Open.xlsx decoding to 16:00, 11:00 and 02:00 under New York, UTC and Tokyo, and concludes that ExcelDataReader's output depends on the host. A twenty-line program that calls the library directly and prints the CLR type returns 2009-01-01 11:00:00 under all three.

The spread was mine. Release 3.9.0 returns a DateTimeOffset for those cells and attaches the host's offset to a file that carries none, and my dumper renders a DateTimeOffset through .UtcDateTime. Converting a fixed wall clock with a varying offset into UTC is what moved the number. Release 4.0.0 returns a plain DateTime and there is nothing left to see.

Something real is underneath: 3.9.0 hands you an instant that depends on where you are standing, which will bite anyone who calls .ToUniversalTime(). But that is a much smaller claim than the one I published, and it is fixed in the development branch already.

The whitespace finding is the wrong way round. Chapter 8 says the reference trims shared strings carrying xml:space="preserve", and that I think it is wrong to. Issue425.xlsx holds three strings, and the reference returns:

"   text    "     <- the one with xml:space="preserve", preserved
"text"            <- no xml:space, trimmed
"text    text"    <- no xml:space, trimmed

Which is correct. xml:space="preserve" is exactly the marker that says whitespace is significant, and absent it an application may normalise. This library preserves unconditionally, so on that file we are the ones out of step. It is now listed as a defect here rather than a difference of opinion.

Both mistakes have one cause, and it is the same one as the harness that was never committed, one turn further in. I read the reference implementation's behaviour off my own rendering of it. The dump is a lossy projection built to make two languages comparable, and I treated it as though it were the thing itself. It collapses DateTimeOffset into UTC, it collapses int and double into the same bits, and every one of those collapses is deliberate and documented. None of that makes it safe to read a conclusion about the other implementation off it.

The rule I did not have and now do: a claim about the code is proved by the harness; a claim about the other implementation has to be proved against the other implementation. Those are different tools and I own only one of them.