feat(indexing): configurable linesPerChunk for line-based chunking - #302
Conversation
Helweg
left a comment
Helweg
left a comment
There was a problem hiding this comment.
I found one boundary regression that needs fixing before approval:
parseConfig() accepts any finite indexing.linesPerChunk value, but the NAPI parameter is Option<u32>. Values above u32::MAX silently wrap during the native conversion. I reproduced this on the PR head with linesPerChunk: 4_294_967_296: config retains 4294967296, but parsing a two-line text file produces two one-line chunks because Rust receives 0 and clamps it to 1. That contradicts the configured window and can massively over-chunk an index.
Please clamp the public config to the NAPI range, or change the native interface to a type that safely supports every accepted value, and add a regression test for the oversized finite value.
The normal-size plumbing, defaults, and line-based behavior otherwise look sound.
…king The line-based chunking path (chunk_by_lines) hardcodes a 30-line window with a fixed 3-line overlap. This path is used for .jsonl, .txt, unknown extensions, and the AST fallback. For line-delimited files where one line is one record (e.g. Claude .jsonl session transcripts), a 30-line window yields coarse retrieval (a hit returns a 30-record window) and dilutes the embedding signal via mean-pooling over ~2500 tokens. Expose the window size as `indexing.linesPerChunk` (default 30, so existing behavior is unchanged). Thread it through the napi entry points (parse_file / parse_file_as_text / parse_files) into chunk_by_lines as an Option<u32> (undefined -> None -> 30), keeping the TS wrappers' signatures optional so all existing callers compile unchanged. Auto-cap the overlap at one quarter of the window (overlap = min(OVERLAP_LINES, lines_per_chunk / 4)) so smaller opt-in windows shrink the overlap too, preventing near-duplicate chunks. At the default 30 the overlap is min(3, 7) = 3 -- bit-identical to before. Only the line-based path is affected; AST-parsed languages (extract_chunks) are unchanged regardless of the knob. Tests: - Rust: chunk_by_lines default-unchanged, custom size (5 -> step 4, overlap 1), and size-1 (no overlap, one line per chunk). - TS: parseFile/parseFiles honor linesPerChunk on .jsonl/.txt. - Config: default 30, explicit override honored, non-number/<1 coerced.
Code review (codex + claude) found that chunk_by_lines hangs when lines_per_chunk == 0: overlap = min(3, 0) = 0 and step_size = 0, so the window loop never advances. The config layer already clamps to >= 1, but chunk_by_lines is a pub fn reachable through the napi API with an explicit 0 (native.parseFile(path, content, 0) -> Some(0) survives unwrap_or(30)), and a direct caller has no protection. Codex reproduced the hang with a bounded probe. Clamp at the layer that owns the invariant: lines_per_chunk.max(1) at the top of chunk_by_lines. A window of 0 now behaves like 1 (one line per chunk) instead of hanging. This also covers the AST paths that forward the value straight through. The config coercion let NaN and Infinity through the typeof "number" gate (Math.floor(NaN) = NaN, Math.floor(Infinity) = Infinity); at the napi boundary V8 ToUint32 maps both to 0, feeding the same hang. Tighten the guard to also require Number.isFinite. JSON cannot express these, so file configs were already safe; this protects programmatic parseConfig callers. Tests: - Rust: test_chunk_by_lines_zero_clamped_to_one (0 -> 3 one-line chunks, terminates). - Config: NaN and Infinity fall back to the default 30.
The napi fallback value 30 was repeated in three unwrap_or(30) call sites (parse_file, parse_file_as_text, parse_files). Extract DEFAULT_LINES_PER_CHUNK so the Rust fallback has one source, and add bidirectional sync comments linking it to the TS default in src/config/defaults.ts. The cross-language pair cannot share a literal; the comments are the sync mechanism. No behavior change (30 -> 30).
f43dd1a to
5650386
Compare
parseConfig() accepted any finite linesPerChunk >= 1, but the NAPI parameter is Option<u32>. Values above u32::MAX (4294967295) silently wrapped to 0 during the native conversion, and chunk_by_lines treated the 0 as 1, so an oversized configured window over-chunked a file: a 2-line text file with linesPerChunk = 4294967296 produced two 1-line chunks instead of one 2-line chunk. Clamp the public config to the NAPI u32 range (1..=4294967295) so the native layer never receives a wrapped value. Add a regression test for the oversized finite value.
Helweg
left a comment
Helweg
left a comment
There was a problem hiding this comment.
Re-reviewed the current head . The public config now clamps to the NAPI maximum, preventing overflow. Focused tests passed (177 config/native tests), and I exercised the real TypeScript config-to-native parser boundary with : it clamps to and preserves a two-line text file as one chunk. Approved pending the required GitHub-hosted workflows.
|
Clarification to my approval: I reviewed head e05f2d9. The public config now clamps linesPerChunk to the NAPI u32 maximum, preventing overflow. Focused validation passed: 177 config/native tests, plus a real TypeScript config-to-native parser check with 4294967296. It clamps to 4294967295 and preserves a two-line text file as one chunk. Approval remains conditional on the required GitHub-hosted workflows. |
The linesPerChunk PR threaded `lines_per_chunk: Option<u32>` through the napi entry points (parse_file, parse_file_as_text, parse_files) and the parser internals, producing single-line call sites that exceed rustfmt's width limit. CI's `cargo fmt --check` flagged six unformatted sites: native/src/lib.rs:35, 45, 53 (napi entry-point forwarding calls) native/src/parser.rs:117, 145 (chunk_by_lines + internal signatures) native/src/parser.rs:1931 (test_parse_cpp_preserves_small_type_symbols) Run `cargo fmt` to wrap them. Behavior unchanged; 128 native tests pass.
|
uploaded the fmt fix - should unblock workflow. |
2 participants
Summary
The line-based chunking path (
chunk_by_lines) hardcodes a 30-line window with a fixed 3-line overlap. This path handles.jsonl,.txt, unknown extensions, and the AST fallback. For line-delimited files where one line is one record (e.g. Claude.jsonlsession transcripts), a 30-line window yields coarse retrieval (a hit returns a 30-record window, not one record) anddilutes the embedding signal via mean-pooling over ~2,500 tokens.
This PR exposes the window size as
indexing.linesPerChunk(default30= no behavior change) so users can opt into finer-grained chunks for line-based files.Changes
src/config/schema.ts,src/config/defaults.ts): addindexing.linesPerChunk(default30; coerced withtypeof === "number" && Number.isFinite, clamped>= 1, floored). JSDocdocs/configuration.mdtable row.native/src/lib.rs):parse_file/parse_file_as_text/parse_filestakelines_per_chunk: Option<u32>(undefined->None->DEFAULT_LINES_PER_CHUNK). Back-compatiblenapi signature; the default is a single named const with a sync comment to the TS default.
native/src/parser.rs): threadlines_per_chunk: usizethroughparse_file_internal,parse_file_as_text_internal,parse_files_parallel,parse_file_with_symbols_internal,extract_chunks, andchunk_by_lines. Auto-cap the overlap at one quarter of the window:overlap = min(OVERLAP_LINES, lines_per_chunk / 4). At default30, overlap ismin(3, 7) = 3—bit-identical to before. Smaller opt-in windows shrink the overlap so chunks don't collapse into near-duplicates.
chunk_by_linesalso clampslines_per_chunk >= 1(defense in depth — seeReview fixes).
src/native/parsing.ts): optionallinesPerChunk?: numberparam onparseFile/parseFileAsText/parseFiles; forwarded to native. Existing 2-arg callers (tests, etc.)keep working.
src/indexer/index.ts): passthis.config.indexing.linesPerChunkat the two parse call sites (batch parse + text fallback).Scope
Only the line-based path is affected. AST-parsed languages (
extract_chunks) are unchanged regardless of the knob. No version bump, no publish.Tests
native/src/parser.rs):test_chunk_by_lines_default_unchanged(30 -> 30-line chunks, step 27),test_chunk_by_lines_custom_size(5 -> step 4, overlap 1),test_chunk_by_lines_size_one_no_overlap(1 -> one line per chunk, no overlap),test_chunk_by_lines_zero_clamped_to_one(0 -> terminates, behaves like 1). Existingtest_chunk_overlapupdatedfor the new signature.
cargo test: 128 passed.tests/native.test.ts):parseFile/parseFileshonorlinesPerChunkon.jsonl/.txt.tests/config.test.ts): default30, explicit override honored, non-number/<1/NaN/Infinitycoerced to default or clamped.npm run typecheck,npm run lint,npm run build:ts,npm run build:native: clean.npx vitest runaffected suites: 184 passed.Complementarity with #300
Smaller chunks raise chunk count, which is affordable at scale only with ollama request batching (#300). The two PRs are independent but complementary: batching makes fine-grained
linesPerChunkpractical for large line-delimited corpora.