fix: merge a short trailing split in EmbeddingBasedDocumentSplitter - #12438
Open
ShousenZHANG wants to merge 2 commits into
Open
fix: merge a short trailing split in EmbeddingBasedDocumentSplitter#12438ShousenZHANG wants to merge 2 commits into
ShousenZHANG wants to merge 2 commits into
Conversation
`_merge_small_splits` only merges forward: it folds the next split into a running accumulator while that accumulator is below `min_length`. Whatever is left in the accumulator when the loop ends is appended unconditionally, so a final split shorter than `min_length` is emitted as its own document, breaking the documented `min_length` promise. Merge that trailing split into its predecessor instead, subject to the same `max_length` limit that already governs forward merges, so a blocked merge keeps behaving as it does today. Fixes deepset-ai#12436
ShousenZHANG
requested review from
sjrl
and
a lite review from Copilot
and removed request for
a team
August 23, 2026 01:24
|
@ShousenZHANG is attempting to deploy a commit to the deepset Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
There was a problem hiding this comment.
Pull request overview
- Fixes
EmbeddingBasedDocumentSplitter._merge_small_splitsso a final split shorter thanmin_lengthis merged into the preceding split when the combined length remains< max_length, aligning behavior with the documented “small splits will be merged” promise (best-effort whenmax_lengthblocks). - Adds targeted unit tests covering the trailing-short-split regression, the
max_lengthguard behavior for backward merges, and the single-split edge case. - Adds a release note describing the user-visible behavior change.
Changes:
- Update
_merge_small_splitsto perform a backward merge for a short trailing split (subject to the existing< max_lengthconstraint). - Add three unit tests to lock in the new behavior and edge cases.
- Add a release note entry for the fix.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
haystack/components/preprocessors/embedding_based_document_splitter.py |
Adds backward-merge logic for a short trailing split while preserving the existing < max_length merge guard. |
test/components/preprocessors/test_embedding_based_document_splitter.py |
Adds regression + edge-case coverage for trailing split merging and max_length blocking. |
releasenotes/notes/merge-short-trailing-split-8f5b640fdbc3ccbe.yaml |
Documents the fix and its interaction with max_length. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
AGENTS.md specifies reStructuredText for release notes, so inline code takes double backticks. Also drops the em dash so the file stays ASCII, matching every other note in releasenotes/notes.
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
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.
Related Issues
Proposed Changes:
_merge_small_splitsonly merges forward: it keeps a running accumulator and folds the next split into it while the accumulator is belowmin_length. Whatever is left in that accumulator when the loop ends is appended unconditionally, so a final split shorter thanmin_lengthis emitted as its own document — silently breaking the documented promise that "splits below this length will be merged". This is common in practice: a closing line, a signature, a table footer.The trailing split is now merged into its predecessor instead. The merge is guarded by the same
max_lengthlimit that already governs forward merges, so the existing tradeoff is preserved: when a merge would reachmax_lengththe short split is left alone, exactly astest_merge_small_splits_respect_max_lengthalready documents for a blocked forward merge. Because the guard is strictly< max_length, the merged result can never become something_split_large_splitshas to re-split.Both
_split_documentand_split_document_asyncgo through this helper, so both paths are covered.How did you test it?
Three unit tests in
test_embedding_based_document_splitter.py:test_merge_small_splits_merges_short_trailing_split— the regression. Fails onmain(returns["Long enough text ", "Ok."]), passes with this change.test_merge_small_splits_keeps_short_trailing_split_when_max_length_blocks— the backward merge must not overridemax_length.test_merge_small_splits_keeps_a_lone_short_split— a single split has nothing to merge into and must be returned as is.Locally:
hatch run test:unit test/components/preprocessors/→ 361 passed;hatch run test:types→ no issues in 447 source files;hatch run fmt-checkon both changed files → clean.Notes for the reviewer
The behaviour when a merge is blocked by
max_lengthis deliberately unchanged —min_lengthstays best-effort in that case, matching the existing forward-merge behaviour rather than introducing a second rule.One overlap to flag: #12434 also touches this file, but in
_find_split_points(~line 376) and its tests, roughly 50 lines away from_merge_small_splits. I don't expect a conflict; happy to rebase if that one lands first.This PR was fully generated with an AI assistant. I have reviewed the changes and run the relevant tests.
Checklist