feat(chunk): split content at sensible text boundaries instead of a fixed character count - #3211
Open
marevol wants to merge 16 commits into
Open
feat(chunk): split content at sensible text boundaries instead of a fixed character count#3211marevol wants to merge 16 commits into
marevol wants to merge 16 commits into
Conversation
…r predicates Add the ChunkBoundaryFinder LastaDi component that will hold the boundary-aware chunk-splitting search. This task implements only the character-classification predicates (breakable space, newline, sentence terminator, clause separator, brackets, script boundary, cluster continuation) plus the skeleton findChunkEnd/snapOverlapStart methods, which return the ideal offset unchanged until later tasks fill in the search logic. Register the chunkBoundaryFinder component in fess_chunk.xml so it is available in both the main and chunk-indexer child JVMs.
…aryFinder isBreakableSpace disagreed with isNewline about U+0085 (NEL): Character.isWhitespace(0x0085) returns false and NEL was missing from the explicit switch, so isSkippableAfterBoundary (built on isBreakableSpace) never treated NEL as skippable. That would have made a NEL line break silently fail to register as a boundary once the reverse scan lands, while every other newline works. Add 0x0085 to isBreakableSpace's switch and pin the invariant with test assertions in test_isBreakableSpace_coversUnicodeSpacesJavaMisses and test_isSkippableAfterBoundary_spacesAndClosingMarks.
…inder Implement the backward scan in ChunkBoundaryFinder.findChunkEnd: a single reverse pass from the ideal cut offset collects STRONG (sentence-ending), WEAK (clause-separating), and SCRIPT (writing-system change) boundary candidates within the lookback window, preferring the nearest STRONG candidate and falling back to WEAK/SCRIPT. Adds alignToCodePoint so the scan and its callers never land inside a surrogate pair. The forward search, grapheme-cluster fallback, and snapOverlapStart are left for later tasks.
Address four defects found in review of the backward scan: - searchBackward discarded a pending skippable run when the window ended mid-run, silently dropping a valid STRONG/WEAK candidate and making the result non-monotonic in lookback. Classify the pending run after the loop instead of throwing it away. - The SCRIPT tier kept calling isScriptBoundary (a Unicode block table binary search) even after a WEAK candidate already made it dead work; gate it on weak == NO_CANDIDATE too. - Character.codePointAt(text, b) was decoded twice per iteration (opening-bracket and script checks) even though it is bit-for-bit the `before` value the previous iteration already decoded. Carry it across iterations in a single local instead of re-decoding. - alignToCodePoint left the offset splitting a surrogate pair when ideal == start + 1, because pulling back would reach start. Add an upperBound parameter so it can escape forward past the pair instead. Adds regression tests for each fix and rewrites test_findChunkEnd_neverReturnsAtOrBeforeStart, which previously could not fail, so the Math.max(start + 1, ...) clamp is load-bearing.
…ryFinder Fix round 2 review findings: - findChunkEnd's new `after` initializer decoded codePointAt(text, b) guarded only by `b < limitEnd`, but the in-range bound is text.length(). An oversized limitEnd (out of contract, but the method is a public DI component another deployment can call) now throws StringIndexOutOfBoundsException instead of returning a wrong answer. Clamp once at the top of findChunkEnd via `bound = Math.min(limitEnd, text.length())` and use bound everywhere in the method, including the calls to alignToCodePoint and searchBackward. When the contract holds (limitEnd == text.length()) behavior is unchanged. - The SCRIPT tier's nearest-wins rule (script == NO_CANDIDATE) had no regression test; deleting it left the suite green. Added a test with two script changes in the window and confirmed by temporarily removing the guard that the test goes red without it. Adds test_findChunkEnd_oversizedLimitEnd_doesNotThrow and test_findChunkEnd_nearestScriptChangeWinsWithinTheTier.
Complete ChunkBoundaryFinder.findChunkEnd: - searchForward scans ahead of the ideal offset for a sentence end or line break, but only runs when no backward STRONG boundary was found; it beats a backward WEAK/SCRIPT candidate. - adjustToClusterStart is the hard-cut fallback: it walks the offset back far enough to avoid splitting a grapheme cluster (combining marks, variation selectors, ZWJ sequences). When walking backward cannot clear the chunk start (e.g. a ZWJ-joined emoji sequence that begins exactly at the chunk start), it walks forward past the cluster instead, so the boundary still never splits it. Adds MAX_CLUSTER_ADJUST and the forward-search/cluster-adjust test coverage in ChunkBoundaryFinderTest.
…ogate pair adjustToClusterStart is now three explicit phases instead of "walk back, then clamp": walk backward for a safe cut (never below start), then walk forward from the original offset if backward found nothing, stepping strictly by Character.charCount so the cursor stays code-point aligned, then give up and return the original offset if neither direction found a safe cut. This fixes two bugs found during review of the forward-search task: - The old forward escape clamped to upperBound unconditionally, which could land inside a surrogate pair when upperBound itself fell between a pair's two halves. - The old backward walk gave up as soon as it reached start + 1 without testing that position itself, so a cluster one offset away from start was still split. Updates findChunkEnd's javadoc to document that the returned offset can overshoot ideal by up to lookahead code points (forward sentence search) and, independently and not governed by lookahead, by up to 2 * MAX_CLUSTER_ADJUST UTF-16 units (grapheme-cluster escape) -- because returning a cluster-splitting offset defeats the purpose of this search. Adds regression tests for both fixed cases and strengthens the existing never-goes-at-or-before-start test to assert cluster safety (or the documented give-up) instead of only bounds.
- findChunkEnd's javadoc previously implied the forward-sentence-search overshoot and the grapheme-cluster-escape overshoot could stack, and understated the bound by one unit. Corrected: the true bound past ideal is max(lookahead, 2 * MAX_CLUSTER_ADJUST) + 1 UTF-16 units -- the two paths are mutually exclusive (findChunkEnd returns as soon as the forward search succeeds), and the extra +1 comes from alignToCodePoint's own forward escape off a surrogate pair, which can run before either search does. - adjustToClusterStart's backward walk used the loop condition `b > start` after the prior restructure, which reaches codePointBefore(text, 0) and throws when start is negative (out of contract, but reachable through the public findChunkEnd on a component deployments are invited to subclass). Clamped the floor to Math.max(start, 0), a no-op for the in-contract start >= 0 case. Adds a regression test for the negative-start case.
The previous round guarded only adjustToClusterStart's backward walk
against a negative start, but searchBackward has the identical hazard
through the same public entry point: its floor is
Math.max(start + 1, ideal - lookback), never clamped to 0, so
findChunkEnd("ab", -1, 1, 2, 1, 0) still threw
StringIndexOutOfBoundsException.
Fixed at the single point of entry instead of patching each helper
again: findChunkEnd now computes `origin = Math.max(start, 0)`,
mirroring the existing `bound = Math.min(limitEnd, text.length())`
clamp, and threads `origin` through every downstream use -- the
alignToCodePoint call, the base <= start guard, the searchBackward
call, and the adjustToClusterStart call.
Removed the now-redundant Math.max(start, 0) added inside
adjustToClusterStart in the prior round: it is a private method with a
single call site (findChunkEnd), which already guarantees a
non-negative start once origin is threaded through, so the inner
clamp duplicated an invariant the caller already enforces.
Verified start >= 0 behaviour is bit-for-bit identical to before this
change: compiled the pre-fix and post-fix classes side by side and
diffed 702,000 in-contract findChunkEnd calls across a range of texts,
offsets, and window sizes -- zero mismatches.
Extends the existing negative-start regression test to cover both the
cluster-escape path and the backward-scan path.
…act input Out-of-contract arguments (ideal <= start, or ideal past end/text) previously made snapOverlapStart hand back an offset outside its own documented start < result <= end contract. It now returns `end` (dropping the overlap for that step) instead, matching the promised postcondition.
… LengthChunker Add the three content_chunker.length.boundary.* configuration keys (enabled, lookback_percent, lookahead_percent) and their defaults/maxima, plus lazy resolution of the chunkBoundaryFinder LastaDi component with a built-in fallback. Not yet wired into split(); that lands in a follow-up task. Also documents the three new keys and the boundary-aware splitting behaviour in fess_config.properties.
…racter count Wire LengthChunker.split(String, int) to ChunkBoundaryFinder.findChunkEnd / snapOverlapStart so chunk boundaries land on a line break, sentence end, clause separator, space or script change instead of a blind fixed-length cut. Boundary-aware splitting is on by default (content_chunker.length.boundary.enabled=true); set it to false to keep the legacy fixed-length behaviour. The finder is resolved once per split call, never per chunk or cached on the singleton. Adds boundary-aware coverage to LengthChunkerTest (Japanese/English text, prefix consistency with limit, disabled/zero-percent parity with the legacy fixed-length split, forward-search overshoot ceiling, overlap contiguity, degenerate all-space/all-newline content, a 4M-character linearity guard, and the two boundary-percent warning tests deferred from the config task).
…urrogate fallback Review follow-up on the boundary-aware split wiring: - LengthChunker's class Javadoc and MAX_LOOKAHEAD_PERCENT's Javadoc claimed chunk_size + lookahead is a hard ceiling. It is not: ChunkBoundaryFinder's grapheme-cluster escape is not governed by lookahead and can fire even at lookahead_percent=0, so the real worst case is chunk_size + max(lookahead, 2 * MAX_CLUSTER_ADJUST) + 1 characters (841 at the shipped defaults, never less than 833). Corrected both doc comments; no behaviour change. - Reworded a test assertion message that repeated the same false ceiling as a general guarantee; the numeric bound is unchanged and still correct for that test's grapheme-cluster-free content. - Strengthened the overlap-contiguity test to track each chunk's verified start offset and search strictly after it for the next chunk, instead of a loosely-bounded content.indexOf lower bound that could mask a real skip when the same text repeats in the content. - Hardened split's forward-progress fallback to step a whole code point (Character.charCount(codePointAt(start))) instead of a flat +1, so a custom ChunkBoundaryFinder returning a mid-surrogate-pair offset can no longer strand a lone high surrogate that the next loop iteration would silently skip past. Unreachable with the shipped finder; defensive only.
…eam test gap Final pre-merge review fixes for the boundary-aware LengthChunker split. No behavioural change except a two-line refactor in ChunkBoundaryFinder that routes the preceding-code-point ZWJ check through an overridable predicate. - LengthChunker/ChunkBoundaryFinder Javadoc and fess_config.properties no longer describe boundary search as "nearest sensible break"; they now state the actual tier precedence (line break/sentence end, then clause separator/space, then script change) and widen "a comma" to the full clause-separator set. - LengthChunker's class Javadoc now documents the undershoot (a chunk can end up to `lookback` characters shorter than chunk_size) alongside the existing overshoot discussion, and corrects the "never less than 833" claim, which did not hold when both lookback_percent and lookahead_percent are 0. - ChunkBoundaryFinderTest gains a test that subclasses ChunkBoundaryFinder, overrides isSentenceTerminator, and asserts findChunkEnd picks a different offset than the base class -- pinning that the search actually dispatches through the protected predicates rather than a hardcoded switch. - findChunkEnd's Javadoc now notes its start/limitEnd guarantee holds only for in-contract arguments, matching the caveat already on snapOverlapStart. - MIN_CHUNK_SIZE's Javadoc no longer cites a surrogate-pair collision that the code-point-wide forward-progress fallback already closed; it is now documented as a sanity floor. - ChunkBoundaryFinder.adjustToClusterStart routes its preceding-code-point ZWJ check through a new overridable isClusterJoiner predicate, matching the existing overridable isClusterContinuation check on the following code point. MAX_CLUSTER_ADJUST's Javadoc now notes it is not actually subclass-tunable despite being protected, since the private method that reads it does not participate in virtual dispatch. - Renamed a LengthChunkerTest test whose name claimed to exercise a chunk-length ceiling that the test data never actually reaches.
…ffective overlap Review follow-ups on the boundary-aware splitter. Grapheme clusters were only protected on the hard-cut fallback. A STRONG, WEAK, SCRIPT or forward candidate was returned unchecked, so boundary search could split a cluster that the fixed-length cut it replaces had left intact -- the opposite of the escape's stated purpose. Candidates that would strand a combining mark, a variation selector or a joiner half are now rejected during the scan and the search continues. The two code points bracketing a run are carried across iterations, so the guard costs no extra decoding. This also makes findChunkEnd monotonic in lookback, which it was not before: a randomised sweep that previously produced 9,316 violations in 300,000 documents now produces none. The following-space rule kept ASCII 3.14 and 1,234 whole but did not cover the fullwidth forms, which are the decimal point, thousands separator and time separator of ordinary Japanese typography -- and U+FF0E was in the STRONG tier, so a decimal point outranked every space and comma in the window. It cannot simply be made to require a space either, because CJK sentences carry none. A new isPunctuationRequiringNonDigit rejects the fullwidth stop, comma and colon plus the hyphens when the next code point is a digit, so the fullwidth forms of 1.5, 1,234 and 10:30 as well as 2026-08-09 and UTF-8 stay whole, while the JIS sentence convention that writes the fullwidth stop for a period keeps working. U+2011 NON-BREAKING HYPHEN is no longer a break opportunity at all. snapOverlapStart was given the lookback window, which is derived from chunk_size rather than from overlap, and snapping only ever moves the restart point earlier -- so a configured overlap of 10 could become an effective 165 at the shipped defaults, silently multiplying the index duplication that warnOnOverlapSideEffect exists to warn about. The window is now capped at the configured overlap, bounding the effective overlap at twice the configured value. LengthChunker.register now logs the effective boundary configuration once, so an upgrade that turns chunk_size from a ceiling into a target leaves a trace to correlate an embedding-provider rejection against. Documentation corrections: the worst case reachable through LengthChunker is chunk_size + max(lookahead, 32), i.e. 840 and 832 at the defaults, not 841 and 833 -- the extra +1 needs ideal == start + 1, which MIN_CHUNK_SIZE makes unreachable; fess_config.properties stated 840 and 841 eight lines apart. The forward search accepts a line break as well as a sentence end. The rule is the nearest candidate of the highest tier present, not the nearest candidate. The lossless-reconstruction guarantee needs overlap=0. Chunks end up to lookback_percent of chunk_size shorter, not lookback_percent shorter, which yields measurably more chunks per document (+9% on English prose, up to +25%) and can push a document past max_chunks_per_document, where it is skipped outright. Tests: the round-trip assertions were tautological -- deleting the entire boundary search left 35 of 38 LengthChunkerTest tests green, because with overlap=0 any partition reconstructs the input. Added tests that assert where the cuts land, that boundary-on differs from boundary-off, that the overlap snap is actually wired into split(), that the effective-overlap cap holds, that the config channel and the boundary.enabled kill switch work through the real getSystemProperty path, and a differential check against an independent fixed-length reference splitter over cluster-rich content. Each was verified by mutating the production code and observing the test fail. The linearity guard now asserts a 4N/N ratio instead of an absolute wall-clock budget. Full suite: 6994 tests, 0 failures.
This was referenced Aug 9, 2026
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
1 participant
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
LengthChunkercut everycontent_chunker.length.chunk_sizecharacters regardless of what was at that offset, so words and sentences were routinely split in half. Because the chunks become both the searchablecontentarray and the embedding input, a cut landing mid-word degrades both.Each cut now moves to a suitable break near the ideal offset. No character is dropped -- only the cut point moves -- so with the default
content_chunker.length.overlap=0, concatenating a document's chunks still reproduces its content exactly.How boundaries are chosen
Candidates inside the search window are grouped into three tiers, and the nearest candidate of the highest tier present wins -- not simply the nearest candidate. A sentence end 100 characters back beats a space 2 characters back.
.。｡！？!?…‥and similar),、，;:・-and similar); space; bracketCOMMON/INHERITED/UNKNOWNare excluded, so digits, punctuation, emoji andーnever register)Punctuation that is ambiguous inside numbers is rejected two ways:
.,;:only count when whitespace (or the end of the content) follows, so3.14and1,234are never split at them.．，：and hyphens (-,‐) only count when the code point immediately after them is not a digit. CJK sentences carry no trailing space, so the whitespace rule cannot be used for them -- but１．５倍,１，２３４,１０：３０,2026-08-09andUTF-8must not be cut apart, while the JIS，．sentence convention (終わり．次は) must keep working.‑U+2011 NON-BREAKING HYPHEN is not a break opportunity at all.The SCRIPT tier is what keeps Japanese text without spaces or punctuation from falling back to a blind cut.
The search runs backward from the ideal offset. Only when no STRONG candidate exists behind it does it look ahead, and only for a STRONG break -- a sentence end or a line break -- so overshooting the chunk size is reserved for keeping a sentence or a line whole.
Unicode whitespace that
Character.isWhitespacerejects (NBSP, FIGURE SPACE, NNBSP, ZWSP, ZWNJ, WORD JOINER, BOM, NEL) is treated as breakable. Scanning is code-point-based, so a boundary can never split a UTF-16 surrogate pair, and no tier returns an offset that splits a grapheme cluster: a candidate that would strand a combining mark, a variation selector or a zero-width-joined emoji half is rejected during the scan and the search continues. The hard-cut fallback additionally steps off a cluster rather than splitting it.New settings
All three use the same
conf/system.propertieschannel as the existing chunker keys (-Dfess.system.<key>also works).content_chunker.length.boundary.enabledtruecontent_chunker.length.boundary.lookback_percent20content_chunker.length.boundary.lookahead_percent5Setting
boundary.enabled=false, or both percentages to0, reproduces the previous fixed-length output exactly. The effective configuration is logged once at registration time.Behaviour changes to be aware of
chunk_sizebecomes a target rather than a hard ceiling. The forward sentence search may overshoot it by up tolookahead_percent, and -- independently, and not governed bylookahead_percent-- the grapheme-cluster escape may overshoot by up to 32 UTF-16 units. The two are mutually exclusive, so the worst case reachable throughLengthChunkerischunk_size + max(lookahead, 32): 840 characters at the shipped defaults. Leave that margin against the embedding model's token limit. (ChunkBoundaryFinder#findChunkEnddocuments one further+ 1for direct API callers; that branch needsideal == start + 1, whichMIN_CHUNK_SIZEmakes unreachable from the chunker.)A document produces more chunks than before. Chunks can end up to
lookback_percentofchunk_sizeshorter -- 160 characters at the defaults. Measured: +9% on English prose, +2.7% on Japanese prose, up to +25% in the worst case. If that pushes a document pastcontent_chunker.max_chunks_per_document(default 1000) it is markedskippedand gets no embeddings at all, so raise that cap or lowerlookback_percentfor very large corpora of long documents.With
overlap > 0the effective overlap grows. The restart point is snapped to a boundary too, and snapping can only move it earlier. The snap window is capped at the configured overlap, so the effective overlap never exceeds2 * overlap.Existing indexed documents keep their stored chunk arrays; re-chunking requires a recrawl, as it already did for
chunk_size.Structure
Boundary search lives in a new
ChunkBoundaryFinder, registered as the LastaDi componentchunkBoundaryFinderinfess_chunk.xml. It holds no state (the chunk job runs it concurrently), and its character predicates areprotectedso a deployment can register a subclass that swaps only the character sets. The twoprotectedconstants (ZWJ,MAX_CLUSTER_ADJUST) are not part of that seam --staticfields have no virtual dispatch.LengthChunkerresolves the component once persplitcall, falling back to a built-in instance when it is not registered.The backward scan is a single reverse pass that carries its run state and its decoded code point across iterations, so it costs O(W) per chunk and decodes each code point once. Character classification is
intswitches only -- no collections, boxing, regex or streams on the scan path.Testing
ChunkBoundaryFinderTest: each tier, tier precedence, the ASCII-punctuation and non-digit rules, script changes, surrogate pairs, grapheme clusters on every tier path, the search windows, overlap snapping, the subclass seam, out-of-contract arguments, and a randomised property sweep asserting cluster safety, code-point alignment and lookback monotonicity.LengthChunkerTest: where the cuts actually land (not just that nothing was lost), boundary-aware splitting for Japanese and English, lossless reconstruction over a corpus including combining marks and ZWJ sequences, the overlap snap wired end to end, the effective-overlap cap, the realgetSystemPropertyconfig channel including theboundary.enabled=falsekill switch, a differential check against an independent fixed-length reference splitter, and a relative linearity guard.Known limitations (follow-ups, not addressed here)
।, Arabic؟،, Myanmar။, Khmer។and others are not recognised, so those languages never reach the STRONG tier.'and"are classified as closing marks only, so English contractions can be cut at the apostrophe (don'/t).｢｣,・and〜inside words, and bidi isolate/embedding controls are not special-cased.