refactor: comments state current behavior, not project history

Comments across the tree justified the code by pointing at a Rust
implementation that is no longer in the repository, citing files and line
numbers (config.rs:132, schema.rs:26-54, reader.rs:42) that cannot be
opened, plus crates and datasets that are equally gone. A reader could not
check any of it.

Every invariant those comments carried is kept and restated so it stands on
its own: the bytewise sort that decides which row survives a duplicate exam
number, the literal U+0300..U+036F range that must match the site's toAscii,
the trailing space in "SINH ", the BIFF and shared-string corrections, the
VACUUM-after-COMMIT rule, the deploy-from-main guard.

The reader's contract is now anchored to the frozen oracle in
parser/testdata, which still exists and is still checked, rather than to the
tool that originally produced it.

TestDDLMatchesRust becomes TestDDLIsFrozen: it compares against a copy of
the DDL inside the test and never read schema.rs, so both the name and the
failure message were misleading.

ToAscii no longer claims the d-replacement must precede lowercasing. Both
cases map to 'd' and ToLower runs last, so the order has no effect.
This commit is contained in:
tiennm99 committed 2026-08-14 09:40:41 +07:00
1 parent c988bfafcf
commit 933adf70c9
43 files changed
+392 -515

No files matched your search

+15 -14
View File
@@ -13,19 +13,20 @@ import (
"github.com/tiennm99/thptqg/parser/internal/reader"
)
// TestReaderFidelity asserts the Go reader reproduces calamine byte-for-byte on
// every real input file.
// TestReaderFidelity asserts the reader reproduces the reference rendering
// byte-for-byte on every real input file.
//
// The oracle is a committed SHA-256 per file over a canonical cell dump. The
// dumps themselves are real student names and birthdates, so only the hashes are
// committed — regenerate the dumps from the Rust side on demand
// (parser/examples/dump_cells.rs), which is possible because parser/ still
// builds.
// The oracle is parser/testdata/reader-fidelity-hashes.tsv: one SHA-256 per
// file over a canonical cell dump. The dumps are real student names and
// birthdates, so only the hashes are committed. The manifest is FROZEN — it is
// the only guard on reader output, so a mismatch is a reader bug until proven
// otherwise, never a cue to regenerate the hashes.
//
// The canonical form carries geometry and rendered cell values only. Whether a
// cell was absent or an empty string is excluded on purpose: both render "" and
// both count as blank in IsAllBlank and transform, so the distinction cannot
// reach the database.
//
// The canonical form carries geometry and rendered cell values. The calamine
// Data variant is excluded on purpose: Data::Empty and Data::String("") both
// render "" and both count as blank in is_all_blank and transform, so the
// distinction cannot reach the database.
// Runs by default so CI and `go test ./...` keep the full guarantee; skipped
// under -short, which is how to iterate without paying ~77s to re-read 418 MB.
func TestReaderFidelity(t *testing.T) {
@@ -65,7 +66,7 @@ func TestReaderFidelity(t *testing.T) {
t.Fatalf("hash %s: %v", rel, err)
}
if got != want {
t.Errorf("cell dump diverges from calamine\n want %s\n got %s", want, got)
t.Errorf("cell dump diverges from the frozen oracle\n want %s\n got %s", want, got)
}
})
}
@@ -77,8 +78,8 @@ func TestReaderFidelity(t *testing.T) {
}
}
// canonicalHash renders one file in the canonical form and hashes it. Kept
// byte-identical to the awk canonicalisation used to build the manifest.
// canonicalHash renders one file in the canonical form and hashes it. The byte
// format is fixed: any change to it invalidates every hash in the manifest.
func canonicalHash(path string) (string, error) {
wb, err := reader.Open(path)
if err != nil {
+15 -14
View File
@@ -1,6 +1,9 @@
// Package reader wraps the two spreadsheet libraries behind one streaming
// contract, mirroring parser/src/reader.rs (which wraps calamine's
// open_workbook_auto for both formats).
// Package reader wraps the two spreadsheet libraries behind one contract.
//
// Cell rendering and sheet geometry are pinned by the frozen fidelity oracle in
// parser/testdata/reader-fidelity-hashes.tsv. "The reference" below means that
// pinned rendering: whenever an underlying library disagrees with it, this
// package corrects the library, never the oracle.
//
// The *contract* is row-at-a-time: callers receive one row per callback and
// never hold the workbook. The two implementations do not honour that in
@@ -16,21 +19,19 @@ import (
"strings"
)
// Cell is one spreadsheet cell rendered the way calamine's Data::to_string()
// renders it.
// Cell is one spreadsheet cell rendered as reference text.
//
// IsEmpty tracks calamine's Data::Empty variant separately from a string cell
// that happens to be empty. Rust distinguishes them at reader.rs:42, but both
// render "" and both count as blank in is_all_blank and in transform, so the
// flag is diagnostic only — never compare on it.
// IsEmpty marks a cell with no value, as opposed to a string cell that happens
// to be empty. Both render "" and both count as blank in IsAllBlank and in
// transform, so the flag is diagnostic only — never compare on it.
type Cell struct {
Str string
IsEmpty bool
}
// Sheet carries a sheet's identity and geometry. Height and Width describe the
// used range, matching calamine's Range::height()/width(); every used range in
// the corpus starts at (0,0), verified across every input file.
// reference used range; every used range in the corpus starts at (0,0),
// verified across every input file.
type Sheet struct {
Index int
Name string
@@ -40,8 +41,8 @@ type Sheet struct {
// RowFunc receives each row of each sheet. Rows are padded to the sheet's used
// width — width is load-bearing because every column read downstream is
// positional with an unwrap_or_default() equivalent, so a short row silently
// NULLs its tail columns.
// positional and falls back to "" when the index is out of range, so a short
// row silently NULLs its tail columns.
type RowFunc func(sheet Sheet, rowIdx int, row []Cell) error
// Workbook is one opened spreadsheet.
@@ -53,7 +54,7 @@ type Workbook interface {
Close() error
}
// Open dispatches on file extension, mirroring calamine's open_workbook_auto.
// Open dispatches on file extension.
func Open(path string) (Workbook, error) {
switch strings.ToLower(filepath.Ext(path)) {
case ".xls":
+10 -12
View File
@@ -10,18 +10,16 @@ import (
// xlsWorkbook reads legacy BIFF through pbnjay/grate.
//
// grate replaced extrame/xls, which was measured against calamine ground truth
// and found to corrupt 69% of cells and drop a further 28%: undecoded UTF-16LE
// and BIFF record framing leaked into cell values, content moved between rows
// and columns, and tail rows came back blank. That was charset-independent and
// unfixable from the outside. grate reproduces calamine exactly on the same
// files.
// Do not swap grate back for extrame/xls: on this corpus it corrupted 69% of
// cells and dropped a further 28% — undecoded UTF-16LE and BIFF record framing
// leaked into cell values, content moved between rows and columns, and tail
// rows came back blank. Charset-independent, and unfixable from the outside.
//
// The one normalisation grate needs is trailing blank rows: it yields rows past
// the end of calamine's used range (one for a populated sheet, two for an empty
// one), so trailing all-blank rows are trimmed. Note this is the opposite of
// the xlsx path, where excelize already trims and calamine keeps a 1x1 empty
// range — in both cases the rule is "match calamine's used range".
// the end of the reference used range (one for a populated sheet, two for an
// empty one), so trailing all-blank rows are trimmed. Note this is the opposite
// of the xlsx path, where excelize already trims and the reference keeps a 1x1
// empty range — in both cases the rule is "match the reference used range".
type xlsWorkbook struct {
sheets []Sheet
rows [][][]Cell
@@ -56,7 +54,7 @@ func openXLS(path string) (Workbook, error) {
raw = append(raw, cp)
}
// Trim to calamine's used range.
// Trim to the reference used range.
height := len(raw)
for height > 0 && rowAllBlank(raw[height-1]) {
height--
@@ -87,7 +85,7 @@ func openXLS(path string) (Workbook, error) {
// demergeMarker blanks grate's merged-cell continuation markers.
//
// grate fills the cells covered by a merge with sentinel runes; calamine
// grate fills the cells covered by a merge with sentinel runes; the reference
// reports them as empty. In this corpus they occur only in the merged title
// block of the 2016 spreadsheets (19 cells in rows 0-2 of one file). Only an
// exact whole-value match is blanked, so a real cell that merely contains an
+14 -13
View File
@@ -8,12 +8,12 @@ import (
// xlsxWorkbook reads OOXML through excelize.
//
// Two excelize behaviours must be corrected to match calamine:
// Two excelize behaviours must be corrected to match the reference:
//
// 1. GetRows applies the cell number format by default, while calamine renders
// the underlying value. RawCellValue: true disables that.
// 2. GetRows trims trailing blank cells, so rows are ragged; calamine returns a
// rectangular used range. Rows are padded back out to the sheet width.
// 1. GetRows applies the cell number format by default, while the reference
// renders the underlying value. RawCellValue: true disables that.
// 2. GetRows trims trailing blank cells, so rows are ragged; the reference used
// range is rectangular. Rows are padded back out to the sheet width.
type xlsxWorkbook struct {
f *excelize.File
sheets []Sheet
@@ -35,15 +35,16 @@ func openXLSX(path string) (Workbook, error) {
return nil, fmt.Errorf("excelize GetRows %s/%s: %w", path, name, err)
}
// Do NOT trim trailing blank rows: calamine's used range keeps them, and
// excelize's GetRows already drops trailing fully-empty rows itself.
// Do NOT trim trailing blank rows: the reference used range keeps them,
// and excelize's GetRows already drops trailing fully-empty rows itself.
//
// One correction is needed. 63 sheets in 2017-old and 53 in 2017-old2
// hold a single empty shared-string cell at A1; calamine reports those
// as a 1x1 range, while GetRows returns nothing. A genuinely empty sheet
// (230 of them in 2016) is height 0 on both sides. GetCellType tells the
// two apart: the empty-shared-string cell exists in the XML and types as
// CellTypeSharedString, an absent cell types as CellTypeUnset.
// One correction is needed. A sheet holding a single empty
// shared-string cell at A1 is a 1x1 range to the reference, while
// GetRows returns nothing for it. A genuinely empty sheet (230 of them
// in 2016) is height 0 either way.
// GetCellType tells the two apart: the empty-shared-string cell exists
// in the XML and types as CellTypeSharedString, an absent cell types as
// CellTypeUnset.
if len(raw) == 0 {
if t, terr := f.GetCellType(name, "A1"); terr == nil && t != excelize.CellTypeUnset {
raw = [][]string{{""}}
+5 -4
View File
@@ -10,10 +10,11 @@ import (
"github.com/xuri/excelize/v2"
)
// normalizeNumeric reproduces calamine's rendering of a numeric cell.
// normalizeNumeric reproduces the reference rendering of a numeric cell.
//
// calamine parses a numeric cell to f64 and renders it with Rust's f64 Display,
// so the stored literal "6.0" becomes "6". RawCellValue hands back the literal.
// The reference parses a numeric cell to a float and prints its shortest
// round-tripping form, so the stored literal "6.0" becomes "6". RawCellValue
// hands back the literal instead.
//
// The cell type must be consulted, not guessed: "01063476", "6.00" and "NAN" are
// all shared strings that survive ParseFloat, and renumbering them would drop a
@@ -54,7 +55,7 @@ func normalizeNumeric(f *excelize.File, sheet string, col, row int, v string) st
//
// Go's encoding/xml performs the line-ending normalisation the XML 1.0 spec
// mandates (CRLF and lone CR both become LF), so excelize returns "a\nb" where
// calamine — which reads the raw bytes — returns "a\r\nb". That difference
// the reference — which reads the raw bytes — returns "a\r\nb". That difference
// reaches the database: in one 2016 file it affects 2,233 TEN_CUMTHI values,
// which populate the ten_cum_thi column.
//