test(chunker): self-contained guard for splitOversizedUnit running-sum flush - #17740
Merged
yuzhichang merged 1 commit intoAug 3, 2026
Merged
Conversation
…g-sum flush Add split_oversized_guard_test.go pinning splitOversizedUnitWith's whitespace-atom sub-split against Python's rag/nlp._split_oversized_unit: - TestSplitOversizedUnitRunningSumMatchesPython asserts the exact piece boundaries (live tokenizer), compensating the slack=1 relaxation in token_strict_cap_test.go that shipped with PR infiniflow#17729. - TestSplitOversizedUnitDeadTokenizerCollapses asserts a zero-counting tokenizer collapses the B1 paragraph into exactly one chunk, catching a silently dead encoder that a non-empty-result check would miss. Self-contained: no harness loader, no testdata; the Python oracle is inlined. Function names are distinct from PR infiniflow#17735's TestSplitOversizedUnitMatchesPython so both PRs verify independently. Co-Authored-By: CodeBuddy <noreply@cnb.cool>
dosubot
Bot
added
the
size:XS
Contributor
📝 WalkthroughWalkthroughThe PR adds tests for oversized chunk splitting. The tests compare live-tokenizer boundaries with Python-compatible results and verify that a zero-counting tokenizer returns one collapsed chunk. ChangesOversized chunk test coverage
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/ingestion/component/chunker/split_oversized_guard_test.go`:
- Around line 79-81: Strengthen the assertion in the dead-tokenizer test after
the existing len(got) check by verifying that got[0] exactly equals b1Text. Keep
the chunk-count assertion and ensure the test fails for empty or truncated
collapsed text.
🪄 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: d4359a7d-65d3-4f13-b714-75442c820844
📒 Files selected for processing (1)
internal/ingestion/component/chunker/split_oversized_guard_test.go
Comment on lines
+79
to
+81
| if len(got) != 1 { | ||
| t.Fatalf("dead tokenizer must collapse paragraph into exactly one chunk, got %d", len(got)) | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Verify the collapsed text, not only the chunk count.
If splitOversizedUnitWith returns one empty or truncated chunk, this test passes. After the length check, compare got[0] with b1Text.
Proposed test fix
if len(got) != 1 {
t.Fatalf("dead tokenizer must collapse paragraph into exactly one chunk, got %d", len(got))
}
+ if got[0] != b1Text {
+ t.Fatalf("dead tokenizer must preserve the complete paragraph")
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if len(got) != 1 { | |
| t.Fatalf("dead tokenizer must collapse paragraph into exactly one chunk, got %d", len(got)) | |
| } | |
| if len(got) != 1 { | |
| t.Fatalf("dead tokenizer must collapse paragraph into exactly one chunk, got %d", len(got)) | |
| } | |
| if got[0] != b1Text { | |
| t.Fatalf("dead tokenizer must preserve the complete paragraph") | |
| } |
🤖 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 `@internal/ingestion/component/chunker/split_oversized_guard_test.go` around
lines 79 - 81, Strengthen the assertion in the dead-tokenizer test after the
existing len(got) check by verifying that got[0] exactly equals b1Text. Keep the
chunk-count assertion and ensure the test fails for empty or truncated collapsed
text.
Collaborator
Author
|
@yuzhichang Pls help to review. |
Member
|
LGTM |
This was referenced Aug 4, 2026
yuzhichang
pushed a commit
that referenced
this pull request
Aug 6, 2026
## Summary - Remove `splitOversizedUnit`, `splitAtomByTokenBudget` and `atomRE` from `internal/ingestion/component/chunker/token.go`. - Delete `split_oversized_guard_test.go` (added by #17740), which guarded the removed atom-split behaviour. - Drop the now-unused `wordCount`/`charCount` helpers from `token_strict_cap_test.go`. - Add `TestMergeByTokenSize_OversizedUnitStaysWhole` to pin the #17799 contract invariant (over-budget unit stays whole, never atom-split) on the **text path**. The JSON path is already covered by `TestMergeByTokenSizeFromJSON_OversizedUnitStaysWhole`. ## Why The production merge path (`mergeByTokenSize` / `mergeByTokenSizeFromJSON`) keeps over-budget units whole and relies on the embedding/rerank layer to truncate them, per the TokenChunker contract (#17799: remove atom-split, no hard_cap). The deleted helpers implemented the opposite behaviour and had **no production caller**, so they contradicted the contract and misled readers into thinking atom-split was active. ## Parser vs chunker layering Python's `_split_oversized_unit` lives at the **parser layer** (pre-split before `naive_merge`), not in the chunker. Go's parser backends are currently skeletons, so there is no parser-side equivalent yet; if added later it belongs in `internal/parser/parser/*`, not the chunker. ## Test plan `bash build.sh --test ./internal/ingestion/component/chunker/...` passes; the new text-path test passes and the orphaned atom-split tests are gone. ## Changes - 3 files changed, 32 insertions(+), 250 deletions(-)
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
2 participants
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.
Summary
Adds a self-contained regression guard for
splitOversizedUnitWithafter PR #17729aligned it with Python's
rag/nlp._split_oversized_unitrunning-sum flush.#17729 shipped a
slack=1relaxation intoken_strict_cap_test.go(the oversizedunit is now sub-split with the same running-sum flush Python uses, which can leave a
piece one token over the nominal budget due to cl100k non-additivity). This PR adds
the missing positive proof that the sub-split boundaries are correct, so the relaxed
assertion is no longer unguarded.
Changes
split_oversized_guard_test.go(new, self-contained — no harness loader, notestdata; the Python oracle is inlined):TestSplitOversizedUnitRunningSumMatchesPython: asserts the exact pieceboundaries (live tokenizer) match Python's
_split_oversized_unit, compensatingthe
slack=1relaxation from fix(chunker): align splitOversizedUnitWith with Python running-sum flush #17729.TestSplitOversizedUnitDeadTokenizerCollapses: asserts a zero-counting tokenizercollapses the B1 paragraph into exactly one chunk, catching a silently dead encoder
that a non-empty-result check would miss.
Notes
TestSplitOversizedUnitMatchesPython, so the two PRs verify independently and do notconflict at merge time.
Test plan
bash build.sh --test ./internal/ingestion/component/chunker/— green, including bothnew tests.