Compute token_count the same way everywhere - #72
Conversation
The chunking code in C has always sized chunks with a four-characters-per-token estimate that rounds up, whilst the three plpgsql paths that actually write the token_count column open-coded the same estimate as length(chunk_text) / 4, which truncates. The two therefore disagreed by a token on most chunks, and on anything shorter than four characters the plpgsql paths stored a zero that the BM25 scoring path in worker.c then had to clamp back up to one. Since token_count feeds the BM25 document-length normalisation, by way of AVG(token_count) in bm25.c and the per-chunk value in worker.c, hybrid search scored chunks written by the trigger slightly differently from chunks written by the C chunker. Expose the existing C counter as pgedge_vectorizer.count_tokens(text) and call it from enable_vectorization(), vectorization_trigger() and recreate_chunks(), so that there is one definition of the rule rather than two. It is declared STABLE rather than IMMUTABLE deliberately: the estimate is defined in terms of pgedge_vectorizer.model, which does not matter whilst the counter ignores the model, but would quietly invalidate an expression index or a cached plan the moment it stops doing so. Existing chunk tables are left alone. The stored values are an approximation either way, and rewriting every chunk table to correct a single token is not a trade worth making on upgrade. This is the first change for 1.2, so the extension version moves on and sql/pgedge_vectorizer--1.1--1.2.sql carries the upgrade; the 1.1 scripts are untouched.
Up to standards ✅🟢 Issues
|
| Category | Results |
|---|---|
| Compatibility | 2 high (1 false positive) |
🟢 Metrics 0 complexity · 0 duplication
Metric Results Complexity 0 Duplication 0
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Essentials Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
Included review availability: 3 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. 📝 WalkthroughWalkthroughThe extension version changes from 1.1 to 1.2. The release adds the Priority: ➖ Normal Severity of issue fixed: Low Merge Risk: ⚪ Minimal · up to The token-counting paths are consistent and covered by passing regression tests; no merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. (7 skipped: 7 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
1 participant
The C chunking code sizes chunks with a four-characters-per-token estimate that rounds up. The three plpgsql paths that actually write the
token_countcolumn open-coded the same estimate aslength(chunk_text) / 4, which truncates, so the two disagreed by a token on most chunks and stored a zero for anything shorter than four characters.That matters because
token_countis not decorative:bm25.cusesAVG(token_count)as the average document length andworker.creads the per-chunk value, both feeding the BM25 length normalisation, so hybrid search scored chunks written by the trigger slightly differently from chunks written by the C chunker.worker.cwas already clamping the stored zeroes back up to one.pgedge_vectorizer.count_tokens(text), and call it fromenable_vectorization(),vectorization_trigger()andrecreate_chunks(), so there is one definition of the rule rather than two.STABLE, notIMMUTABLE. The estimate is defined in terms ofpgedge_vectorizer.model, which does not matter whilst the counter ignores the model, but would quietly invalidate an expression index or a cached plan the moment it stops doing so.sql/pgedge_vectorizer--1.1--1.2.sqlcarries the upgrade and the 1.1 scripts are untouched.New
count_tokensregression test covers the estimate itself (rounding, empty, NULL, multi-byte, the declared volatility) and, more to the point, asserts that nothing in a chunk table disagrees withcount_tokens(content)after each of the three write paths.Test run: 21 pg_regress tests and 69 TAP tests pass against PostgreSQL 18.4.
The
count_tokens()part of this originates in #22, from @syedkazim110, which this replaces.refresh_token_counts()from that PR is not carried over: nothing writes chunk rows outside the paths above, so there is nothing left for it to back-fill.