Repository navigation
Compute token_count the same way everywhere - #72
Conversation
Up to standards ✅🟢 Issues
|
| 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.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: pgEdge/pgedge-vectorizer/.coderabbit.yaml Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: 2 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 3 reviews per hour. 📝 WalkthroughWalkthroughThe extension version changes from 1.1 to 1.2. The release adds the Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to This release adds a shared 🚥 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. (2 skipped: 2 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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.
The upgrade leaves existing token_count values alone, but recreate_chunks() rewrites a chunk table through the new path, so anyone who does want the stored counts consistent has a supported way to get there. Note that in both the upgrade script and the changelog, since dropping refresh_token_counts() otherwise reads as there being no way to fix existing rows.
f52d4cb to
076ebbf
Compare
|
Codacy's two high-severity Compatibility findings on this PR are both SQLint failing to parse the |
ibrarahmad
left a comment
ibrarahmad
left a comment
There was a problem hiding this comment.
Checked the upgrade path as well as a fresh install: 1.1 -> 1.2 applies, count_tokens() lands STABLE STRICT, and after the upgrade nothing in a chunk table disagrees with count_tokens(content) through the trigger path. The short-string case that used to store 0 now stores 1.
All three call sites are present in both the 1.2 and the 1.1--1.2 script, and the only length()/4 left anywhere is the one in the header comment describing the old behaviour. setup, count_tokens, chunking, queue and vectorization pass.
Nothing blocking from me.
2 participants
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.recreate_chunks()on it rewrites every row through the new path.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.