amadan

oficina / sheets Public

tools/gencorpus: bring cases to parity with the committed corpus

Code review found gencorpus's case list had 41 entries against the
committed corpus's 60 -- 7 missing were VLOOKUP/INDEX/MATCH (a
structural gap, since a flat expression list has nowhere to put a
table_array), but 9 more were plain scalars with no excuse
(IF(FALSE,"yes","no"), OR(FALSE,FALSE), NOT(FALSE),
IFERROR(5,"fallback"), LEFT("hello"), RIGHT("hello"), LEN(""), ABS(7),
CONCAT("a",1,"b")), plus 3 more that read a genuinely blank cell
(AVERAGE(A1,A2,A3), COUNT(A1,C1), COUNTA(A1,C1)). As it stood, running
this tool would have produced a SMALLER corpus and overwritten the
file's own honest "hand-authored, unverified" note with a claim of
full generation.

Fixed to actual parity, not just a fail-loud stopgap:

- The 9 unexcused scalars are added directly.
- writeODS now also writes a small fixed lookup table (10/20/30 ->
  ten/twenty/thirty, mirroring corpusEnv/lookupTable() in the calc
  package's own tests) into columns B:C, rows 1-3, as real ODF value
  cells (office:value-type, not formula cells) -- column A there is
  already occupied by each row's own formula case, so this table lives
  beside it rather than colliding with it. The 10 cell-reference cases
  (VLOOKUP/INDEX/MATCH x7, COUNT/COUNTA/AVERAGE-of-a-blank-cell x3) now
  reference B1:C3/B1:B3/B1/D1 -- not the hand-authored corpus's A1:B3,
  since that convention was never available to this tool's own
  self-contained sheet. Documented in cases' doc comment as an expected,
  fine divergence: a real gencorpus run replaces the hand-authored file
  wholesale, it doesn't need field-by-field text agreement with it.
- cases is now exactly 60 entries with the same per-function coverage
  as testdata/corpus/formulas.json (verified: zero mismatches, by
  function name and count, between the two lists).

Also added a narrowing guard for the future: before writing, main now
reads any existing file at -out and refuses to proceed if it already
has more entries than this run is about to produce (existingEntryCount,
corpus.go) -- so a case accidentally dropped from the list later fails
loudly instead of silently shrinking the committed corpus the way this
review just caught happening once already.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01APtNStXDZ5H2fAc1ZuGqtS
Paul Campbell 17e26e20ec367ec558559a94d74748ea68c5fd8d parent 4f12eaf
3 files changed, +115 −16
  • tools/gencorpus/corpus.go +17 −0
  • tools/gencorpus/main.go +58 −14
  • tools/gencorpus/ods.go +40 −2

Intent

full ledger: core

diff --git a/tools/gencorpus/corpus.go b/tools/gencorpus/corpus.go
index b06166e..41dd80e 100644
--- a/tools/gencorpus/corpus.go
+++ b/tools/gencorpus/corpus.go
@@ -94,3 +94,20 @@ func writeCorpusJSON(path string, cases []corpusCase, results []string) error {
}
return os.WriteFile(path, append(data, '\n'), 0o644)
}
+
+// existingEntryCount reads path's current entry count, if the file
+// exists and parses — used by main to refuse a run that would silently
+// narrow an already-committed corpus. A missing or unparsable file
+// returns a non-nil error, which main treats as "nothing to compare
+// against" rather than a reason to fail.
+func existingEntryCount(path string) (int, error) {
+ data, err := os.ReadFile(path)
+ if err != nil {
+ return 0, err
+ }
+ var cf corpusFileJSON
+ if err := json.Unmarshal(data, &cf); err != nil {
+ return 0, err
+ }
+ return len(cf.Entries), nil
+}
diff --git a/tools/gencorpus/main.go b/tools/gencorpus/main.go
index 2882846..201612c 100644
--- a/tools/gencorpus/main.go
+++ b/tools/gencorpus/main.go
@@ -50,7 +50,7 @@ import (
// corpusCase is one row to feed LibreOffice: Function names the
// spreadsheet function under test (matched against
// testdata/corpus/volatile-excluded.txt and used for
-// TestAgainstGoldenCorpus's per-row skip), Expr is the formula text as a
+// TestAgainstReferenceCorpus's per-row skip), Expr is the formula text as a
// person would type it after "=" — Excel/English syntax, comma argument
// separators; toODFFormula (ods.go) translates to ODF's own syntax for
// the file this tool actually writes.
@@ -59,13 +59,33 @@ type corpusCase struct {
Expr string
}
-// cases mirrors testdata/corpus/formulas.json's hand-authored entries as
-// closely as a flat, cell-reference-free list can — VLOOKUP/INDEX/MATCH
-// need real cell references (a table_array) that a standalone expression
-// list cannot supply; see the comment below the SUM/AVERAGE/... rows.
-// Keep this list in step with the hand-authored corpus when adding a new
-// function family, so a real gencorpus run reproduces (and can then
-// replace) the same shape of corpus rather than a smaller one.
+// cases has the same 60-case shape and per-function coverage as
+// testdata/corpus/formulas.json's hand-authored entries — a prior
+// version of this list stopped at 41, silently dropping 9 plain scalar
+// cases with no structural excuse plus the entire VLOOKUP/INDEX/MATCH
+// family and three more cases that read a genuinely blank cell, which
+// would have made a real run overwrite the corpus's honest
+// "hand-authored, unverified" note with a claim of full generation while
+// actually producing a SMALLER corpus. Caught by code review, not by
+// this tool running successfully, since nothing here checked its own
+// output size against the file it was about to overwrite.
+//
+// The ten cases that need real cell references (VLOOKUP/INDEX/MATCH, and
+// the three that read a deliberately blank cell) use writeODS's own
+// fixed lookup table at B1:C3 (see ods.go's lookupTableRows) rather than
+// A1:B3, because column A here is already occupied by each row's own
+// formula case — row 1 is SUM's first case, not lookup data. That means
+// their Expr text will NOT come out byte-identical to
+// formulas.json's hand-authored VLOOKUP/INDEX/MATCH rows (which use
+// A1:B3, matching corpus_test.go's own corpusEnv) — expected and fine;
+// a real gencorpus run replaces the hand-authored file wholesale, it
+// does not need to diff field-by-field against it.
+//
+// Keep this list at parity with testdata/corpus/formulas.json — same
+// per-function coverage, not necessarily identical cell references —
+// whenever a new function family is added, so a real gencorpus run
+// always produces a corpus at least as large as the one it replaces,
+// never a smaller one that silently narrows coverage.
var cases = []corpusCase{
{"SUM", "SUM(1,2,3)"},
{"SUM", "SUM(1,2,3,4,5)"},
@@ -73,8 +93,11 @@ var cases = []corpusCase{
{"SUM", "SUM(1.5,2.5)"},
{"AVERAGE", "AVERAGE(2,4,6)"},
{"AVERAGE", "AVERAGE(1,2,3,4)"},
+ {"AVERAGE", "AVERAGE(B1,B2,B3)"},
{"COUNT", `COUNT(1,2,"a",TRUE)`},
+ {"COUNT", "COUNT(B1,D1)"},
{"COUNTA", `COUNTA(1,2,"a",TRUE)`},
+ {"COUNTA", "COUNTA(B1,D1)"},
{"MIN", "MIN(4,2,9,1)"},
{"MAX", "MAX(4,2,9,1)"},
{"ROUND", "ROUND(3.14159,2)"},
@@ -85,6 +108,7 @@ var cases = []corpusCase{
{"ROUNDDOWN", "ROUNDDOWN(3.99,0)"},
{"ROUNDDOWN", "ROUNDDOWN(-3.99,0)"},
{"ABS", "ABS(-7)"},
+ {"ABS", "ABS(7)"},
{"SQRT", "SQRT(16)"},
{"SQRT", "SQRT(2)"},
{"POWER", "POWER(2,10)"},
@@ -93,27 +117,36 @@ var cases = []corpusCase{
{"MOD", "MOD(-10,3)"},
{"MOD", "MOD(10,-3)"},
{"IF", `IF(TRUE,"yes","no")`},
+ {"IF", `IF(FALSE,"yes","no")`},
{"IF", `IF(FALSE,1/0,5)`},
{"IFS", "IFS(FALSE,1,TRUE,2,TRUE,3)"},
{"AND", "AND(TRUE,TRUE,TRUE)"},
{"AND", "AND(TRUE,FALSE)"},
{"OR", "OR(FALSE,FALSE,TRUE)"},
+ {"OR", "OR(FALSE,FALSE)"},
{"NOT", "NOT(TRUE)"},
+ {"NOT", "NOT(FALSE)"},
{"IFERROR", `IFERROR(1/0,"fallback")`},
+ {"IFERROR", `IFERROR(5,"fallback")`},
{"CONCAT", `CONCAT("foo","bar")`},
+ {"CONCAT", `CONCAT("a",1,"b")`},
{"LEFT", `LEFT("hello",3)`},
+ {"LEFT", `LEFT("hello")`},
{"RIGHT", `RIGHT("hello",3)`},
+ {"RIGHT", `RIGHT("hello")`},
{"MID", `MID("hello",2,3)`},
{"LEN", `LEN("hello")`},
+ {"LEN", `LEN("")`},
{"TRIM", `TRIM(" a b ")`},
{"UPPER", `UPPER("MixedCase")`},
{"LOWER", `LOWER("MixedCase")`},
- // VLOOKUP/INDEX/MATCH need real cell references, which this flat
- // list of standalone expressions cannot supply on their own — a
- // human extending this tool for the lookup family should add a
- // small fixed table to a few cells in the sheet (e.g. columns C:D)
- // alongside the formula column, and reference it here, rather than
- // trying to express a table_array as a formula literal.
+ {"VLOOKUP", "VLOOKUP(20,B1:C3,2,FALSE)"},
+ {"VLOOKUP", "VLOOKUP(25,B1:C3,2,TRUE)"},
+ {"VLOOKUP", "VLOOKUP(99,B1:C3,2,FALSE)"},
+ {"INDEX", "INDEX(B1:C3,2,2)"},
+ {"INDEX", "INDEX(B1:C3,3,1)"},
+ {"MATCH", "MATCH(20,B1:B3,0)"},
+ {"MATCH", "MATCH(25,B1:B3,1)"},
}
func main() {
@@ -134,6 +167,17 @@ func main() {
}
defer os.RemoveAll(tmpDir)
+ // Refuse to silently narrow an existing corpus: if *out already has
+ // more entries than this run is about to produce, something regressed
+ // (a case dropped from the list below without a function actually
+ // going away) and overwriting it would replace an honest "hand-
+ // authored, unverified" corpus — or a real one — with a smaller file
+ // that still claims full coverage. See cases' own doc comment for the
+ // concrete incident this guards against.
+ if existing, err := existingEntryCount(*out); err == nil && existing > len(cases) {
+ fatal(fmt.Errorf("gencorpus: %s already has %d entries; this run would only produce %d — refusing to narrow the corpus (add the missing cases to gencorpus's own list, don't just let it overwrite)", *out, existing, len(cases)))
+ }
+
odsPath := filepath.Join(tmpDir, "corpus.ods")
if err := writeODS(odsPath, cases); err != nil {
fatal(err)
diff --git a/tools/gencorpus/ods.go b/tools/gencorpus/ods.go
index 10631aa..d5e154f 100644
--- a/tools/gencorpus/ods.go
+++ b/tools/gencorpus/ods.go
@@ -4,11 +4,14 @@ import (
"archive/zip"
"fmt"
"os"
+ "strconv"
"strings"
)
// writeODS writes a minimal OpenDocument spreadsheet at path with one
-// formula per row in column A. See main.go's package doc comment for the
+// formula per row in column A, plus a small fixed lookup table in
+// columns B:C, rows 1-3 (see lookupTableRows) that the VLOOKUP/INDEX/
+// MATCH cases reference. See main.go's package doc comment for the
// overall mechanism and this tool's untested status.
func writeODS(path string, cases []corpusCase) error {
f, err := os.Create(path)
@@ -56,11 +59,33 @@ const manifestXML = `<?xml version="1.0" encoding="UTF-8"?>
</manifest:manifest>
`
+// lookupTableRows is the fixed B:C data VLOOKUP/INDEX/MATCH cases
+// reference as "B1:C3" (numbers) or "B1:B3" (the number column alone) —
+// the same 10/20/30 -> ten/twenty/thirty table
+// internal/sheets/calc/fn_lookup_test.go's lookupTable() and
+// corpus_test.go's corpusEnv use, just shifted one column right because
+// column A here is already occupied by each row's own formula case (row
+// 1 is SUM's first case, not lookup data — the table lives beside it,
+// not inside it). D1 is deliberately left unwritten so it reads as a
+// genuinely blank cell for the COUNT(B1,D1)/COUNTA(B1,D1) cases.
+var lookupTableRows = []struct {
+ num float64
+ text string
+}{
+ {10, "ten"},
+ {20, "twenty"},
+ {30, "thirty"},
+}
+
func contentXML(cases []corpusCase) string {
var rows strings.Builder
- for _, c := range cases {
+ for i, c := range cases {
rows.WriteString(" <table:table-row>")
rows.WriteString(fmt.Sprintf(`<table:table-cell table:formula="of:=%s"/>`, toODFFormula(c.Expr)))
+ if i < len(lookupTableRows) {
+ rows.WriteString(numberCell(lookupTableRows[i].num))
+ rows.WriteString(stringCell(lookupTableRows[i].text))
+ }
rows.WriteString("</table:table-row>\n")
}
return `<?xml version="1.0" encoding="UTF-8"?>
@@ -78,6 +103,19 @@ func contentXML(cases []corpusCase) string {
`
}
+// numberCell/stringCell write a literal (not formula) value cell — the
+// lookup table's own data, which must be reflected in the file's actual
+// value/type attributes, not just visible text, or LibreOffice would not
+// see it as a number/string for VLOOKUP/INDEX/MATCH to compare against.
+func numberCell(n float64) string {
+ s := strconv.FormatFloat(n, 'g', -1, 64)
+ return fmt.Sprintf(`<table:table-cell office:value-type="float" office:value="%s"><text:p>%s</text:p></table:table-cell>`, s, s)
+}
+
+func stringCell(s string) string {
+ return fmt.Sprintf(`<table:table-cell office:value-type="string"><text:p>%s</text:p></table:table-cell>`, s)
+}
+
// toODFFormula translates a comma-argument-separated Excel-style formula
// (this tool's own case list, and this project's own formula syntax —
// see internal/sheets/calc/parse.go) into ODF's own formula syntax, which