fix(semantic): split sentences that exceed chunk_size#627
fix(semantic): split sentences that exceed chunk_size#627chakshu-dhannawat wants to merge 2 commits into
Conversation
SemanticChunker only ever split between sentences, never inside one. So a single sentence longer than chunk_size (common with delimiter-free blobs like OpenAPI specs or long JSON) was emitted as one oversized chunk. On the OpenAI OpenAPI YAML this produced a ~48k token chunk with chunk_size=2048. _split_groups now splits any sentence larger than chunk_size into token-sized pieces (same approach as TokenChunker) before packing, so no chunk exceeds the limit. The few-sentences early-return path now routes through _split_groups too, since it previously skipped size enforcement entirely. Added regression tests for both paths. Closes feyninc#222
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughSemanticChunker now splits oversized sentences into chunk_size-bounded token pieces via a new helper, and applies this enforcement in both group-splitting logic and the short-input path. Regression tests cover oversized single sentences and short inputs with oversized content. ChangesSemantic Chunker Size Enforcement
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Chunk as chunk()
participant SplitGroups as _split_groups
participant SplitSentence as _split_oversized_sentence
participant CreateChunks as _create_chunks
Chunk->>SplitGroups: _split_groups([sentences])
SplitGroups->>SplitSentence: split sentence if token_count > chunk_size
SplitSentence-->>SplitGroups: return Sentence pieces
SplitGroups-->>Chunk: return grouped pieces within chunk_size
Chunk->>CreateChunks: _create_chunks(groups)
CreateChunks-->>Chunk: return chunks respecting chunk_size
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to split oversized sentences that exceed the configured chunk_size in the SemanticChunker, ensuring that chunks respect the size limit even when a single sentence is extremely long. This includes handling the early-return path for fewer sentences and adding corresponding regression tests. The review feedback correctly identifies an issue in _split_oversized_sentence where all split sentence pieces are assigned the same start_index, leading to incorrect and overlapping index ranges, and provides a code suggestion to properly offset the indices.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| return [ | ||
| Sentence( | ||
| text=text, | ||
| start_index=sentence.start_index, | ||
| end_index=sentence.start_index + len(text), | ||
| token_count=len(group), | ||
| ) | ||
| for text, group in zip(texts, token_groups) | ||
| ] |
There was a problem hiding this comment.
In the current implementation, all split Sentence pieces are created with the same start_index (i.e., sentence.start_index). This results in overlapping and incorrect index ranges for the split sentences. Instead, we should accumulate the character lengths of the processed pieces to correctly offset the start_index and end_index for each subsequent piece.
sentences = []
current_start = sentence.start_index
for text, group in zip(texts, token_groups):
sentences.append(
Sentence(
text=text,
start_index=current_start,
end_index=current_start + len(text),
token_count=len(group),
)
)
current_start += len(text)
return sentences![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/chunkers/test_semantic_chunker.py (1)
101-103: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the actual tokenized chunk text, not only metadata.
These regressions can still pass if
Chunk.token_countis underreported whilechunk.textexceeds the tokenizer limit. Count each emitted chunk’s text with the chunker tokenizer to verify the downstream-safe invariant.Proposed test strengthening
- assert all([chunk.token_count <= chunk_size for chunk in chunks]), ( + assert all(chunker.tokenizer.count_tokens(chunk.text) <= chunk_size for chunk in chunks), ( "Every chunk must respect chunk_size even when a single sentence is oversized" ) @@ - assert all([chunk.token_count <= chunk_size for chunk in chunks]), ( + assert all(chunker.tokenizer.count_tokens(chunk.text) <= chunk_size for chunk in chunks), ( "The few-sentences path must also split oversized sentences" )Also applies to: 123-125
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/chunkers/test_semantic_chunker.py` around lines 101 - 103, The semantic chunker test is only checking Chunk.token_count, which can miss regressions where chunk.text is actually too long. Update the affected assertions in test_semantic_chunker to verify each emitted chunk’s text by re-tokenizing chunk.text with the same chunker tokenizer and comparing that count to chunk_size. Use the existing chunk/chunks assertions in the semantic chunker test to enforce the downstream-safe invariant, not just the metadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/chonkie/chunker/semantic.py`:
- Line 416: The semantic chunking path in semantic.py is calling
tokenizer.decode_batch on token_groups, but that method is not guaranteed by the
tokenizer protocol and can break custom tokenizers that only implement
encode/decode. Update the logic in the chunking routine around the decode step
to iterate over each token group and use tokenizer.decode for each one,
preserving protocol compatibility while keeping the rest of the
oversized-sentence handling unchanged.
---
Nitpick comments:
In `@tests/chunkers/test_semantic_chunker.py`:
- Around line 101-103: The semantic chunker test is only checking
Chunk.token_count, which can miss regressions where chunk.text is actually too
long. Update the affected assertions in test_semantic_chunker to verify each
emitted chunk’s text by re-tokenizing chunk.text with the same chunker tokenizer
and comparing that count to chunk_size. Use the existing chunk/chunks assertions
in the semantic chunker test to enforce the downstream-safe invariant, not just
the metadata.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bec8cc71-e556-4a33-b124-f6356629c0b5
📒 Files selected for processing (2)
src/chonkie/chunker/semantic.pytests/chunkers/test_semantic_chunker.py
- Decode token groups one at a time with decode() instead of decode_batch(), which isn't part of the tokenizer protocol and can break custom tokenizers. - Accumulate character offsets so split pieces get sequential start/end indices instead of all sharing the sentence's start. - Regression tests now re-tokenize the chunk text so they can't pass on underreported token_count alone.
|
Pushed a follow-up addressing the review notes:
Tests still green (28 passed) and ruff is clean. |
1 participant
Fixes #222.
Problem
SemanticChunkeronly splits between sentences, never within one. When a single sentence tokenizes to more thanchunk_size(common with delimiter-free content like OpenAPI/JSON specs or very long lines), it was emitted as a single oversized chunk. The issue reporter saw a ~48,811 token chunk withchunk_size=2048on the OpenAI OpenAPI YAML.There were actually two code paths that skipped the size limit:
_split_groupspacks sentences into groups but assumed every individual sentence already fit withinchunk_size. A sentence larger thanchunk_sizejust became its own group unchanged.chunk()(when there are<= similarity_windowsentences) built a single chunk with no size check at all.Fix
_split_oversized_sentence(), which splits a too-large sentence into token-sized pieces usingtokenizer.encode/decode_batch, the same wayTokenChunkeralready does._split_groupscalls it before packing, so no piece ever exceedschunk_size._split_groups+_create_chunksso it enforces the limit too. Output is unchanged when the content already fits.Reproduce (before the fix)
Tests
Added two regression tests (oversized sentence in the normal path, and in the few-sentences path). Both fail on
mainand pass with this change. Fulltest_semantic_chunker.pysuite is green (28 passed).ruff checkandruff format --checkare clean on the touched files.One note: token-level splitting decodes token slices, so on uncased tokenizers the split pieces come back lowercased, this matches
TokenChunker's existing behavior and only affects sentences that were too large to keep whole anyway.Summary by CodeRabbit
chunk_size, including when a single sentence is too large.chunk_size.