fix: apply overlap only once in RecursiveDocumentSplitter - #12284
fix: apply overlap only once in RecursiveDocumentSplitter#12284Diwak4r wants to merge 5 commits into
Conversation
|
@Diwak4r is attempting to deploy a commit to the deepset Team on Vercel. A member of the Team first needs to authorize it. |
|
|
|
Hi @Diwak4r, thanks a lot for your contribution! 🙏 We noticed that the Contributor License Agreement (CLA) check ( To get your PR reviewed, please sign the CLA via the link in the |
HaystackBot
added
the
cla-pending
|
@Diwak4r Please sign the CLA for me to be able to review and merge this PR. Thanks! |
ebarkhordar
left a comment
ebarkhordar
left a comment
There was a problem hiding this comment.
Ran the invariant this fixes across randomized inputs in a clean python:3.11-slim container (pip install -e .), at 1ff39f52 and at 842519e5 on main. 120 random texts assembled from \n\n, \n and space separators, sweeping split_length and split_overlap, asserting that every emitted chunk is a substring of the source document:
split_unit configs main this PR
char 720 263 0
word 720 44 0
token 480 97 0
test/components/preprocessors/test_recursive_splitter.py is 56 passed at 1ff39f52.
Two notes.
The three lines this removes from the fallback path in _chunk_text are the fix from #11768 (7ded47fc2). Its regression tests test_fallback_overlap_char_unit, _word_unit and _token_unit are inside those 56 and still pass from the new call site in _run_one, so the path is covered. It is the first thing to re-check if this placement is ever revisited.
The new regression test pins one hand-built example (exact chunk strings, len(chunks) == 6) on split_unit="char" only, so it will also fail on a future boundary change that has nothing to do with this bug. The property it is really pinning is the one in the table above, and it holds for all three units, so a loop over them asserting containment would catch the same class without freezing the boundaries. Happy to post the generator I used if that is useful.
For word units the overlap is rejoined with a single space in _create_chunk_starting_with_overlap, so the plain substring check does not hold there and I asserted instead that the chunk's word sequence is contiguous in the source. I did not check split_unit="sentence".
|
Hi @Diwak4r, just a friendly reminder: this PR is still in draft because the Contributor License Agreement (CLA) hasn't been signed yet. We'd love to review your contribution! Please sign the CLA via the link in the |
|
Hi @Diwak4r, this PR is still waiting for the Contributor License Agreement (CLA) to be signed. Please note that if the CLA isn't signed within the next 4 days, this PR will be automatically closed as stale. Signing only takes a minute via the link in the |
Per review feedback: the containment property holds for char, word and token units, so loop over all three instead of pinning only the char example. Word units are checked via word-sequence contiguity since the overlap is rejoined with a single space there.
|
CLA is signed now (license/cla is green), and the PR is marked ready for review - @bogdankostic it's all yours. Also acted on @ebarkhordar's review note: the regression test now loops over all three split units asserting the containment invariant (word units via word-sequence contiguity, since the overlap is rejoined with a single space there) instead of pinning only the hand-built char example. The exact-boundary assertions stay for the char case as a concrete anchor. Full file: 56 passed locally; new head a8b3938. |
HaystackBot
removed
the
cla-pending
|
Thanks for signing the CLA, @Diwak4r! 🎉 This PR is now ready for review again and the reviewer has been re-assigned. |
Related Issues
Proposed Changes:
RecursiveDocumentSplitter._chunk_textchunks recursively: a split that still exceedssplit_lengthis re-chunked with the next separator. Whensplit_overlap > 0, the overlap was applied at every recursion level, so chunks produced by an inner recursion got the overlap prepended a second time by the enclosing level. The result is chunks that are not substrings of the source text (e.g."Overview\nOverview\nOverview\nThis module handles ing").This PR moves the overlap application out of
_chunk_textand applies it exactly once, on the fully chunked list, in_run_one(after the trailing-empty-chunk cleanup). Recursion boundaries are unchanged.How did you test it?
Added a regression test (
test_run_multiple_separators_with_overlap_applies_overlap_only_once) that chunks a multi-separator text with overlap and asserts every chunk is a substring of the source text. The test fails on the previous code (garbage chunks like"Overview\nOverview\nOverview\n..."are produced) and passes with this fix.Ran the full
test_recursive_splitter.pyandtest_type_utils.pysuites: 1122 tests pass, including all existing overlap tests (test_apply_overlap_*,test_run_split_document_with_overlap_character_unit,test_run_split_document_with_overlap_and_fallback_character_unit,test_run_custom_sentence_tokenizer_document_and_overlap_char_unit, and the word/token overlap +_split_overlapmetadata tests).Notes for the reviewer
The bug only manifests when multiple separators cause actual recursion (single-separator configs are unaffected, which is why the existing tests did not catch it).
_apply_overlapis a single-pass transform over a flat chunk list, so applying it once to the final list is equivalent to the previous behavior for the non-recursive cases.Checklist
fix:,feat:,build:,chore:,ci:,docs:,style:,refactor:,perf:,test:and added!in case the PR includes breaking changes.