Skip to content

fix(pdf): preserve a line-final hyphen that does not split a word (#4043) - #4052

Merged
dolfim-ibm merged 2 commits into
docling-project:mainfrom
VSathveek:fix/detached-hyphen-line-join
Aug 25, 2026
Merged

dolfim-ibm merged 2 commits into
docling-project:mainfrom
VSathveek:fix/detached-hyphen-line-join

Conversation

@VSathveek

@VSathveek VSathveek commented Aug 23, 2026 •

Copy link
Copy Markdown
Contributor

Issue resolved by this Pull Request:
Resolves #4043

Problem

PageAssembleModel.sanitize_text() drops a line-final hyphen before joining two
lines, assuming it is a hyphen that splits a word across the break. The guard
deciding this inspects the last word token anywhere in the line rather than
the hyphen itself:

prev_words = re.findall(r"\b[\w]+\b", prev_line)
...
if len(prev_words) and len(line_words) and prev_words[-1].isalnum() and line_words[0].isalnum():
    lines[ix] = prev_line[:-1]   # drop the hyphen

\b[\w]+\b skips over whitespace, so for a line ending ... -pn 1 - the last token
is "1", the guard passes, and the hyphen is deleted even though it was never
attached to a word. The wrap reported in the issue is therefore extracted as
... -pn 1 prio 3 ..., the flag silently losing its dash.

The deletion is not specific to CLI syntax. Any line-final hyphen that follows
whitespace is affected, and the existing test corpus turns out to contain four
such documents where a real character is being discarded today.

Fix

Only a hyphen attached to the preceding word marks a split word, so the fix
requires the character immediately before it to be alphanumeric:

hyphen_attached_to_word = len(prev_line) > 1 and prev_line[-2].isalnum()

That condition only ever narrows the existing one, so nothing de-hyphenated
today stops being de-hyphenated: "algo-" + "rithms" → "algorithms" is
unchanged, as is the existing exclusion of underscore-bearing tokens
("foo_bar-" + "baz" stays "foo_bar-baz").

A detached hyphen is then treated as what it is — an ordinary character that
happens to sit at a line break — so it is kept and the lines are joined with a
space, exactly as the non-hyphen branch already does.

Reference data

Four documents change. Three recover a character that is being discarded today:

2203.01017v2  Computer Vision ECCV 2020    ->  Computer Vision - ECCV 2020
2206.01062    complex layouts rdcl2017     ->  complex layouts - rdcl2017
2305.03393v1  - -"C" cell a new table cell ->  - "C" cell - a new table cell

redp5110_sampled improves structurally. Four bullet items whose - marker sat
alone on its own cell were glued to the following word, which pushed them out of
the list and into numbered entries; they are now recognised as list items:

-2. -Human Resources can see the unmasked TAX\_ID of the employees.
+- Human Resources can see the unmasked TAX\_ID of the employees.

One line moves sideways rather than forwards, and I would rather point at it than
let it pass unnoticed. In 2305.03393v1 the author ORCIDs arrive as separate
cells ("[0000", "-", "0002", ...), so the bare hyphens now take a space:

-[0000 -0002 -3723 -6960]
+[0000 - 0002 - 3723 - 6960]

Neither form is the real [0000-0002-3723-6960]; the identifier is already
broken upstream by the per-cell split and the space this branch adds between
cells. No character is gained or lost, so I left it rather than special-casing
digits, but it is the one diff here that is not an improvement.

I ran the suite against unmodified main first to confirm this environment
reproduces the committed reference data exactly, so these diffs are attributable
to the change and not to local drift. Flagging them for the double review the
contributing guide asks for.

A limitation worth stating

Whether a detached hyphen should close up against the next token (-prio) or
keep its space (- ECCV) is not decidable at this layer: the caller .strip()s
each cell before sanitize_text sees it, so the trailing-space evidence is
already gone. All four corpus cases are separator dashes wanting the space, so
that is the default here, and the issue's flag case comes out as -pn 1 - prio 3
— the dash preserved but not closed up. That fixes the data loss the issue is
about without inventing a distinction the input no longer carries. Recovering it
properly means keeping the pre-strip whitespace, which is a larger change to the
caller and I would rather not fold it in here.

Tests

TestSanitizeTextHyphenation in tests/test_page_assemble_model.py covers both
directions: an attached hyphen still joins a split word, a non-alphanumeric
token is still left alone, and detached hyphens (wrapped flag, separator dash,
bullet marker, negative number) are preserved. The detached cases fail on main.

One related spot, left alone deliberately

ReadingOrderModel._merge_elements has a parallel end-of-line hyphen check for
merging elements across cluster boundaries and drops a detached hyphen the same
way. I have not touched it here, since the module has other work in flight
(#3891, #3955). Happy to follow up separately, or to fold it in if you prefer.

Checklist:

  • Documentation has been updated, if necessary.
  • Examples have been added, if necessary.
  • Tests have been added, if necessary.

…cling-project#4043)

Signed-off-by: Varanasi Sathveek <sathveekvaranasi@gmail.com>
@github-actions

Copy link
Copy Markdown
Contributor

✅ DCO Check Passed

Thanks @VSathveek, all your commits are properly signed off. 🎉

@mergify

mergify Bot commented Aug 23, 2026 •

Copy link
Copy Markdown
Contributor

Merge Protections

🟢 All 2 merge protections satisfied — ready to merge.

Show 2 satisfied protections

🟢 Enforce conventional commit

Make sure that we follow https://www.conventionalcommits.org/en/v1.0.0/

  • title ~= ^(fix|feat|docs|style|refactor|perf|test|build|ci|chore|revert)(?:\(.+\))?(!)?:

🟢 Require two reviewer for test updates

When test data is updated, we require two reviewers

  • #approved-reviews-by >= 2

@codecov

codecov Bot commented Aug 24, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

ceberam
ceberam previously approved these changes Aug 24, 2026

@ceberam ceberam left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @VSathveek for suggesting this fix.
I added a minor non-blocking comment.

Comment thread docling/models/stages/page_assemble/page_assemble_model.py Outdated
Signed-off-by: Varanasi Sathveek <sathveekvaranasi@gmail.com>
@dolfim-ibm
dolfim-ibm merged commit 49579ee into docling-project:main Aug 25, 2026
26 checks passed
@gershomnkanta5-a11y

Copy link
Copy Markdown

It's perfect

@milosdzajic

Copy link
Copy Markdown

Confirmed on 2.123.1 — the silent data loss from the original report is resolved, thank you. Residual behavior matches exactly what you flagged in the PR description: all three of my original repro cases now come out as - flagname (detached, space preserved) rather than -flagname (closed up).

Not blocking for me — this is now deterministically catchable/correctable downstream, which is a big improvement over silent corruption. But since you called out the pre-strip whitespace preservation as a larger change you didn't want to fold into this PR, I'd be happy to take a pass at it if that's still open. I don't have full context yet on where the .strip() call happens in the caller before sanitize_text sees each cell — if you can point me at the right spot, I can look into preserving that whitespace evidence so the join logic can tell "close up" (-prio) from "keep the space" (- ECCV) apart properly, rather than defaulting to one behavior for both.

Happy to open a draft PR to get early feedback once I've got a shape for it, rather than surprising you with something fully baked.

@VSathveek

Copy link
Copy Markdown
Contributor Author

Thanks for checking it against 2.123.1, and good to hear the residual shape is what you expected rather than a surprise.

The follow-up is still open — please go ahead, and a draft PR to agree the shape first sounds like exactly the right way to do it.

sanitize_text has only one caller, and the whitespace evidence is dropped in the comprehension that builds its input, page_assemble_model.py L200-205 on main:

textlines = [
    cell.text.replace("\x02", "-").strip()   # L201 - the trailing space dies here
    for cell in cluster.cells
    if len(cell.text.strip()) > 0
]
text = self.sanitize_text(textlines)

Three things that cost me time in that area and may save you some:

  • The .strip() on L201 is per cell, not per visual line, and a cell can be a lone "-". In 2305.03393v1 the author ORCIDs arrive as "[0000", "-", "0002", … which is why that document's reference output shifted in my PR. Whatever signal you thread through has to survive a cell whose entire content is the hyphen.
  • The same line does replace("\x02", "-"), so downstream can no longer distinguish a real hyphen-minus from whatever \x02 encoded. Worth deciding whether those two should stay merged once you have richer information available.
  • There is a second .strip() on the return at L178. Harmless for this, but easy to trip over.

Reference data is the other thing to plan for: four documents moved in my PR, and anything that changes this join will move them again. Running the suite against unmodified main first, to confirm your environment reproduces the committed ground truth, makes the regenerated diff trustworthy — without that baseline it is hard to separate your change from local drift. Note that tests/data changes need two reviewers.

Tag me on the draft and I will review; I know this corner reasonably well now.

One thing so we do not collide: I am going to look at ReadingOrderModel._merge_elements (readingorder_model.py L672-684), which has the same "detached hyphen treated as a word split" defect for merges across cluster boundaries. Separate code path, no overlap with the cell-whitespace work.

@VSathveek

Copy link
Copy Markdown
Contributor Author

Follow-up is up: #4180 applies the same attached-hyphen check to ReadingOrderModel._merge_elements (the cross-element merge path), with two detached-hyphen regression cases. The pre-strip whitespace change for the cell-level join is still open if you'd like to take it — happy to review a draft.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Hyphen-minus stripped when a CLI flag (-flag) wraps across a line break during PDF text extraction

6 participants