Closed revert: align pipeline delimiter chunking (#17723) - #17774
Closed revert: align pipeline delimiter chunking (#17723)#17774xugangqiang wants to merge 1 commit into
Conversation
This reverts commit 9ccb23e.
dosubot
Bot
added
the
size:M
📝 WalkthroughWalkthroughThe parser now enforces token limits when merging and splitting text. TokenChunker preserves delimiters during pattern-based splitting and simplifies text and JSON paths. Parser integration no longer passes ChangesChunking behavior
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@rag/flow/chunker/token_chunker.py`:
- Around line 320-329: In the chunking flow around cks and the corresponding
logic near the alternate referenced block, add content-free logging.debug calls
after each delimiter-splitting or token-size-merging path completes. Log which
path was selected, the configured chunk token size, and the number of emitted
chunks, without including payload content.
- Around line 80-91: Preserve every configured delimiter during split output
construction: update the delimiter loop around the compiled pattern so
delimiters following empty segments are appended to the prior chunk or buffered
for the next text segment instead of discarded. In
rag/flow/chunker/token_chunker.py lines 80-91, retain delimiters for cases such
as adjacent matches; in lines 320-329, continue filtering blank chunks with
c.strip() but emit the original c whenever delimiter splitting is active,
avoiding stripping retained whitespace delimiters.
🪄 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: af79df00-09f5-4cb7-ad05-552063300c9e
📒 Files selected for processing (5)
deepdoc/parser/txt_parser.pyrag/flow/chunker/token_chunker.pyrag/flow/parser/parser.pytest/unit_test/deepdoc/parser/test_txt_parser.pyweb/src/utils/__tests__/delimiter-preview.test.ts
💤 Files with no reviewable changes (1)
- rag/flow/parser/parser.py
| # Split text by the compiled delimiter pattern and keep delimiter text in each chunk. | ||
| if not pattern: | ||
| return [text or ""] | ||
|
|
||
| split_texts = re.split(r"(%s)" % pattern, text or "", flags=re.DOTALL) | ||
| chunks = [] | ||
| for i in range(0, len(split_texts), 2): | ||
| chunk = split_texts[i] | ||
| if not chunk: | ||
| continue | ||
| if i + 1 < len(split_texts): | ||
| chunk += split_texts[i + 1] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve delimiters through splitting and output construction.
The delimiter flow loses configured delimiter text. For A####B with a ## pattern, Lines 86-91 discard the second delimiter because its preceding split segment is empty. For a whitespace delimiter such as `\n`, the new route then removes the retained delimiter with c.strip() at Line 341.
rag/flow/chunker/token_chunker.py#L80-L91: retain delimiters that follow an empty segment by appending them to the prior chunk or buffering them for the next text segment.rag/flow/chunker/token_chunker.py#L320-L329: filter blank chunks withc.strip(), but emitcwithout stripping when delimiter splitting is active.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 83-83: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.split(r"(%s)" % pattern, text or "", flags=re.DOTALL)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-python)
📍 Affects 1 file
rag/flow/chunker/token_chunker.py#L80-L91(this comment)rag/flow/chunker/token_chunker.py#L320-L329
🤖 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 `@rag/flow/chunker/token_chunker.py` around lines 80 - 91, Preserve every
configured delimiter during split output construction: update the delimiter loop
around the compiled pattern so delimiters following empty segments are appended
to the prior chunk or buffered for the next text segment instead of discarded.
In rag/flow/chunker/token_chunker.py lines 80-91, retain delimiters for cases
such as adjacent matches; in lines 320-329, continue filtering blank chunks with
c.strip() but emit the original c whenever delimiter splitting is active,
avoiding stripping retained whitespace delimiters.
| cks = ( | ||
| _split_text_by_pattern(payload, delimiter_pattern) | ||
| if delimiter_pattern | ||
| else naive_merge( | ||
| payload, | ||
| self._param.chunk_token_size, | ||
| "".join(self._param.delimiters), | ||
| overlapped_percent, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add debug logs for the changed chunking decisions.
These paths now select delimiter splitting or token-size merging. They do not record the selected path, token budget, or emitted chunk count. Add content-free logging.debug records after each path completes.
As per coding guidelines, “Add logging for new flows.”
Also applies to: 360-365
🤖 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 `@rag/flow/chunker/token_chunker.py` around lines 320 - 329, In the chunking
flow around cks and the corresponding logic near the alternate referenced block,
add content-free logging.debug calls after each delimiter-splitting or
token-size-merging path completes. Log which path was selected, the configured
chunk token size, and the number of emitted chunks, without including payload
content.
Source: Coding guidelines
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #17774 +/- ##
=======================================
Coverage 90.65% 90.65%
=======================================
Files 10 10
Lines 717 717
Branches 118 118
=======================================
Hits 650 650
Misses 39 39
Partials 28 28 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
wangq8
left a comment
wangq8
left a comment
There was a problem hiding this comment.
Thanks for the thorough revert and detailed rationale.
What I checked:
txt_parser.py: Restores_split_oversized_unitwith the proactive token-cap check and merged-token projection — this correctly prevents oversized chunks.token_chunker.py: Restores delimiter-keeping split (gluing delimiter to preceding segment) and removes the brokendelimiter_mode == "delimiter"JSON branch that was concatenating adjacent items with"".join.parser.py: Removes thekeep_delimiters=Trueoverride that was passed toRAGFlowTxtParser.test_txt_parser.py: Restores the 178-line guard test covering no-overshoot, boundary packing, oversized-line sub-splitting, empty input, and newline-join token cap edge cases.delimiter-preview.test.ts: Restores the 4-line formatting expectation.
One note: The PR description mentions a follow-up to re-align the Go side (internal/ingestion/component/chunker/token.go) to the restored Python behavior. Please make sure that follow-up is tracked (issue or linked PR) so the Go/Python parity doesn't drift.
LGTM.
xugangqiang
changed the title
|
Closing: this revert of #17723 targets the wrong object. The regression actually originates in #17203 (strict-cap atom-split), and #17723's delimiter handling had its own bugs (consecutive-delimiter drop, JSON glue, ignored children_delimiters / PDF positions). The fix is PR #17808, which removes atom-split, keeps delimiter as a chunk boundary, and preserves the #17723 delimiter direction — without re-introducing delimiter-in-chunk or the strict cap. Reverting #17723 would undo the correct delimiter direction and re-add the hard cap. |
xugangqiang
changed the title
…P default) (#17808) ## Summary Fixes a regression introduced by #17203 (strict-cap atom-split) and a secondary delimiter-handling bug from #17723. **Root cause:** - #17203 added `_split_oversized_unit` / `_compute_chunk_update`, which split oversize units into ≤ token_size pieces. This collapsed `token_size=1` into 1-token chunks and set the cap at 512, mismatching the model-layer truncation boundary (embedding ~8191 / rerank 500/4096/8192/2048). Atom-split is unnecessary: oversize units stay whole and the model layer truncates. - #17723's delimiter handling dropped consecutive delimiters (`A####B` -> `A##B`), glued JSON items with `"".join`, ignored `children_delimiters`, and stripped whitespace delimiters. ## Changes - New pure helper `merge_paragraphs(paragraphs, token_size, strategy)` with a `MergeStrategy` enum (`UNDER_CAP` / `OVER_CAP`); **default `OVER_CAP`**. `UNDER_CAP` is a strict cap (never overflows `token_size`); `OVER_CAP` greedily accumulates adjacent paragraphs while the projected total stays within `token_size`, merging one boundary-overflow paragraph before closing. Oversize paragraphs stand alone. - `naive_merge` / `naive_merge_with_images` / `RAGFlowTxtParser.parser_txt` now use `merge_paragraphs`; atom-split removed. `naive_merge` / `naive_merge_with_images` always split a section on the delimiter whenever one is present (even when the section already fits `token_size`), so delimiter text never leaks into a chunk. Only the empty-delimiter (size-only) mode skips splitting. - `token_chunker`: delimiter text is dropped (not stripped); JSON flush joins buffered items with `"\n"`; `children_delimiters` and `PDF_POSITIONS_KEY` are preserved on the delimiter path. PDF positions are now attributed **per segment** — each split chunk carries only the positions of the item(s) that contributed to it — fixing a leak where page-N coordinates were attached to page-M chunks and all segments shared one preview image. - `test_txt_parser.py` rewritten to assert the new contract (not the old strict cap); `naive_merge` and delimiter-case-sensitive matrices updated. ## Contract (refs #17799) - user specified delimiter = chunk boundary; user specified delimiter text never enters a chunk. - `token_size` = soft target + merge strategy; no atom-split. - Default strategy = `OVER_CAP`; migration can switch to `UNDER_CAP` (strict cap). - `OVER_CAP` has no hard cap; the model layer truncates oversize units. `UNDER_CAP` enforces a strict cap. ## Notes - Closes the wrong-object revert in #17774 (revert #17723 would re-introduce delimiter-in-chunk and the strict cap). - Go-side alignment (`internal/ingestion/component/chunker/token.go`) is a follow-up PR. --------- Co-authored-by: CodeBuddy <noreply@tencent.com>
2 participants
Summary
Reverts #17723 (
fix: align pipeline delimiter chunking).After review we concluded that #17723 should be reverted and the delimiter-chunking behavior reworked from a clean base, rather than fixed incrementally on top of it. The revert restores the pre-#17723 Python behavior byte-for-byte (verified against the parent commit
3e7cfbe05).Why revert instead of fix-on-top
#17723 introduced several regressions against the intended chunking contract and removed behavior that the default path still relies on:
deepdoc/parser/txt_parser.pyremoved_split_oversized_unitand the proactivetk_nums[-1] > chunk_token_numcap, so over-budget sections are no longer sub-split. The pre-fix: align pipeline delimiter chunking #17723 path enforced the cap correctly._split_text_by_patternwas changed from keeping the delimiter (glued to the preceding segment) to silently dropping it for every delimiter mode, includingtoken_size, which changes chunk text content for existing users.delimiter_mode == "delimiter"JSON branch. The new JSON branch flushes the text buffer with"".join(...)(adjacent item text gets concatenated, e.g.helloworld), does not carryPDF_POSITIONS_KEY, and returns early — bypassing_split_chunk_docs_by_children, sochildren_delimitersis silently ignored wheneverdelimiter_mode == "delimiter"+children_delimitersare combined.test/unit_test/deepdoc/parser/test_txt_parser.py(178 lines) — which protected the token-cap invariant — was removed.Restoring the pre-#17723 state is lower-effort than undoing each regression individually, and gives a clean base for the intended blend model (
delimiter= preferred break point,token_size= hard upper bound, the two are orthogonal and combinable).Changes in this PR
Reverts the 5 files changed by #17723:
deepdoc/parser/txt_parser.py— restores_split_oversized_unitand the proactive token cap.rag/flow/chunker/token_chunker.py— restores delimiter-keeping split and the pre-fix: align pipeline delimiter chunking #17723 JSON/text paths.rag/flow/parser/parser.py— removes thekeep_delimiters=Truechange.test/unit_test/deepdoc/parser/test_txt_parser.py— restores the deleted guard test.web/src/utils/__tests__/delimiter-preview.test.ts— restores the 4-line preview expectation.Known follow-up (out of scope here)
The Go side was aligned to the post-#17723 Python behavior in #17729 / #17739 / #17740 (
internal/ingestion/component/chunker/token.go). After this revert, a follow-up PR will re-align Go to the restored Python behavior and implement the blend model. Parity-infra PRs #17735 / #17744 are intentionally deferred per the discussion.Test plan
python -m pytest test/unit_test/deepdoc/parser/test_txt_parser.pypasses (restored test).pytestonrag/flow/chunkerpasses.delimiter_mode=delimiter,delimiter_mode=token_size, anddelimiter_mode=oneproduce the pre-fix: align pipeline delimiter chunking #17723 outputs.