Skip to content

feat(autoindex): fill vertically merged XLSX cells down the rows they cover - #1132

Open
thomas-villani wants to merge 1 commit into
xerj-org:mainfrom
thomas-villani:feat/xlsx-merged-cells
Open

thomas-villani wants to merge 1 commit into
xerj-org:mainfrom
thomas-villani:feat/xlsx-merged-cells

Conversation

@thomas-villani

Copy link
Copy Markdown
Contributor

What this changes, and why

A cell merged down over several rows stores its value only in the top row. pandas does this for every MultiIndex export (df.groupby([...]).sum().to_excel(...), where merge_cells=True is the default), and hand-made reports do it for a category label beside its line items. #1124 read only the top cell, so the value was missing from every other row:

pandas groupby export, region merged A2:A4 and A5:A6
  before  Sales!r3 {"product":"B","sales":20}
  after   Sales!r3 {"region":"East","product":"B","sales":20}

region: East matched 1 of East's 3 rows, and a terms aggregation on region undercounted.

Mechanism. <mergeCells> comes after <sheetData>, but rows are streamed and emitted as they are read. So each sheet gets a pre-pass, scan_merges, that streams the part once without parsing cells:

  • It byte-searches for the <mergeCells tag with memchr::memmem. The search is unambiguous, because < cannot appear unescaped in XML text.
  • It XML-parses only the tail from that tag on, capped at 16 MB, so a following <hyperlink ref=…> is not mistaken for a merge.

Then FillDown gives each streamed row the value of any vertical merge that covers it, in the merge's own top-left column.

Deliberately not done:

  • Merges across columns are not expanded. A title merged over a table's width has to stay one cell, or it turns into a header-shaped row. A block merge (A3:B4) fills down column A only.
  • No row is invented. A covered row with no cells of its own stays absent.
  • A covered cell that has its own value keeps it (that only happens in a malformed file).
  • Two-row headers (pandas MultiIndex columns, with Q1 over Jan/Feb, currently give Jan, Jan_2) are left for a follow-up PR.

Sampling. A sampling run (phase A and --dry-run, 500 rows per sheet) has to stay cheap, so it skips the pre-pass on sheets whose declared decompressed size is over 64 MB. A sample of such a sheet gets no fill-down. Full runs always scan. If you'd rather keep sampling and full runs identical regardless of cost, it's a one-line change.

Evidence

Release build, median of 3 runs, in a rust:latest container. The full time includes the pre-pass.

workbook sheet XML records vertical merges full extract pre-pass share
openpyxl, 300k rows × 8 112.6 MB 300,000 0 6.274 s 0.640 s 10.2%
pandas groupby export 1.5 MB 14,178 1,204 0.067 s 0.011 s 16.8%

There are six new tests:

  • a_vertical_merge_gives_its_value_to_every_row_it_covers
  • merges_across_columns_are_not_expanded
  • fill_down_invents_no_rows_and_overwrites_no_values
  • the_merge_scan_finds_the_tag_and_only_the_tag (a prefixed tag, a tag split across 7-byte reads, mergeCells inside cell text, the tail cap)
  • a_sampling_run_skips_the_merge_scan_only_on_a_large_sheet
  • cell_references_parse_to_column_and_row

With fill.apply disabled, the three behavior tests fail:

extract::xlsx::tests::a_vertical_merge_gives_its_value_to_every_row_it_covers
extract::xlsx::tests::fill_down_invents_no_rows_and_overwrites_no_values
extract::xlsx::tests::merges_across_columns_are_not_expanded
test result: FAILED. 19 passed; 3 failed

With the fix:

cargo test -p xerj-autoindex --lib  ->  test result: ok. 1195 passed; 0 failed; 2 ignored

Checks

  • cargo fmt --all (--check clean)
  • Scoped release build: cargo build --release -j 32 -p xerj-autoindex
  • cargo test -p xerj-autoindex --lib passes
  • ES-YAML conformance suite: not run. The change is confined to the autoindex XLSX extractor, a client-side crate, so no engine or wire behavior changes.
  • New ES-compatible YAML case: N/A
  • Docs: see the note below
  • CONTRIBUTION_REVIEW audit: not run in full. Failure atomicity: when the pre-pass fails or runs out of budget, it yields no merges and the rows stream exactly as before.

Not run / notes:

  • failure_resume_http_tests::a_run_reports_progress_through_every_phase_and_closes_the_stream fails when run alone on this machine (3/3). It fails identically on clean main (3/3), and it passes inside the full suite. It asserts on wall-clock time ("a sub-2s run relays exactly one bar"), and the host was under heavy load from another job, so I'm treating it as environmental and haven't filed it.
  • Docs: docs: list .pptx/.xlsx as extracted formats; retire "no XLSX/PPTX extractor" #1130 (open) writes "merged cells not expanded" into llms-full.txt and into the fact-check gate for .xlsx. Whichever of the two PRs lands second, I'll update that wording to "vertical merges filled down; merges across columns not expanded".

Provenance

  • Written by: AI agent (Claude Code, Claude Opus 5.5), run by @thomas-villani, who reviewed it.
  • Verified: every command and number above. The real-file check covered pandas MultiIndex-rows exports and an openpyxl report with a merged title and merged categories; the probe test used for it was removed before commit.
  • Assumed: merge lists written by Excel itself, as opposed to openpyxl or pandas, look the same, since the format defines one <mergeCells> block after <sheetData>. No Excel-authored file was tested. The 64 MB sampling threshold comes from one measured scan rate (about 176 MB/s on this host) and is not tuned beyond that.

