numbering order fixes and performance updates - #137
Open
JasonRJFleischer wants to merge 6 commits into
Open
Conversation
Multi-component lvlText (e.g. `%1.%2.%3`) was assembled by splicing apart the previous paragraph's rendered number string. That heuristic worked when a same-list precedent existed but produced incorrect output otherwise — and crashed with `ValueError: invalid literal for int() with base 10: 'F'` when the previous paragraph belonged to an unrelated list whose number contained letters (upperLetter / upperRoman formats). Resolve each %N independently from the numbering state of the same numbering family: - Walk preceding paragraphs in the same list (matched by numId, abstractNumId, or linked style), count those at the target ilvl, and stop at any reset (lower ilvl). - Combine the count with the level's <w:start> value (and any lvlOverride/startOverride). Word uses these start values to seed parent-level context — e.g. a chapter number stored as the ilvl=1 start value lets `%2` render as the chapter number in a section's multi-component lvlText, even when no chapter paragraph is itself in the numbering tree. - Format every %N substitution with the current level's numFmt, not the referenced level's. An ilvl=0 with upperRoman, seen via `%1` from an ilvl=1 decimal lvlText, renders as decimal "2", not "II" — matching what Word displays. Removes the dead `get_preceding_paragraph` helper, which only the old splicing path used.
count_same_numIds blindly added a preceding list's startOverride to the current paragraph's running count whenever the two paragraphs were in different numbering families. That worked for documents that re-key the numId at each section (the previous list's startOverride doubles as the continuation point for ours), but produced wrong numbers when the current list also carries its own startOverride. The previous list's offset got added on top of our own start, e.g. a paragraph whose numId has startOverride=5 appearing after one whose numId has startOverride=4 rendered as (9) instead of (5) — both are independent lists in Word and each should display its own startOverride. Only borrow the preceding list's startOverride when the current list does not carry an explicit reset of its own. If both paragraphs sit on numIds with startOverride > 1, treat them as independent counters: stop counting and leave num at our own startOverride. The skip-across-an-interleaved-short-list path is unchanged.
Since e966ee1, same_abstract_num() resolves abstract numbering definitions per preceding-paragraph comparison, each an uncached xpath scan over w:num plus a linear scan over w:abstractNum — making get_num_for_p O(n² × m) on large documents. Memoize get_abstractNum(numId) and hoist the get_start_override closure into a cached get_startOverride(numId, ilvl) method. Caches live on the CT_Numbering lxml proxy (kept alive by NumberingPart) and are invalidated by add_num, the sole mutation path used by set_li_lvl. Exception contracts are unchanged and nothing is cached when a lookup raises. Numbering output is byte-identical on real documents; a 5,900-paragraph docx drops from 377s to 48s (7.8×).
Paragraph numbering resolution was O(n² × m): for every paragraph, get_num_for_p walks all preceding paragraphs, and since e966ee1 each comparison resolved abstract numbering definitions via uncached xpath scans, plus a full pPr/numPr/style resolution per visited paragraph. Memoize on the CT_Numbering instance: get_abstractNum(numId), the new get_startOverride(numId, ilvl) (hoisted from the get_start_override closure), and per-paragraph (ilvl, numId) and pPr/pStyle keyed by the paragraph element. Documents are treated as immutable during a read — the same assumption Paragraph._number and the text cache already make; add_num and set_li_lvl invalidate. AttributeError contracts are preserved so the walk loops' skip logic is unchanged. Numbering output is byte-identical on real documents. A 15,000- paragraph docx drops from ~40min to 35s; a 6,000-paragraph one from 377s to 3.5s.
value_at_ilvl walks all preceding paragraphs once per %N component of a multi-component lvlText, reading pPr/pStyle directly and re-scanning the num's lvlOverrides via xpath for every component. Use the cached get_pStyle and get_startOverride instead — semantics are identical, including AttributeError propagation for a missing pPr or a lvlOverride without startOverride, but repeat lookups become dict hits, keeping multi-component documents on the same fast path as single-component ones.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description (e.g. "Related to ...", "Closes ...", etc.)
Two numbering fixes and a caching overhaul for
get_num_for_p, released as 0.8.10.41Fixes
%1.%2) is computed per-level from the family's counts and<w:start>values instead of splicing the previous paragraph's rendered string — no more wrong components orValueErrorwhen the neighbor list uses letter formats.startOverrideno longer bleeds into a list carrying its own ((9)→(5)).Performance
Numbering resolution was O(n² × m): every paragraph re-walks all preceding paragraphs, resolving abstract nums and paragraph properties from scratch each comparison. Memoize on
CT_Numbering(abstractNum, startOverride, per-paragraph ilvl/numId/pStyle), invalidated byadd_num/set_li_lvl. Output is byte-identical; a 15k-paragraph docx drops from ~40 min to 35 s.Tests (fixture tests for both fixes, cache-invalidation tests, golden updates) land in platform with the version bump.
Written by Claude
Code review checklist
core/tests/test_python-docxare updated and passing