Repository navigation
fix: use each chunk's own position for its wider_text window - #276
MohammadHijjawi97 wants to merge 3 commits into
Conversation
_sentence_window_split located every split chunk with original_text.index(doc_text), which always returns the first occurrence. When a chunk's text also appears earlier in the document (e.g. a short trailing chunk or a repeated paragraph), its wider_text was taken from the earlier location, so the agents that use text_window_splitter passed unrelated context to the LLM. Search from the previous chunk's position instead, since split docs are in document order. Also make the right-hand window include `offset` characters like the left-hand one; it was one character short. Signed-off-by: Mohammad Hijjawi <mohammad.hijjawi1997@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: MohammadHijjawi97 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
| start_index = original_text.find(doc_text, search_start) | ||
| if start_index == -1: | ||
| start_index = original_text.index(doc_text) | ||
| search_start = start_index + 1 |
There was a problem hiding this comment.
deepsearcher/loader/splitter.py line:74
Medium ---- The search_start = start_index + 1 heuristic still mislocates a chunk whose exact text also occurs inside the previously matched span. Example: original_text = "XABCABCY" with chunks ["XABC", "ABCY"] — after matching XABC at 0, find("ABCY", 1) returns 1 (inside XABC) instead of the chunk's real position 4, so its wider_text is still cut from the wrong location. This is the same repeated-text class the PR targets. Consider deriving the next search start from the previous match's end (accounting for chunk overlap) or validating the monotonic position, and add a regression test for a chunk whose text is a substring of the previous chunk.
There was a problem hiding this comment.
Addressed in 821f980. A small correction to the example: "XABCABCY".find("ABCY", 1) returns 4, not 1.
The problem is real, though, whenever a chunk's text also occurs inside the previous chunk. For example, with "XABCABC" and chunks ["XABC", "ABC"], the old search found "ABC" at 1 instead of 4. The splitter produces this too: "ab ab ab ab ab" with chunk_size=5 and chunk_overlap=0 was placed at 0, 3, 6, 9 instead of 0, 6, 9, 12.
The fix:
_sentence_window_splitnow takeschunk_overlap, whichsplit_docs_to_chunkspasses in.- The next search starts at
max(previous_start + 1, previous_end - chunk_overlap), the same bound langchain uses forstart_index. - If nothing is found there, it falls back to the looser searches.
Regression tests cover the "XABCABC" case, and compare every wider_text against the splitter's own add_start_index positions with overlap 0 and 1.
| # locate repeated text at its own position instead of its first occurrence. | ||
| start_index = original_text.find(doc_text, search_start) | ||
| if start_index == -1: | ||
| start_index = original_text.index(doc_text) |
There was a problem hiding this comment.
deepsearcher/loader/splitter.py line:73
Low ---- If find returns -1 and the chunk text is not present anywhere in original_text, this fallback still raises ValueError, which propagates through split_docs_to_chunks and aborts the whole document load. The failure mode is pre-existing, but the fallback path is now newly reachable when a chunk's text only occurs before the previous match (e.g., after splitter whitespace normalization). Consider handling the missing-text case explicitly or documenting the substring assumption.
There was a problem hiding this comment.
Addressed in d31ccba. The fallback now uses find() from the start of the text instead of index(). If the chunk text is not in the original text at all, wider_text falls back to the chunk text itself and the search position is left unchanged, so the rest of the document still loads. Before this PR index() raised ValueError in this case too; it now degrades to no extra context. test_sentence_window_split_text_not_in_original covers it.
If a split chunk's text cannot be found anywhere in the original document (for example after splitter normalization), use the chunk text itself as wider_text instead of raising ValueError and aborting the document load. Signed-off-by: Mohammad Hijjawi <mohammad.hijjawi1997@gmail.com>
Starting the search one character after the previous match could place a chunk inside the previous chunk when its text also occurs there. Consecutive split pieces overlap by at most chunk_overlap characters, so start the search at max(previous start + 1, previous end - chunk_overlap), and pass chunk_overlap from split_docs_to_chunks. Signed-off-by: Mohammad Hijjawi <mohammad.hijjawi1997@gmail.com>
This branch has not been deployed
3 participants
_sentence_window_splitlocated each split chunk withoriginal_text.index(doc_text), which always returns the first occurrence. When a chunk's text also appears earlier in the document (e.g. a short trailing chunk, or a repeated paragraph), itswider_textwas cut from the earlier location.NaiveRAG,DeepSearchandChainOfRAGsendwider_textto the LLM whentext_window_splitteris enabled, so the answer was built from unrelated context.Example with offset=3, original text
"x" + "."*20 + "y" + "-"*20 + "x", and chunksx,y,x:wider_textis"x..", the window of the firstx"---x"Changes:
offsetcharacters, like the left-hand one. It was one character short, contrary to the docstring.Tests:
test_sentence_window_split_repeated_textintests/loader/test_splitter.py; it fails on master and passes with this change.pytest tests/loader/test_splitter.py: 4 passedruff format --diffandruff check: clean