🤖 Generated with Claude Code

… cover

Motivation: a cell merged DOWN over several rows stores its value only in
the top row. pandas writes exactly that for every MultiIndex export
(`df.groupby([...]).sum().to_excel(...)`, merge_cells=True is the default),
and hand-made reports do it for a category beside its line items. xerj-org#1124
read only the top cell, so for a pandas groupby export with region merged
A2:A4 / A5:A6:

  before  Sales!r3 {"product":"B","sales":20}           (no region)
  after   Sales!r3 {"region":"East","product":"B","sales":20}

`region: East` matched 1 of East's 3 rows and a `terms` aggregation on
region undercounted. Same for a hand-made report's merged Category column.

Mechanism: `<mergeCells>` follows `<sheetData>`, but rows are streamed and
emitted as read. So each sheet gets a pre-pass, `scan_merges`, that streams
the part once WITHOUT parsing cells: it byte-searches (memchr::memmem) for
the `<mergeCells` tag — unambiguous, since `<` cannot appear unescaped in
XML text — and XML-parses only the tail from there (capped at 16 MB),
which keeps a following `<hyperlink ref=...>` from being read as a merge.
`FillDown` then gives each streamed row the value of any live vertical
merge covering it, in the merge's own (top-left) column.

Deliberately NOT done:
- Merges across columns are not expanded: a title merged over a table's
  width must stay one cell, or it becomes a header-shaped row. A block
  merge (A3:B4) fills down column A only.
- No row is invented: a covered row with no cells of its own stays absent.
- A covered cell that has its own value (malformed file) keeps it.
- Two-row headers (pandas MultiIndex columns: Q1 over Jan/Feb) are a
  separate change.

Cost, measured (release, median of 3, rust:latest container; full time
includes the pre-pass):
  300k x 8 sheet, 112.6 MB XML   full 6.274 s   pre-pass 0.640 s  (10.2%)
  pandas groupby, 1.5 MB, 1204 vertical merges
                                 full 0.067 s   pre-pass 0.011 s  (16.8%)
The pre-pass is charged to the decompression budget like any read.
A sampling run (phase A / --dry-run, 500 rows per sheet) must stay cheap,
so it skips the pre-pass on sheets declared over 64 MB decompressed
(~0.35 s of scanning at the measured rate); such a sample has no
fill-down. Full runs always scan.

Tests (extract/xlsx.rs):
- a_vertical_merge_gives_its_value_to_every_row_it_covers (pandas shape,
  merges out of order, hyperlink ref after the list)
- merges_across_columns_are_not_expanded (merged title, block merge)
- fill_down_invents_no_rows_and_overwrites_no_values
- the_merge_scan_finds_the_tag_and_only_the_tag (prefixed tag, tag split
  across 7-byte reads, "mergeCells" in cell text, tail cap)
- a_sampling_run_skips_the_merge_scan_only_on_a_large_sheet
- cell_references_parse_to_column_and_row
With `fill.apply` disabled, the three behavior tests fail; with it, all
pass. Real pandas/openpyxl workbooks (MultiIndex rows, merged-title
report) checked with a temporary probe, removed before commit.

Evidence: cargo test -p xerj-autoindex --lib: 1195 passed, 0 failed,
2 ignored. cargo build --release -p xerj-autoindex ok; clippy -D warnings
clean; cargo fmt --check clean.

Written by an AI agent (Claude Code) on behalf of the PR author.
@cla-bot cla-bot Bot added the cla-signed label Oct 4, 2026
thomas-villani added a commit to thomas-villani/xerj that referenced this pull request Oct 4, 2026
…org#1132-xerj-org#1134)

Motivation: xerj-org#1132 fills vertical merges down, xerj-org#1133 names columns from a
two-row grouped header (Q1_Jan) and xerj-org#1134 adds a roff man(7) extractor.
The format lists and the fact-check matrix should say so. This commit
must merge AFTER those three code PRs, because it describes them as shipped.

- README, landing/llms.txt, landing/llms-full.txt: man pages added to the
  format lists (llms-full: plain or gzipped, one record per section, titled
  NAME(SECT), mdoc(7) pages stay plain text). The XLSX entry in llms-full
  now says vertical merges are filled down, a two-row grouped header is
  combined, and merges across columns are not expanded. It replaces
  "merged cells not expanded".
- scripts/seo/claims_rules.py: new GREEN THING row "Man pages (roff man(7))"
  citing extract/man.rs:1, and its gate says mdoc is not parsed. The .xlsx
  gate gets the same merged-cell wording as llms-full.
- testdata/factcheck: new good_man_answer.md fixture (git check-ignore
  confirms the !scripts/seo/testdata/**/*.md re-include applies), and
  good_xlsx_answer.md loses the old "merged cells are not expanded" line.

Verified: factcheck --self-test OK (35 THING rows); --fail-on error 0 ERROR;
build_articles --check current; landing-constants-guard passes.
--fixture-check reports 1 false positive (FC-EV-DANGLING) on this branch
alone, because good_man_answer.md cites extract/man.rs, which only exists
once xerj-org#1134 merges. With man.rs from xerj-org#1134 checked out it reports 0 false
positives and 13/13 good fixtures clean.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant