Repository navigation
fix(chunking): record chunk offsets into the original source, not the normalized text - #44
Conversation
…e_whitespace collapses space/newline runs and strips the ends before splitting, but span offsets were recorded as-is - source[start:end] missed the chunk's real bytes and citation spans pointed at the wrong text. normalize_whitespace_with_map returns the index map back to the source; chunkers remap through it. Signed-off-by: Charan Rathore <180254320+charan-rathore@users.noreply.github.com> Added functions to normalize whitespace with source mapping and remap offsets.
…e Signed-off-by: Charan Rathore <180254320+charan-rathore@users.noreply.github.com> Updated text normalization to include mapping to original source offsets for accurate citation handling.
… Signed-off-by: Charan Rathore <180254320+charan-rathore@users.noreply.github.com> Updated text normalization to include mapping to source offsets for citation and highlighting purposes.
… source Signed-off-by: Charan Rathore <180254320+charan-rathore@users.noreply.github.com> Replaced normalize_whitespace with normalize_whitespace_with_map to retain source offsets.
…Rathore <180254320+charan-rathore@users.noreply.github.com>
…l source Signed-off-by: Charan Rathore <180254320+charan-rathore@users.noreply.github.com> Added tests to ensure offsets index the original source correctly, addressing issues with normalized whitespace affecting citation spans.
…riginal source Signed-off-by: Charan Rathore <180254320+charan-rathore@users.noreply.github.com> Added unit tests for StructureAwareChunker to verify original-source offset coverage.
… source Signed-off-by: Charan Rathore <180254320+charan-rathore@users.noreply.github.com> Add tests for semantic chunker to validate original-source offsets.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01c0f8eac3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| out.append(c) | ||
| idx_map.append(i) |
There was a problem hiding this comment.
For large documents with little collapsible whitespace, this appends a distinct Python integer for nearly every character, adding roughly tens of bytes per character on top of the original and normalized strings. A document of tens of megabytes can therefore require well over a gigabyte merely to construct the offset map, potentially terminating ingestion with an OOM. Represent normalization changes as sparse runs/boundaries or collect only the source positions needed for chunk endpoints instead.
Useful? React with 👍 / 👎.
| version_id=version_id, | ||
| base_metadata=base_metadata, | ||
| ) | ||
| remap_offsets_to_source(chunks, norm_to_source, len(text)) |
There was a problem hiding this comment.
Whenever overlap is enabled and the document produces multiple chunks, _create_chunks_with_overlap prepends text from the preceding span but leaves start_char_offset at the current span's start; this post-pass merely translates that unchanged start. For example, a second chunk whose text begins with has words\n\n. Second... can receive an offset pointing at . Second..., so slicing the source omits the overlap and citations/highlights do not cover the returned chunk text. Track the normalized source position of the selected overlap and remap that as the chunk start.
Useful? React with 👍 / 👎.
| version_id=version_id, | ||
| base_metadata=base_metadata, | ||
| ) | ||
| remap_offsets_to_source(chunks, norm_to_source, len(text)) |
There was a problem hiding this comment.
Even with overlap disabled, this remaps span positions that are already inaccurate when a separator-prefixed segment requires deeper recursive splitting. _split_on_separator filters whitespace-only pieces without retaining their lengths, so _recursive_split does not advance over them; for example, splitting Alpha alpha alpha.\n\nBeta beta beta. with a small chunk size assigns the Beta chunk a source slice of \n\nBet, and every following offset remains shifted. Preserve the consumed separator gaps or derive spans from match positions before translating them through the source map.
Useful? React with 👍 / 👎.
…arators in offset accounting Signed-off-by: Charan Rathore <180254320+charan-rathore@users.noreply.github.com> Normalize whitespace in chunk creation process.
…ed-off-by: Charan Rathore <180254320+charan-rathore@users.noreply.github.com>
…gressions Signed-off-by: Charan Rathore <180254320+charan-rathore@users.noreply.github.com> Added tests for overlap coverage and discarded separators in RecursiveChunker.
|
Deployment failed for project intellirag-live-own-track with the following error: Learn More: https://vercel.com/own-track?upgradeToPro=build-rate-limit |
|
Deployment failed for project intellirag with the following error: Learn More: https://vercel.com/own-track?upgradeToPro=build-rate-limit |
…ints Signed-off-by: Charan Rathore <180254320+charan-rathore@users.noreply.github.com> Refactor whitespace normalization to use NormalizationMap for tracking source indices. Update related functions to accommodate the new mapping structure.
|
Deployment failed for project irag-fix-0904 with the following error: Learn More: https://vercel.com/own-track?upgradeToPro=build-rate-limit |
|
@codex review All three findings addressed, each with failing-first regressions:
Chunking suite: 153 passed, 2 skipped; whole repo 263 passed, same 26 pre-existing failures outside the change. Signed-off-by: Charan Rathore 180254320+charan-rathore@users.noreply.github.com |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3480fda1aa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # The overlap is a suffix of the previous span: move the | ||
| # start back over it so citations/highlights cover the | ||
| # whole chunk text, not just the current span. | ||
| start = previous_end - len(overlap) |
There was a problem hiding this comment.
When adjacent spans have been merged, merge_small_spans can add a synthetic "\n\n" even though the following span already contains its separator, so len(previous_text) may exceed previous_end - previous_start. With a sufficiently large overlap that returns the whole merged text, this subtraction produces a negative start (for example, four paragraph-sized spans with chunk_size=35, chunk_overlap=30, min_chunk_size=15, and max_chunk_size=40 yields start_char_offset=-2), and Python slicing then omits the entire overlap. Fresh evidence in the current tree is that the new length-based correction assumes the generated overlap is a source suffix; track its actual normalized source start instead. The identical calculation in the semantic chunker needs the same treatment.
Useful? React with 👍 / 👎.
| if j != prev_src + (len(out) - prev_norm): | ||
| checkpoints.append((len(out), j)) | ||
| else: | ||
| out.append(c) |
There was a problem hiding this comment.
For documents with little collapsible whitespace, this branch appends one list element for every source character until the final join, so the supposedly sparse implementation still carries roughly 9 bytes of transient traced allocation per ASCII character in addition to the source and result strings. Large ingestion inputs can therefore consume hundreds of megabytes solely for out and still OOM; accumulate unchanged runs or use a streaming string buffer so auxiliary memory is governed by normalization boundaries rather than document length.
Useful? React with 👍 / 👎.
…n Signed-off-by: Charan Rathore <180254320+charan-rathore@users.noreply.github.com> Refactor chunk overlap handling to clamp overlap length to prevent negative start offsets.
…egion Signed-off-by: Charan Rathore <180254320+charan-rathore@users.noreply.github.com> Added a test to ensure overlap start does not go before the previous span's source region.
|
@codex review Fixed: the overlap is now clamped to the previous span's source-backed length ( Signed-off-by: Charan Rathore 180254320+charan-rathore@users.noreply.github.com |
…pends Signed-off-by: Charan Rathore <180254320+charan-rathore@users.noreply.github.com> Updated normalization logic to handle newline characters and adjusted normalization length calculation.
|
@codex review Addressed the normalization-memory finding: the output side no longer appends one list element per source character - unchanged characters are flushed as whole source slices, so allocations scale with the number of runs instead of document length. Behavior-preserving: a 3000-case fuzz check against the previous implementation gives byte-identical output and in-bounds mapping, and the chunking suite (154) plus the whole repo suite pass unchanged. Signed-off-by: Charan Rathore 180254320+charan-rathore@users.noreply.github.com |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Problem
The recursive chunker normalized whitespace before computing offsets, then recorded the normalized positions as offsets into the original source. Every space run, extra newline, or leading/trailing whitespace shifted all later offsets, so citations and highlight spans sliced the wrong text.
Before - source
" Alpha beta.\n\nGamma delta. "produced one chunk with offsets(0, 25), butsource[0:25]is" Alpha beta.\n\nGamma de"- it ends mid-word and includes leading whitespace:After - the same chunk now carries
(2, 31), andsource[2:31]is exactly the chunk's content:(The chunk text itself is the normalized string; the offsets now point at where that content actually lives in the source, so slicing and normalizing the slice reproduces the chunk.)
Fix
chunking/utils.pygainsnormalize_whitespace_with_map, which performs the same transformation asnormalize_whitespacebut also returns an index map from normalized position to original-source position, plusremap_offsets_to_sourceto translate a chunk's offsets through that map (end offsets stay exclusive).recursive.py,semantic.py(all three return paths), andstructure_aware.pyapply the remap as a post-pass before building the result.github_chunkeris untouched: its summary-chunk offsets index synthesized text (# {title}\n\n...), not the source document, so there is nothing to remap.Tests
source[start:end]must cover the chunk text (after whitespace normalization) for the recursive, structure-aware, and semantic chunkers. Verified RED on main, GREEN with the fix.chunking/on both.Signed-off-by: Charan Rathore 180254320+charan-rathore@users.noreply.github.com