Repository navigation
fix: preserve markdown table rows when chunking - #1074
SomSamantray wants to merge 2 commits into
Conversation
S3yon
left a comment
S3yon
left a comment
There was a problem hiding this comment.
Ran it locally: the store tests pass on Node (330 pass) and tsc is clean. On Bun one store test fails ("legacy fingerprint adoption ... 16 MiB SQLite budget"), and it fails the same way on 86d2365, so it isn't from this PR.
One problem in the path that indexes documents. The embed path goes through chunkDocumentByTokensWithLlm, which re-splits any chunk over the token limit and rebuilds each piece with text.slice(subChunk.pos, subChunk.pos + subChunk.text.length) (src/store.ts:4028). That assumes chunk.text is the source span starting at pos. With the header prefixed it isn't, so the slice drops the header and runs header-length characters past the cut.
Tables hit that path a lot because they're token-dense. A 300-row numeric table (| p0 | 0 | 13 | 0 | ms | and so on) through chunkDocumentByTokens with the defaults gave 15 chunks: 10 of the 14 continuation chunks had no header, and one ended in a cut row (| p67). Through chunkDocument alone the same table comes out right, which is what the new tests cover.
A possible fix: push subChunk.text as-is instead of re-slicing, and add a table test on the token path. The "Token-based Chunking" block is skipped in CI, so it may need a small fake tokenizer.
Smaller one: a table that fits the 1.5x allowance, but starts more than a window before the target and ends past charPos + maxChars * 1.5, is cut once and then emitted whole in the next chunk. chunkDocument("x".repeat(150) + "\n\n" + <12-row table> + ..., 400, 60, 100) gives a chunk with rows 0 to 5, then one with all 12. Not wrong, just duplicated.
Markdown table chunks on the token-based embedding path lost their header and cut rows, and chunk positions drifted from the document text. - Re-split pieces are taken from their own text. Re-slicing the parent by length dropped a continued table's header and ran past the piece's end. - Each chunk reports its header prefix length, and the token path maps positions through it, so pos is the source offset of the chunk's raw text on both paths. - A chunk end that lands inside a table which fits the budget stops just before the table, so the overlap rewind no longer emits its leading rows twice. The rewind also skips a table the chunk already holds whole, and a chunk that would hold only overlap starts at the table instead. - A chunk end inside an oversized table's header extends to its first row, so no chunk holds a header alone. - Each recursion step must strictly shrink every piece, since a continued table re-adds its header to each piece. Otherwise the recursion can stop making progress. - A piece whose source span lies inside the previous piece's is dropped, so a row that nearly fills the budget is not emitted twice. - The closing fence of a code block ends just past its marker when the fence is indented. Refs tobi#955
|
Thanks for the review, @S3yon. Both points are fixed in 05f6568.
Also in this commit: each recursion step must strictly shrink its pieces (a continued table re-adds its header to each piece), so the token recursion always terminates, and an indented closing code fence now ends just past its marker. Tests: a token-path table test with a small fake tokenizer that runs in CI (header on every chunk, no cut rows, positions match the source), duplication regression tests on both paths, a termination test, a header-only test, and the fence-end test. Node 22 vitest suite and the Bun store tests pass, and |
S3yon
left a comment
S3yon
left a comment
There was a problem hiding this comment.
Re-ran it at 05f6568. The store tests pass on Node (350) and tsc is clean. On Bun the only failure is the same 16 MiB budget test that fails on 86d2365.
The token path is fixed. The 300-row numeric table through chunkDocumentByTokens with the real tokenizer and the defaults now gives 10 chunks: every continued chunk starts with the header, no row is cut, all 300 rows are there, and pos lines up with the source text after the header. Same with prose around the table, and at a 120-token budget where the pieces get re-split more than once.
The duplication repro now puts each of the 12 rows in exactly one chunk.
Looks good to me. Thanks for the extra tests.
Large Markdown tables now produce chunks with complete rows and the column header in every continuation. Tables that fit the chunk budget stay together, and fenced code examples containing pipe characters remain code.
Fixes #955.
Tests
AI disclosure: Developed with OpenAI Codex assistance.