fix: displayed fractions fuse the relation sign with the denominator ('=dt') (#830) - #831
Conversation
yfedoseev
left a comment
There was a problem hiding this comment.
Reviewed this against the PDF spec and SOLID/KISS/DRY (clean — single-responsibility fix, minimal boundary-condition change, well-commented, no issues there), then ran the full 419-doc corpus sweep against main and read every diff. Good news and a real problem, in that order.
Confirmed working as intended
- All 3 unit tests pass, including the real-document one (skips gracefully without the gitignored fixture, as designed).
- Bonus fix, unclaimed but real:
maintimes out/crashes onbooks/war_peace.pdfin our corpus; this branch extracts it successfully. Likely the old unbounded-gap merge created pathological merge-chain behavior there that the new bounded floor avoids. - The TOC/dot-leader/multi-column fusion case this PR targets works well — confirmed genuine improvements across ~80% of the corpus (page-number un-fusion, IRS form table reconstruction, fanzine packet-header un-fusion, etc.).
Regression 1 (critical) — RTL text broken
A_golden/multilingual/wiki-cat-ar/source.pdf — on main, this Arabic document extracts as 5 clean RTL paragraphs. On this branch, the same content shreds into a fake <table> with scrambled, reversed word fragments (word_jaccard 0.03).
Root-caused with real geometry, not a guess — pulled the actual word bboxes via extract_words(): consecutive words in natural RTL reading order have gaps of -8 to -159 points. That's because RTL text is drawn right-to-left, so each subsequent word's origin sits well left of the previous word's end — completely normal RTL layout. The new merge floor (gap >= -(font_size * 0.5)) and line-emitter branch (gap < -fs) were tuned against the fraction-denominator case (~-24pt at 12pt text) but have no way to distinguish that from ordinary RTL flow, which produces gaps of the same sign and often much larger magnitude. This isn't a one-off — it's structural, and will hit any RTL-heavy document (the corpus just doesn't have many to fully demonstrate the blast radius; I checked every other document with non-Latin content and found no other instance, but that's corpus coverage, not evidence the bug is narrow).
Regression 2 — a recurring false-positive-table interaction, independent of RTL
Four more documents get spurious <table> wrapping from the same word/line-topology change interacting with spatial_table_detector.rs's fallback: PDFBOX-5002.pdf (trivial mixed-font-size English test page), 2503.09472_pg15.pdf (dense equation block), the two tracemonkey documents (CFG-diagram node labels, with actual content loss — i5/i6 labels vanish), and issue6127.pdf (French Cerfa form, content_ratio collapses to ~0.20 in md). Plus one case in the opposite direction I verified directly: headers_footers_78643402...page_1.pdf — a real, correctly-detected 2-column bid-pricing table on main gets dissolved into flat unstructured text here (content preserved, structure lost).
This matches a pattern across the whole #814/#817 review cycle too: the table-detection fallback is extremely sensitive to any upstream change in span/word geometry and keeps surfacing new false positives whenever unrelated text-assembly code changes. Worth a dedicated hardening pass at some point rather than one-off patches per newly-discovered trigger — not blocking this specific PR, just flagging the pattern.
Numbers
16 regressions total across the 419-doc sweep (1 RTL-critical + 4 more table false-positives/dissolutions in the pattern above + others noted in detail), against a much larger set of genuine improvements. Full per-document breakdown available if useful — happy to share the complete diff list.
Recommendation: don't merge as-is. The core idea is right and most of the corpus improves, but the RTL break is a real, structural correctness bug (not cosmetic), and I'd want the gap-floor threshold to account for text direction (e.g. reading the span's already-tracked writing-direction/RTL flag rather than a bare geometric magnitude) before this lands. Happy to help narrow down repro geometry further if useful, similar to what we did for #814.
|
Correction to my review above: the I re-verified by hand-timing the document against a fresh |
|
@yfedoseev both review findings are addressed. Marking ready for review. Here is what changed and how it was verified. Fixes1. RTL break (critical). Both new decision sites now gate on text direction before treating a leftward jump as backtracking. Arabic is cursive: one word's glyph clusters are routinely drawn as several runs in right-to-left order, so a later run's origin sits left of the previous run's end (real pages show gaps of -8 to -159 pt between consecutive runs). That is intra-word geometry, not backtracking.
Guarded by 2. Table-fallback interaction. The changed word topology let the text fallback promote a second, completely empty table on tracemonkey (header cells only, every body cell blank). New How this was testedSame 577-document differential sweep as before, main vs this branch, both synced to latest main. Harness and full per-document manifest with URLs: https://gist.github.com/tobocop2/9cdecc49b4ef47899eb1562191166992 (now includes the multilingual and RTL rows added for this round).
The word deltas on the changed documents are the intended de-fusions ( |
yfedoseev
left a comment
There was a problem hiding this comment.
Re-ran everything against f649f496. Good progress — this round genuinely fixes the more severe of the two issues.
RTL regression: confirmed fixed
wiki-cat-ar/source.pdf is now byte-identical to main across text/md/html (was jaccard 0.029 before, with -8 to -159pt negative gaps misclassified as backtracking). The looks_rtl() gate on both the fraction-glue and word-merge sites is exactly the right fix, and the new rtl_split_word_still_merges_across_negative_gap test covers it directly. Nice bonus: tracemonkey_annotation_on_page_8.pdf's diagram-label regression is also gone (remaining diff is cosmetic label splitting only, content_ratio=1.0000).
Table-fallback claim: only partially true
Table::is_effectively_empty() correctly drops fully-blank grids (confirmed via real_tracemonkey_has_no_empty_tables), but the regressions I originally flagged were mostly garbled-but-non-empty fake tables, which this guard doesn't touch. Re-checked directly against f649f496, all still reproduce unchanged:
pdfbox/.../input/PDFBOX-5002.pdf(jaccard 0.118) — same as round 1, "some text with" / "Title in a big font" still fuses into a garbled<table>with mixed-up cells.headers_footers/78643402...page_1.pdf(jaccard 0.808) — same as round 1, main's genuine bid-pricing table still dissolves into flat<p>$0.00 $0.00</p>paragraphs.arxiv_math/2503.09472_pg15.pdf(jaccard 0.879) — same as round 1, dense subscript equation block still gets a spurious<table>wrapped around part of it.- New since round 1, not previously flagged:
PMC8129076/source.pdf(jaccard 0.993, small but real) — a prose paragraph about circadian biology gets scattered diagram labels (SIRT1/AMPK/PI3K/PTEN) spliced out into a fake table, splitting the paragraph. Same bug class as the above three — worth a look at whether it's newly surfaced by this diff or was just missed in round 1.
Full 419-corpus sweep vs main: 133 byte-diffs, unit tests 5720/5720 pass.
Recommendation
The RTL fix is solid and ready on its own. The table-fallback side is the harder problem — spatial_table_detector.rs's fallback path is proving to be a recurring theme across this whole review cycle (same fragility surfaced independently in #814's review too), and an empty-check alone doesn't cover the "garbled but non-empty" failure mode. Two paths that would work: (a) broaden the guard to also drop grids that don't look like real tabular data (there's already prose-shape heuristics elsewhere in this file to borrow from), or (b) split this PR down to just the RTL fix, which is genuinely done, and move the table-fallback work to a follow-up.
One thing worth naming since you're the same author who just went through five rounds on #814: I'd like this one to close out faster — targeting at most 4 rounds here, at least one fewer than #814 took. The RTL fix landing clean on the first re-check is a good sign that's realistic.
…nator Displayed fractions glue the relation sign to the denominator during text composition: `dx1/dt = ...` extracts as `dx1 =dt`. The `=` sits at the fraction's mid-height and the denominator `dt` is drawn afterwards, starting behind the `=` origin at a small baseline offset, so the line emitter sees a same-line pair whose next span backtracks and concatenates them. Add the missing emitter branch in assemble_text_from_spans: a backtracking span with a real baseline offset (y_diff > 1), an overlap beyond kerning (gap < -1em) that returns to at-or-left-of the previous origin (delta_x <= 0.5) breaks the line instead of fusing. The branch is gated off for right-to-left runs, whose leftward flow is ordinary reading order rather than a backtrack, so Arabic and other RTL scripts are untouched. The fix lives entirely in the composed-text path (extract_text / to_markdown / to_html) and does not alter word or span geometry, so table detection is byte-identical to main. Verified across a 1431-page per-page differential against main (arXiv math, govdocs, PMC, multilingual, forms, plus the Apache PDFBox big-then-small-font repro, a US GPO bid-pricing schedule, and Arabic): zero table-structure changes, and every text change is character-conserving. Covered by a synthetic fixture that fails on main (emits `=dt`) and opt-in real-document guards for the arXiv fraction page and Arabic RTL. Signed-off-by: Tobias Perelstein <5562156+tobocop2@users.noreply.github.com>
f649f49 to
2325493
Compare
|
@yfedoseev thanks for the precise repro cases. They pointed straight at the root cause, and I have narrowed this PR to close it out cleanly. It is ready for review again. Root cause of the table regressions
In other words the word-level fix and the fallback detector are entangled: you cannot change word topology without hardening the detector at the same time. That is the harder problem you named, and it does not belong in this fix. What changedThis PR is now only the composed-text emitter branch, and nothing else. It lives entirely in Your four cases on this branch versus main:
The spurious and garbled tables are gone because the code that caused them is no longer in this PR. The bid-pricing table is preserved. The RTL fix you already confirmed is retained (the emitter branch stays gated off for right-to-left runs). How this was validatedYou were right that whole-document jaccard was too coarse. The new harness splits every PDF into single pages and fingerprints table STRUCTURE per page (the multiset of cell texts), so it flags a table that keeps its count but scrambles its cells, and a table that appears or vanishes on one page:
Run across 1431 pages (arXiv math, govdocs1, PMC, multilingual/RTL, IRS forms, PoC‖GTFO, plus your named cases and GPO bid-pricing):
Every text change is character-conserving; the drift is Follow-upThe word-level de-fusion ( |
yfedoseev
left a comment
There was a problem hiding this comment.
Re-ran everything against 23254939. This is exactly the right move — appreciate you tracing the entanglement instead of continuing to patch the guard.
Root cause check: confirmed
You were right that extract_page_tables builds its detector input from extract_words() output — the round-1/round-2 word-level de-fusion was itself perturbing table geometry, independent of whatever RTL gating sat on top of it. Scoping this PR down to just the assemble_text_from_spans emitter branch (which never touches word/span geometry) is the correct fix, not a workaround.
Independent verification
- Unit tests: 5714 lib + 3/3 in
test_display_math_word_fusion.rs, including the newrtl_backtracking_runs_are_not_broken_by_the_emitterguard against the realwiki-cat-arfixture. - Full 419-corpus sweep vs
main: 0/419 md diffs, 0/419 html diffs — full byte-identity on every table-detection-relevant path. Onlytextformat shows diffs (99), all from the emitter branch itself. - Re-checked all 4 documents from round 2 directly:
PDFBOX-5002.pdf,2503.09472_pg15.pdf,PMC8129076/source.pdf,headers_footers_78643402...page_1.pdf— every one is byte-identical tomainin html/md now. Zero diffs. wiki-cat-ar/source.pdf— byte-identical tomain. RTL fix holds.- Every remaining text diff shows
content_ratio=1.0000and there isn't a single entry withdword <= -3anywhere in the corpus — no content loss. Read the worst-severity one by hand (2503.09472_pg15.pdf, jaccard 0.872): it's exactly the intended fix,dx 3 =dtsplitting intodx/3 =/dtacross lines, nothing dropped.
Your 1431-page structural-fingerprint harness claim (0 spurious/dissolved/garbled tables, 0 chars lost) checks out against independent verification on the 419-doc corpus too.
Approving. Good call deferring the word-level de-fusion + detector hardening to a follow-up — that's the harder problem and it deserves its own review cycle rather than riding along here.
Description
Displayed fractions glue the relation sign to the denominator during text composition:
dx1/dt = ...extracts asdx1 =dt. The=sits at the fraction's mid-height and the denominatordtis drawn afterwards, starting behind the=origin at a small baseline offset, so the line emitter sees a same-line pair whose next span backtracks and concatenates them.The fix adds the missing branch to the
extract_textline emitter (assemble_text_from_spans): a backtracking span with a real baseline offset (y_diff > 1), an overlap beyond kerning (gap < -1em) that returns to at-or-left-of the previous origin (delta_x <= 0.5) breaks the line instead of fusing. The branch is gated off for right-to-left runs, whose leftward flow is ordinary reading order rather than a backtrack.The change lives entirely in the composed-text path (
extract_text/to_markdown/to_html) and does not alter word or span geometry, so table detection is byte-identical to main.Type of Change
Related Issues
Fixes #830
Changes Made
extract_textline emitter insrc/document.rs, gated off for RTL runs.tests/test_display_math_word_fusion.rs: a synthetic fixture that fails onmain(emits=dt) and passes with the branch (mutation-verified), plus opt-in real-document guards for the arXiv fraction-dense page and Arabic RTL.Testing
Verified across a 1431-page per-page differential against
main(arXiv math, govdocs1, PMC, multilingual/RTL, IRS forms, PoC‖GTFO, plus the Apache PDFBox big-then-small-font repro, a US GPO bid-pricing schedule, and Arabic): zero table-structure changes (no spurious, dissolved, or garbled tables; every table byte-identical to main), and every text change character-conserving.make checkis green (fmt, clippy-D warnings, 5994 lib tests).The scope was narrowed from an earlier revision after review: fixing the fusion at the
extract_wordslayer changed word geometry that the spatial table detector consumes (extract_page_tablesbuilds its input fromextract_words), which fabricated tables on prose pages. The word-level de-fusion plus the detector hardening it requires are a separate follow-up so this fix stays table-neutral.Harness and full document set with per-document URLs: https://gist.github.com/tobocop2/9cdecc49b4ef47899eb1562191166992