Fix: delimiter is chunk boundary, drop token_size atom-split (OVER_CAP default) - #17808
Merged
yuzhichang merged 6 commits intoAug 5, 2026
Merged
Conversation
…P default) PR infiniflow#17203 introduced a strict-cap atom-split (_split_oversized_unit / _compute_chunk_update) that collapsed token_size=1 into 1-token chunks and mismatched the model-layer truncation boundary. PR infiniflow#17723's delimiter handling then dropped consecutive delimiters (A####B -> A##B), glued JSON items with "".join", and ignored children_delimiters and PDF positions. Changes: - Add pure merge_paragraphs(paragraphs, token_size, strategy) with a MergeStrategy enum (RESPECT_CAP / OVER_CAP); default OVER_CAP. - naive_merge / naive_merge_with_images / RAGFlowTxtParser.parser_txt use merge_paragraphs; atom-split is removed so oversize units stay whole and the model layer truncates them. - token_chunker: delimiter text is dropped (not stripped); JSON flush joins buffered items with "\n"; children_delimiters and PDF_POSITIONS_KEY are preserved through the delimiter path. - test_txt_parser.py rewritten to assert the new contract (not the old strict cap); naive_merge and delimiter-case-sensitive matrices updated. Refs infiniflow#17799. Closes the wrong-object revert in infiniflow#17774.
3 tasks
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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)
📝 WalkthroughWalkthroughThe PR replaces atom-level token splitting with strategy-based paragraph grouping. It updates text, image, parser, and delimiter-mode chunking flows. It preserves oversized units, delimiter boundaries, whitespace, overlap behavior, and PDF positions. ChangesChunking refactor
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@deepdoc/parser/txt_parser.py`:
- Line 21: Update the text parser module around its merge_paragraphs usage to
import or define a parser-local num_tokens_from_string counter, ensuring
txt_mod.num_tokens_from_string remains monkeypatchable by tests. Pass this
counter explicitly to merge_paragraphs instead of relying on the function’s
definition-time size value.
In `@rag/flow/chunker/token_chunker.py`:
- Around line 373-388: Update the chunking flow around _split_text_by_pattern
and the children_delimiters handling to track text spans together with their
source PDF positions through both split stages. When emitting each chunk or
child, attach only the positions contributing to that segment instead of the
aggregate combined_pos; preserve existing empty-segment filtering and add a
regression test covering text split across two pages.
- Around line 367-391: Update flush_text_buffer in the buffered text flow to add
a debug log reporting only the number of buffered items, emitted segments, and
aggregated positions. Track or compute these counts around the join, position
aggregation, and split/emission steps; do not include chunk text or coordinate
values in the log.
In `@rag/nlp/__init__.py`:
- Around line 1240-1242: Update the paragraph grouping logic around the `elif i
+ 1 < n` branch to inspect the next paragraph’s size before pairing it; only
group adjacent paragraphs when the second paragraph is within cap, otherwise
emit the oversized `paragraphs[i + 1]` as its own group while preserving the
documented OVER_CAP behavior. Add coverage for paragraph sizes [60, 150, 60].
- Around line 1354-1361: Update the paragraph-processing logic around the
section split and the corresponding image-paragraph logic in rag/nlp/__init__.py
at lines 1354-1361 and 1417-1424: append the whole section only when no
delimiter pattern exists, regardless of token count, and always apply re.split
when dels is configured so the merge strategy groups the resulting paragraphs
correctly.
🪄 Autofix
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: b088a3ed-e128-404a-a0ce-744ac54d9b79
📒 Files selected for processing (9)
deepdoc/parser/txt_parser.pyrag/flow/chunker/token_chunker.pyrag/flow/tests/test_token_chunker.pyrag/flow/tests/test_token_chunker_delimiter.pyrag/nlp/__init__.pytest/unit_test/deepdoc/parser/test_txt_parser.pytest/unit_test/rag/test_delimiter_case_sensitive.pytest/unit_test/rag/test_merge_paragraphs.pytest/unit_test/rag/test_naive_merge.py
Comment on lines
367
to
+391
| def flush_text_buffer(): | ||
| if not text_buffer: | ||
| return | ||
| combined_text = "".join(text_buffer) | ||
| # Join buffered text items with "\n" so adjacent item text is not | ||
| # glued together (e.g. "hello" + "world" must not become "helloworld"). | ||
| # PDF coordinates are carried on the combined chunk. | ||
| combined_text = "\n".join(text_buffer) | ||
| combined_pos = [] | ||
| for pos in text_buffer_pos: | ||
| combined_pos.extend(pos or []) | ||
| split_texts = _split_text_by_pattern(combined_text, delimiter_pattern) | ||
| chunks.extend( | ||
| { | ||
| "text": text, | ||
| "doc_type_kwd": "text", | ||
| "ck_type": "text", | ||
| "tk_nums": num_tokens_from_string(text), | ||
| } | ||
| for text in split_texts | ||
| if text.strip() | ||
| ) | ||
| for text in split_texts: | ||
| if not text.strip(): | ||
| continue | ||
| chunks.append( | ||
| { | ||
| "text": text, | ||
| "doc_type_kwd": "text", | ||
| "ck_type": "text", | ||
| PDF_POSITIONS_KEY: deepcopy(combined_pos), | ||
| "tk_nums": num_tokens_from_string(text), | ||
| } | ||
| ) | ||
| text_buffer.clear() | ||
| text_buffer_pos.clear() |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a debug log for the new buffer flush flow.
Line 367 adds JSON delimiter buffering and position aggregation without an operational log. Log count-only fields for buffered items, emitted segments, and aggregated positions. Do not log chunk text or coordinates.
As per coding guidelines, “Add logging for new flows.”
🤖 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 367 - 391, Update
flush_text_buffer in the buffered text flow to add a debug log reporting only
the number of buffered items, emitted segments, and aggregated positions. Track
or compute these counts around the join, position aggregation, and
split/emission steps; do not include chunk text or coordinate values in the log.
Source: Coding guidelines
Comment on lines
+1240
to
+1242
| elif i + 1 < n: | ||
| groups.append([i, i + 1]) | ||
| 10BC0 | i += 2 |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep an oversized second paragraph standalone.
When paragraphs[i] fits but paragraphs[i + 1] exceeds cap, Line 1241 emits both paragraphs in one group. This violates the documented OVER_CAP rule that an oversized paragraph stands alone. Check the next paragraph before pairing. Add coverage for sizes [60, 150, 60].
Proposed change
- elif i + 1 < n:
+ elif i + 1 < n and size(paragraphs[i + 1]) <= cap:
groups.append([i, i + 1])
i += 2📝 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
| elif i + 1 < n: | |
| groups.append([i, i + 1]) | |
| i += 2 | |
| elif i + 1 < n and size(paragraphs[i + 1]) <= cap: | |
| groups.append([i, i + 1]) | |
| i += 2 |
🤖 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/nlp/__init__.py` around lines 1240 - 1242, Update the paragraph grouping
logic around the `elif i + 1 < n` branch to inspect the next paragraph’s size
before pairing it; only group adjacent paragraphs when the second paragraph is
within cap, otherwise emit the oversized `paragraphs[i + 1]` as its own group
while preserving the documented OVER_CAP behavior. Add coverage for paragraph
sizes [60, 150, 60].
…ze at call time The new test_txt_parser.py monkeypatched the wrong module (deepdoc.parser.txt_parser has no num_tokens_from_string attribute; parser_txt routes through rag.nlp.merge_paragraphs, which captured the tokenizer default at definition time). monkeypatch.setattr then raised AttributeError in CI. - merge_paragraphs: resolve `size` at call time (default None -> num_tokens_from_string) so the tokenizer can be monkeypatched deterministically. - test_txt_parser.py: monkeypatch rag.nlp.num_tokens_from_string instead, making the OVER_CAP grouping assertions independent of a live tiktoken encoder.
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 `@rag/nlp/__init__.py`:
- Around line 1249-1261: Update merge_paragraphs, or its nearest integration
boundary if purity must be preserved, to emit one bounded debug-level log for
each merge operation. Include strategy, input paragraph count, token_size, and
resulting group count, while excluding all paragraph contents and avoiding
additional logging.
🪄 Autofix
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: de97a300-2147-48d5-8ae0-1439bb471072
📒 Files selected for processing (2)
rag/nlp/__init__.pytest/unit_test/deepdoc/parser/test_txt_parser.py
🚧 Files skipped from review as they are similar to previous changes (1)
- test/unit_test/deepdoc/parser/test_txt_parser.py
Comment on lines
+1249
to
+1261
| def merge_paragraphs(paragraphs, token_size, strategy=MergeStrategy.OVER_CAP, size=None): | ||
| """Group delimiter-split ``paragraphs`` into chunks using ``strategy``. | ||
|
|
||
| Pure function: no pos / PDF coordinate handling, no atom-split. Returns a | ||
| list of chunks, each a list of the original paragraph strings (order and | ||
| identity preserved). ``token_size`` is a soft target; see ``MergeStrategy``. | ||
|
|
||
| def _split_oversized_unit(text, chunk_token_num, token_count_fn=None): | ||
| """Split a single unit that exceeds ``chunk_token_num`` tokens into pieces | ||
| that each fit the budget. Whitespace is used as the primary break (mirrors | ||
| ``RAGFlowHtmlParser._split_oversized_block``); a single run of non-whitespace | ||
| longer than the budget falls back to token-budget-based character windows. | ||
| ``size`` defaults to ``num_tokens_from_string`` and is resolved at call | ||
| time (not captured at definition) so tests can monkeypatch the tokenizer | ||
| deterministically via ``rag.nlp.num_tokens_from_string``. | ||
| """ | ||
| if token_count_fn is None: | ||
| token_count_fn = num_tokens_from_string | ||
| if token_count_fn(text or "") <= chunk_token_num: | ||
| return [text] | ||
| pieces = [] | ||
| current = "" | ||
| current_tokens = 0 | ||
| token_cache = {} | ||
|
|
||
| def atom_tokens(atom): | ||
| if atom.isspace(): | ||
| return 0 | ||
| if atom not in token_cache: | ||
| token_cache[atom] = token_count_fn(atom) | ||
| return token_cache[atom] | ||
|
|
||
| # Match whitespace runs OR non-whitespace runs (i.e. individual words/tokens). | ||
| for atom in re.findall(r"\s+|\S+", text or ""): | ||
| a_tokens = atom_tokens(atom) | ||
| if a_tokens > chunk_token_num and not atom.isspace(): | ||
| A0BA | # An atom longer than the budget: flush current buffer, then carve | |
| # token-budget-based slices out of the atom itself. | ||
| if current: | ||
| pieces.append(current) | ||
| current = "" | ||
| current_tokens = 0 | ||
| for sub_piece in _split_atom_by_token_budget(atom, chunk_token_num, token_count_fn): | ||
| pieces.append(sub_piece) | ||
| if size is None: | ||
| size = num_tokens_from_string |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add bounded logging for the new merge flow.
This new merge entry point emits no operational signal. Add one debug-level event here, or at the nearest integration boundary if this helper must remain pure. Include strategy, paragraph count, token_size, and output group count. Do not log paragraph contents.
As per coding guidelines, **/*.py: Add logging for new flows.
🤖 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/nlp/__init__.py` around lines 1249 - 1261, Update merge_paragraphs, or
its nearest integration boundary if purity must be preserved, to emit one
bounded debug-level log for each merge operation. Include strategy, input
paragraph count, token_size, and resulting group count, while excluding all
paragraph contents and avoiding additional logging.
Source: Coding guidelines
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #17808 +/- ##
=======================================
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:
|
xugangqiang
marked this pull request as draft
August 4, 2026 11:13
…it small sections at delimiter - MergeStrategy.RESPECT_CAP renamed to UNDER_CAP (strict hard cap: never overflows token_size). OVER_CAP now greedily accumulates adjacent paragraphs while the projected total stays within token_size; when the next paragraph would exceed, it is merged anyway (one boundary overflow allowed), then the chunk is closed. Oversized paragraphs stand alone. Replaces the previous pairwise (max-two) implementation that severely under-filled many small segments. - naive_merge / naive_merge_with_images now split every section on the delimiter whenever one is present, even when the section already fits token_size. Previously a small section was kept whole, leaking delimiter text into the chunk. Only the empty-delimiter (size-only) mode skips splitting. - Tests updated to the corrected contract: pairwise/hard-cap assertions moved to UNDER_CAP; greedy accumulation and delimiter-boundary assertions added. Co-Authored-By: CodeBuddy <noreply@tencent.com>
…ter mode) In the JSON delimiter_mode path, consecutive text items were buffered, joined, and split by the delimiter; every resulting segment was then attached the union of ALL buffered items' PDF positions. For multi-page input this leaked page-N coordinates into page-M chunks and made all segments share one preview image. Track each buffered item's character range in the combined text and, for every split segment, collect only the positions of the items whose range intersects the segment. A segment spanning an item boundary (the "\n" glue is not a delimiter) still carries both pages' coordinates; single-page segments no longer carry foreign coordinates. Segmentation is unchanged (same re.split + even-index selection), so existing behaviour is preserved. A regression test feeds two adjacent text items from different pages split by a custom delimiter and asserts each segment carries only its own page coordinates and that preview images are not shared. Co-Authored-By: CodeBuddy <noreply@tencent.com>
Collaborator
Author
On the two "needs evaluation" points (chunk-text hash change; rerank/embedding truncation)1. Chunk-text change → existing chunk hashes / dedup / hit Documents that are never re-parsed keep the old-format chunks, but those chunks are still valid retrievable text; they simply coexist in a mixed format. There is no data corruption. Therefore no migration script is required — at most a release note stating "re-parse affected documents if you want a uniform format." (The only residual edge case is global cross-document dedup during a partial re-parse, where old-format and new-format hashes of overlapping text will not collide. This is minor and transient.) 2. rerank / embedding truncation of oversized units The real (but narrow) regression introduced by removing oversize splitting entirely: a genuinely oversized single paragraph (e.g., a PDF long unbroken line) is now kept whole and gets silently truncated at the model boundary during rerank, instead of being split to fit. Note the old hard-coded 512 cap was itself below real model limits (over-splitting), so "old code kept things under the limit" was true but not optimal. A reasonable, separate follow-up (independent of the OVER_CAP contract) is to split truly oversized units at the real model upper bound — |
xugangqiang
marked this pull request as ready for review
August 4, 2026 13:33
xugangqiang
marked this pull request as draft
August 4, 2026 13:47
xugangqiang
marked this pull request as ready for review
August 4, 2026 13:48
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rag/nlp/__init__.py (2)
1381-1393: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftBudget embedded position tags in
UNDER_CAP.
_merge_paragraph_groups()measures onlyp[0], but_reconstruct_text_chunk()appendsp[1]later. When the tokenizer counts that suffix, a non-oversized group exceedschunk_token_num. The changed test simulates this case and accepts the overflow, which breaks the strictUNDER_CAPcontract.
rag/nlp/__init__.py#L1381-L1393: Include reconstructed position tags in the group budget, or keep them outside returned chunk text.test/unit_test/rag/test_naive_merge.py#L347-L356: Restore a strict<= chunk_token_numassertion after the implementation budgets the tag.As per coding guidelines, remove “move later” notes instead of preserving them.
🤖 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/nlp/__init__.py` around lines 1381 - 1393, The naive_merge paragraph grouping currently excludes position tags from the token budget, allowing _reconstruct_text_chunk to produce chunks exceeding chunk_token_num. In rag/nlp/__init__.py lines 1381-1393, make _merge_paragraph_groups budget the same reconstructed text, including each position tag, while preserving returned chunk contents; in test/unit_test/rag/test_naive_merge.py lines 347-356, restore the strict <= chunk_token_num assertion and remove any “move later” note.Source: Coding guidelines
1437-1461: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSize image group chunks before position-tag reconstruction.
naive_merge_with_images()groups[p[0] for p in paragraphs], but_reconstruct_image_chunk()appendstext_posafter merging when it is missing. Use reconstructed chunk text for size checks/overflow handling, or rebuild before measuring.🤖 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/nlp/__init__.py` around lines 1437 - 1461, Update naive_merge_with_images() so chunk sizing and overflow decisions account for position tags added by _reconstruct_image_chunk(). Reconstruct each candidate group before applying size checks, or otherwise measure the fully reconstructed chunk text, while preserving the existing image association and output behavior.
🧹 Nitpick comments (1)
test/unit_test/rag/test_merge_paragraphs.py (1)
119-123: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the external-memory reference.
feedback_over_cap_contractis not a repository reference. The surrounding comments already define the contract. Delete Line 122.As per coding guidelines, remove stale superseded comments and documentation.
Proposed cleanup
-# See memory: feedback_over_cap_contract.🤖 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 `@test/unit_test/rag/test_merge_paragraphs.py` around lines 119 - 123, Remove the stale “See memory: feedback_over_cap_contract.” line from the OVER_CAP greedy accumulation comment block in test_merge_paragraphs.py, leaving the existing contract description unchanged.Source: Coding guidelines
🤖 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 409-414: Update the segment-to-item matching loop in the token
chunking flow to use a forward cursor over the ordered item_ranges and
text_buffer_pos instead of rescanning all items for each segment. Advance past
ranges ending before seg_start, inspect only ranges that can overlap
seg_start–seg_end, and preserve collecting item_pos coordinates for every
overlap while keeping the cursor monotonic across emitted segments.
In `@test/unit_test/rag/test_naive_merge.py`:
- Around line 160-168: Strengthen the UNDER_CAP tests around naive_merge to
verify overlap behavior directly, using sentences with distinct tokens so chunk
membership is observable. Compare each next chunk against the calculated tail
prefix of the preceding chunk, asserting overlap is inserted when within the
strict budget and omitted when it would exceed the cap; update the related tests
in the additional range consistently.
---
Outside diff comments:
In `@rag/nlp/__init__.py`:
- Around line 1381-1393: The naive_merge paragraph grouping currently excludes
position tags from the token budget, allowing _reconstruct_text_chunk to produce
chunks exceeding chunk_token_num. In rag/nlp/__init__.py lines 1381-1393, make
_merge_paragraph_groups budget the same reconstructed text, including each
position tag, while preserving returned chunk contents; in
test/unit_test/rag/test_naive_merge.py lines 347-356, restore the strict <=
chunk_token_num assertion and remove any “move later” note.
- Around line 1437-1461: Update naive_merge_with_images() so chunk sizing and
overflow decisions account for position tags added by
_reconstruct_image_chunk(). Reconstruct each candidate group before applying
size checks, or otherwise measure the fully reconstructed chunk text, while
preserving the existing image association and output behavior.
---
Nitpick comments:
In `@test/unit_test/rag/test_merge_paragraphs.py`:
- Around line 119-123: Remove the stale “See memory:
feedback_over_cap_contract.” line from the OVER_CAP greedy accumulation comment
block in test_merge_paragraphs.py, leaving the existing contract description
unchanged.
🪄 Autofix
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: 9cc894a3-06a9-4c19-83e3-880fa1ed33e9
📒 Files selected for processing (6)
rag/flow/chunker/token_chunker.pyrag/flow/tests/test_token_chunker.pyrag/nlp/__init__.pytest/unit_test/deepdoc/parser/test_txt_parser.pytest/unit_test/rag/test_merge_paragraphs.pytest/unit_test/rag/test_naive_merge.py
🚧 Files skipped from review as they are similar to previous changes (2)
- test/unit_test/deepdoc/parser/test_txt_parser.py
- rag/flow/tests/test_token_chunker.py
Comment on lines
+409
to
+414
| seg_pos = [] | ||
| for (istart, iend), item_pos in zip(item_ranges, text_buffer_pos): | ||
| # A segment overlaps an item when their character ranges | ||
| # intersect; collect that item's coordinates. | ||
| if seg_start < iend and istart < seg_end: | ||
| seg_pos.extend(item_pos or []) |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Avoid quadratic source-position matching.
Lines 409-414 rescan every buffered item for every emitted segment. N buffered items and N delimiter segments cause N² interval comparisons. Large JSON documents can block this chunking flow. Sweep the ordered ranges with an item cursor and inspect only overlapping items.
Proposed bounded scan
- for text, seg_start, seg_end in segments:
+ item_index = 0
+ for text, seg_start, seg_end in segments:
if not text.strip():
continue
+ while (
+ item_index < len(item_ranges)
+ and item_ranges[item_index][1] <= seg_start
+ ):
+ item_index += 1
+
seg_pos = []
- for (istart, iend), item_pos in zip(item_ranges, text_buffer_pos):
- # A segment overlaps an item when their character ranges
- # intersect; collect that item's coordinates.
- if seg_start < iend and istart < seg_end:
- seg_pos.extend(item_pos or [])
+ scan_index = item_index
+ while scan_index < len(item_ranges):
+ istart, iend = item_ranges[scan_index]
+ if istart >= seg_end:
+ break
+ if seg_start < iend:
+ seg_pos.extend(text_buffer_pos[scan_index] or [])
+ scan_index += 1📝 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
| seg_pos = [] | |
| for (istart, iend), item_pos in zip(item_ranges, text_buffer_pos): | |
| # A segment overlaps an item when their character ranges | |
| # intersect; collect that item's coordinates. | |
| if seg_start < iend and istart < seg_end: | |
| seg_pos.extend(item_pos or []) | |
| item_index = 0 | |
| for text, seg_start, seg_end in segments: | |
| if not text.strip(): | |
| continue | |
| while ( | |
| item_index < len(item_ranges) | |
| and item_ranges[item_index][1] <= seg_start | |
| ): | |
| item_index += 1 | |
| seg_pos = [] | |
| scan_index = item_index | |
| while scan_index < len(item_ranges): | |
| istart, iend = item_ranges[scan_index] | |
| if istart >= seg_end: | |
| break | |
| if seg_start < iend: | |
| seg_pos.extend(text_buffer_pos[scan_index] or []) | |
| scan_index += 1 |
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 410-410: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
🤖 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 409 - 414, Update the
segment-to-item matching loop in the token chunking flow to use a forward cursor
over the ordered item_ranges and text_buffer_pos instead of rescanning all items
for each segment. Advance past ranges ending before seg_start, inspect only
ranges that can overlap seg_start–seg_end, and preserve collecting item_pos
coordinates for every overlap while keeping the cursor monotonic across emitted
segments.
Comment on lines
+160
to
168
| # UNDER_CAP (strict): content chunks never overflow chunk_token_num, so the | ||
| # overlap-prefix budget check is the only thing under test here. | ||
| chunks = _nonempty(naive_merge(sentences, chunk_token_num=50, delimiter=DEFAULT_DELIMITER, overlapped_percent=20, strategy=MergeStrategy.UNDER_CAP)) | ||
| assert len(chunks) > 1 | ||
| # Each chunk stays within the budget. Sentences are 10 tokens, the budget | ||
| # is 50, so even a 10-token overlap prefix (20% of 50) fits a 40-token | ||
| # remainder and the projected-total guarantee holds exactly. | ||
| # Each content chunk stays within the budget. Sentences are 10 tokens, the | ||
| # budget is 50, so a 5-sentence chunk is exactly 50; a 10-token overlap | ||
| # prefix (20% of 50) would push it to 60 and is therefore dropped at the | ||
| # boundary rather than letting the chunk overshoot. | ||
| assert all(_tok(c) <= 50 for c in chunks) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert overlap insertion and omission directly.
The cap-only assertions pass if overlap is never added. The membership check also passes without overlap because every token is "w". Use distinct content and compare the calculated tail prefix from one chunk with the next chunk. Assert that the prefix is absent when it would exceed the strict cap.
Also applies to: 283-325
🤖 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 `@test/unit_test/rag/test_naive_merge.py` around lines 160 - 168, Strengthen
the UNDER_CAP tests around naive_merge to verify overlap behavior directly,
using sentences with distinct tokens so chunk membership is observable. Compare
each next chunk against the calculated tail prefix of the preceding chunk,
asserting overlap is inserted when within the strict budget and omitted when it
would exceed the cap; update the related tests in the additional range
consistently.
xugangqiang
marked this pull request as draft
August 5, 2026 01:46
xugangqiang
marked this pull request as ready for review
August 5, 2026 01:46
xugangqiang
marked this pull request as draft
August 5, 2026 01:48
xugangqiang
marked this pull request as ready for review
August 5, 2026 01:48
Collaborator
Author
|
@yuzhichang @buua436 pls help to review |
Member
|
LGTM |
…e_paragraphs contract (infiniflow#17799)
xugangqiang
added a commit
to xugangqiang/ragflow
that referenced
this pull request
Aug 5, 2026
Extract a shared mergeDecision helper (mergeAction + allowBoundaryOverflow) and newChunkText, and rewrite the text (addChunk) and JSON (addTextChunk) merge paths to use it. Go now matches Python's canonical OVER_CAP default (rag/nlp MergeStrategy): greedily accumulate adjacent units, allow one boundary overflow then close the chunk, but never merge a unit that already exceeds the cap (it stands alone, mirroring Python's `if pt > cap` branch). Update the cap/overlap tests to the OVER_CAP contract (a chunk may exceed budget by at most one incoming unit) and restructure the overlap-tag test to exercise the overlap path under OVER_CAP. Part of consolidating infiniflow#17744/infiniflow#17748 into a single Go PR that syncs the chunker with Python PR infiniflow#17808 (OVER_CAP default).
yuzhichang
pushed a commit
that referenced
this pull request
Aug 5, 2026
…egy enum (#17851) ## Summary Follow-up to #17835 (merged). The OVER_CAP / UNDER_CAP merge strategy was threaded through `mergeDecision` and `mergeByTokenSizeFromJSON` as an inlined `allowBoundaryOverflow bool` derived from `!c.param.UnderCap` at three call sites. This replaces that with a named `schema.MergeStrategy` enum. ## Why - The `!c.param.UnderCap` inversion was hand-written in three places, so a future strategy addition could silently drift between the JSON path (`invokeTextPayload` / `invokeJSONPayload`) and the text path (`mergeByTokenSize`) — no compile error, and the existing tests don't cover all three sites with both strategies. - The strategy concept was never named; `allowBoundaryOverflow` (true = OVER_CAP) is a double-negation of `UnderCap` and reads opaquely at the 5th positional argument. ## What changed - Add `schema.MergeStrategy` (`MergeOverCap` / `MergeUnderCap`) mirroring Python's `rag/nlp/__init__.py` `MergeStrategy`, so Go and Python stay on the same vocabulary. - Expose `TokenChunkerParam.MergeStrategy()` derived from the wire-facing `UnderCap bool` (existing `"under_cap"` configs keep working — no schema break). - `mergeDecision` and `mergeByTokenSizeFromJSON` now take `schema.MergeStrategy` instead of `allowBoundaryOverflow bool`; the three call sites pass `c.param.MergeStrategy()` (no `!`). - Tests updated to pass the enum; added a guard test for the `UnderCap` -> `MergeStrategy` mapping and an end-to-end test for UNDER_CAP on the JSON path. No behavior change: default remains OVER_CAP, `under_cap=true` still selects UNDER_CAP. ## Test plan `bash build.sh --test ./internal/ingestion/component/chunker/... ./internal/ingestion/component/schema/...` — all green, including `TestMergeByTokenSizeFromJSON_UnderCapNoOverflow`, `TestMergeByTokenSize_UnderCapNoOverflow`, `TestInvokeJSONPayload_UnderCapEndToEnd`, and `TestTokenChunkerParamMergeStrategy`. ## Related issues - Relates to #17835 — wired UNDER_CAP as a tested merge-strategy seam (merged) - Relates to #17799 — contract doc for token-chunker cap/delimiter alignment - Relates to #17808 — related chunker alignment work --------- Co-authored-by: CodeBuddy <noreply@cnb.cool>
2 tasks
This was referenced Aug 5, 2026
yuzhichang
pushed a commit
that referenced
this pull request
Aug 6, 2026
…17889) (#17896) ## Background Issue #17889 asks that, when merging adjacent segments, the chunker first checks each segment's type and only merges **text** segments — **table**, **image**, and any other non-text type must each remain a standalone chunk and must never be merged with a neighbouring segment. ## Why this PR closes #17889 (no Go code change required) After tracing the Go TokenChunker, the requirement is **already satisfied** on the structured (JSON / chunks) path. The type-aware rule is enforced at three layers in `internal/ingestion/component/chunker/`: - `common.go:138` `itemDocType` derives the type from `doc_type_kwd` (`"table"` -> `"table"`, `"image"` -> `"image"`, anything else -> `"text"`). It does **not** depend on the `ck_type` field being populated, so the type survives even when only `doc_type_kwd` is set (e.g. upstream Title/Group/Hierarchy chunks). - `token.go:756` `chunkFromItem` emits a non-text item as a single standalone chunk before the merge loop ever runs. - `token.go:1050` `mergeByTokenSizeFromJSON` forces any non-text chunk standalone (`if ck.CKType != "text"`); and `token.go:991` starts a *fresh* text chunk after a non-text chunk, so text on either side of a table/image is never merged across it. The only path without type information is the raw markdown/text/html string path (`PayloadFormatMarkdown/Text/HTML`), where the input is by contract an untyped string and `applyChildrenDelim` hard-codes `CKType: "text"` so merging is correct. There is no non-text segment to merge there, so this is out of #17889's scope (which is about the merge logic). ## Why the Python side is deferred The Python `naive` parser path does not thread a `ck_type` through to `merge_paragraphs` / `naive_merge` / `naive_merge_with_images` (`rag/nlp/__init__.py`): its parsers emit flat `(text, pos)` sections plus a parallel `section_images` list, and the type-aware `_merge_cks` rule (`rag/nlp/__init__.py:1749`) is only wired into the docx path. Propagating `ck_type` end-to-end across every Python parser is a large refactor, so it is intentionally **not** part of this PR. The Go engine is the active ingestion path, and it already honors the rule. ## This PR Adds a regression-lock (characterization) test, not a fix: - `TestTokenChunker_InvokeJSONPayload_KeepsNonTextStandalone` feeds a `[text, table, text, image, text]` structured payload and asserts it produces exactly five standalone chunks in the order `text, table, text, image, text` — proving tables/images stay standalone and text on either side is not merged across them. Verified green: ``` bash build.sh --test -run TestTokenChunker_InvokeJSONPayload_KeepsNonTextStandalone ./internal/ingestion/component/chunker/... --- PASS: TestTokenChunker_InvokeJSONPayload_KeepsNonTextStandalone (0.07s) ``` ## Related - Issue #17889 - PR #17808 (chunking refactor, merged) - Contract doc #17799
xugangqiang
added a commit
to xugangqiang/ragflow
that referenced
this pull request
Aug 10, 2026
…finiflow#17723/infiniflow#17808) Python reference changed via infiniflow#17723 (custom-delimiter rewrite) and infiniflow#17808 (OVER_CAP default + delimiter boundary). Re-capture all golden fixtures so the parity oracle reflects current Python before the Go-side alignment work. Co-authored-by: xugangqiang <>
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
3 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
Fixes a regression introduced by #17203 (strict-cap atom-split) and a secondary delimiter-handling bug from #17723.
Root cause:
_split_oversized_unit/_compute_chunk_update, which split oversize units into ≤ token_size pieces. This collapsedtoken_size=1into 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.A####B->A##B), glued JSON items with"".join, ignoredchildren_delimiters, and stripped whitespace delimiters.Changes
merge_paragraphs(paragraphs, token_size, strategy)with aMergeStrategyenum (UNDER_CAP/OVER_CAP); defaultOVER_CAP.UNDER_CAPis a strict cap (never overflowstoken_size);OVER_CAPgreedily accumulates adjacent paragraphs while the projected total stays withintoken_size, merging one boundary-overflow paragraph before closing. Oversize paragraphs stand alone.naive_merge/naive_merge_with_images/RAGFlowTxtParser.parser_txtnow usemerge_paragraphs; atom-split removed. F440naive_merge/naive_merge_with_imagesalways split a section on the delimiter whenever one is present (even when the section already fitstoken_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_delimitersandPDF_POSITIONS_KEYare 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.pyrewritten to assert the new contract (not the old strict cap);naive_mergeand delimiter-case-sensitive matrices updated.Contract (refs #17799)
token_size= soft target + merge strategy; no atom-split.OVER_CAP; migration can switch toUNDER_CAP(strict cap).OVER_CAPhas no hard cap; the model layer truncates oversize units.UNDER_CAPenforces a strict cap.Notes
internal/ingestion/component/chunker/token.go) is a follow-up PR.