Skip to content

fix(chunking): reject a negative overlap instead of corrupting text - #4501

Open
chrikrah wants to merge 2 commits into
Unstructured-IO:mainfrom
chrikrah:chrikrah/reject-negative-overlap
Open

chrikrah wants to merge 2 commits into
Unstructured-IO:mainfrom
chrikrah:chrikrah/reject-negative-overlap

Conversation

@chrikrah

@chrikrah chrikrah commented Sep 26, 2026 •

Copy link
Copy Markdown

No open issue covers this. None. Found while reading the three sibling validators in _validate() against the parameters they guard.

ChunkingOptions._validate() at unstructured/chunking/base.py rejects a negative new_after_n_chars and a negative new_after_n_tokens. It never checks overlap. A negative overlap then corrupts output two ways, with no error and no warning.

base.py:2731 computes the remainder as s[maxlen - self._opts.overlap :]. With overlap = -5 that remainder starts five characters past the end of the fragment, so five characters of the document disappear at every split boundary.

base.py:755 computes self._text[-overlap:]. With overlap = -5 that is a forward slice, so the tail carried into the next chunk is most of the current chunk.

Before, on a 60-character input of unique characters at max_characters=20:

$ python repro.py
overlap=   0: ['ABCDEFGHIJKLMNOPQRST', 'UVWXYZabcdefghijklmn', 'opqrstuvwxyz01234567']
           dropped from output entirely: ''
overlap=  -5: ['ABCDEFGHIJKLMNOPQRST', 'Zabcdefghijklmnopqrs', 'yz01234567']
           dropped from output entirely: 'UVWXYtuvwx'

The overlap=0 line is the control: same input, same commit, nothing lost.

With overlap_all=True, three 10-character elements at max_characters=12:

overlap=  0 -> ['AAAAAAAAAA', 'BBBBBBBBBB', 'CCCCCCCCCC']
overlap= -4 -> ['AAAAAAAAAA', 'AAAAAA\n\nBBBB', 'BB', 'AA\n\nBBBBBBBB', 'CCCCCCCCCC']

Text duplicated and out of order.

Reachable from all three public entry points: chunk_elements(...), chunk_by_title(...), and partition(filename=..., chunking_strategy="basic", overlap=-5). All three drop the same characters.

After, the four added lines mirror the existing new_after_n_chars check and raise ValueError.

$ python -m pytest test_unstructured/chunking -q
484 passed, 24 skipped in 15.04s

Baseline on 1bedf7b is 482 passed, 24 skipped, so the delta is the two new parametrised cases. Reverting unstructured/chunking/base.py and keeping the test gives 2 failed, 482 passed, 24 skipped, both of them Failed: DID NOT RAISE ValueError. ruff check is clean on both touched files.

I did not run make test or make check in full, and no PDF, image or OCR test ran here: the install was -e ".[csv,docx,md,pptx,xlsx]" without the inference extras. make check-version passes, with CHANGELOG.md and __version__.py both at 0.27.10.

Duplicate check: all 130 open pull requests enumerated, 10 touching unstructured/chunking/. I grepped each diff for added lines matching _validate|overlap.*must be|overlap_arg. The only hit is #4487, which caps combine_text_under_n_chars in title.py::_validate: different file, different function, different parameter. #4392 is the nearest neighbour and changes _split_by_tokens rather than validation, so the two do not collide.

@cragwolfe you merged the last chunking change here. Would you rather this raised, or clamped a negative overlap to zero with a warning?

Review in cubic

ChunkingOptions._validate() checked new_after_n_chars and new_after_n_tokens
for negative values but never checked overlap. A negative overlap then
corrupted output two ways with no error and no warning.

On the character-split path, base.py:2731 computes the remainder as
s[maxlen - overlap:], so a negative overlap starts the remainder past the end
of the fragment and drops that many characters at every split boundary. A
60-character input at max_characters=20 and overlap=-5 loses 'UVWXY' and
'tuvwx' from the output entirely.

With overlap_all=True, base.py:755 computes self._text[-overlap:], which for a
negative overlap is a forward slice, so the tail carried into the next chunk
repeats most of the current one.

Reachable from chunk_elements, chunk_by_title and partition(chunking_strategy=).
Now raises ValueError, matching the two sibling options.
@cragwolfe

Copy link
Copy Markdown
Contributor

@chrikrah , lets keep the ValueError , since overlap is intended to be non-negative. thanks!

@chrikrah chrikrah left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@cragwolfe, make check fails on this branch today, before any merge. #4472 took the 0.27.17 heading on main the next afternoon. The resolution moves CHANGELOG.md and unstructured/__version__.py together, onto whichever number is free.

$ git archive refs/pull/4501/head | tar -x -C /tmp/t && cd /tmp/t   # b180227
$ scripts/version-sync.sh -c -f unstructured/__version__.py semver   # origin/main served from 2c0c7a6
Error: there is already a commit associated with version 0.27.17.
$ sed -i '1s/0.27.17/0.27.18/' CHANGELOG.md
$ scripts/version-sync.sh -c -f unstructured/__version__.py semver
version sync would make the following changes to unstructured/__version__.py:
< __version__ = "0.27.17"  # pragma: no cover
> __version__ = "0.27.18"  # pragma: no cover
Versions are out of sync! See above for diffs.
# not run: python -m pytest test_unstructured/chunking -q, 484 passed / 24 skipped on 81f0630

You approved it on 26 September, and it will collide again on every release until it lands. Is the renumber the last thing, or does something else block it? I can push both files within the hour of your word.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants