Validate float chunk_overlap in TokenChunker, not just int - #633
Validate float chunk_overlap in TokenChunker, not just int#633santhreal wants to merge 2 commits into
Conversation
chunk_overlap accepts an int or a float (a fraction of chunk_size). Only the int case was checked against chunk_size, so a float that resolves to int(chunk_overlap * chunk_size) >= chunk_size slipped through. That made the stride chunk_size - chunk_overlap zero or negative: chunk_overlap=1.0 crashed chunk() with 'range() arg 3 must not be zero', and chunk_overlap=1.5 silently returned zero chunks, dropping the entire input. Validate the resolved chunk_overlap after the int/float conversion so both paths are held to the same 'less than chunk_size' invariant, and reject a negative overlap as well.
|
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)
📝 WalkthroughWalkthrough
ChangesToken overlap validation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 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 improves the validation of the chunk_overlap parameter in TokenChunker by ensuring that resolved float overlaps are held to the same invariants as integer overlaps, preventing zero or negative strides. It also adds several unit tests to verify these constraints. The review feedback highlights a potential issue where small negative float overlaps (e.g., -0.05) could bypass the non-negative check due to integer truncation, and suggests validating the input chunk_overlap before resolution. Additionally, it recommends adding test cases to verify that negative overlaps are correctly rejected.
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.
| if self.chunk_overlap < 0: | ||
| raise ValueError("chunk_overlap must be non-negative") | ||
| if self.chunk_overlap >= chunk_size: | ||
| raise ValueError("chunk_overlap must be less than chunk_size") |
There was a problem hiding this comment.
Checking self.chunk_overlap < 0 after resolving it to an integer can lead to inconsistent behavior for small negative float values due to truncation. For example, if chunk_overlap = -0.05 and chunk_size = 10, int(-0.05 * 10) resolves to 0, which silently bypasses the non-negative check. However, if chunk_size = 100, int(-0.05 * 100) resolves to -5, which correctly raises a ValueError.
To ensure consistent validation regardless of the chunk_size, we should validate that the input chunk_overlap is non-negative before resolving it.
| if self.chunk_overlap < 0: | |
| raise ValueError("chunk_overlap must be non-negative") | |
| if self.chunk_overlap >= chunk_size: | |
| raise ValueError("chunk_overlap must be less than chunk_size") | |
| if chunk_overlap < 0: | |
| raise ValueError("chunk_overlap must be non-negative") | |
| if self.chunk_overlap >= chunk_size: | |
| raise ValueError("chunk_overlap must be less than chunk_size") |
| assert "".join(dict.fromkeys([c.text[0] for c in chunks])) == "x" | ||
| # Every original character is covered by the first chunk's start through the last chunk's end. | ||
| assert chunks[0].start_index == 0 | ||
| assert chunks[-1].end_index == len(text) |
There was a problem hiding this comment.
It would be beneficial to add test cases to verify that negative values for chunk_overlap (both integer and float) are correctly rejected by the chunker.
| assert chunks[-1].end_index == len(text) | |
| assert chunks[-1].end_index == len(text) | |
| def test_token_chunker_rejects_negative_overlap() -> None: | |
| """Negative chunk_overlap values must be rejected.""" | |
| with pytest.raises(ValueError, match="chunk_overlap must be non-negative"): | |
| TokenChunker(tokenizer="character", chunk_size=10, chunk_overlap=-1) | |
| with pytest.raises(ValueError, match="chunk_overlap must be non-negative"): | |
| TokenChunker(tokenizer="character", chunk_size=10, chunk_overlap=-0.1) |
Small negative floats truncated to 0 via int() and bypassed the post-resolution check.
1 participant
chunk_overlapaccepts an int or a float (a fraction ofchunk_size), but only the int case was checked againstchunk_size. A float that resolves toint(chunk_overlap * chunk_size) >= chunk_sizeslipped through and made the stridechunk_size - chunk_overlapzero or negative:chunk_overlap=1.0(withchunk_size=10) crashedchunk()withValueError: range() arg 3 must not be zero;chunk_overlap=1.5produced a negative stride andchunk()silently returned zero chunks, dropping the entire input.Now the resolved
chunk_overlapis validated after the int/float conversion, so both paths share the sameless than chunk_sizeinvariant (a negative overlap is rejected too). Added regression tests covering the int and float cases and a valid fractional overlap.Summary by CodeRabbit
Bug Fixes
Tests