feat: [#4687] native BM25 full-text scoring (field boosts, caret, EXPLAIN/PROFILE) - #4695
Conversation
…LAIN/PROFILE)
Add Okapi BM25 ranking to FULL_TEXT indexes. BM25 (TF/IDF + document-length
normalization) becomes the default similarity for newly created full-text
indexes; existing indexes keep the legacy term-coordination (CLASSIC) scoring,
which stays available via METADATA {"similarity":"CLASSIC"}.
- Per-posting term frequency + document length are stored inline through a RID
subclass (FullTextPostingRID), gated by a storeTermFrequency flag so every
non-full-text LSM index keeps the byte-identical RID-only format.
- Configurable k1/b, per-field boosts (metadata) and Lucene-style caret boosts
(e.g. title:java^3), combinable inside AND/OR/NOT and grouped queries. The
effective weight is caret * field_boost.
- $score exposes the float BM25 relevance on every matching row. EXPLAIN/PROFILE
annotate the full-text fetch step with the similarity, k1/b, corpus stats
(N, avgdl) and each query term's df/idf/boost.
- Corpus counters (N, sum of document lengths) and analyzer configuration now
persist across restart; previously toJSON dropped the metadata and silently
reverted custom analyzers to StandardAnalyzer.
Fix (pre-existing, also affected CLASSIC): full-text index compaction dropped
postings when a single token's value list spanned multiple compacted pages. The
compacted root is a positional sparse index that cannot index one leaf page
under two keys, so a key's values left on a shared continuation page became
unreachable. Overflowing keys now start on a fresh page they fully own.
|
Tick the box to add this pull request to the merge queue (same as
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 9 |
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.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements native Okapi BM25 full-text scoring, allowing newly created full-text indexes to rank results using term frequency, inverse document frequency, and document-length normalization. Key changes include the addition of a pure BM25Scorer utility, the persistence of per-posting statistics via FullTextPostingRID, incremental maintenance of corpus statistics, and support for query-time caret boosts and per-field boosts. Feedback on the changes highlights critical resource management and concurrency issues: IndexCursor and db.iterateType() iterators should be closed to prevent resource leaks, ensureCounters() requires synchronization to avoid concurrent table scans on startup, and collectPhraseMatches should correctly apply field-specific boosts instead of hardcoding a default boost.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| final IndexCursor postings = underlyingIndex.get(new String[] { storedKey }); | ||
|
|
||
| // Collect the postings first so the document frequency (and therefore the IDF) is known before scoring. | ||
| final List<FullTextPostingRID> termPostings = new ArrayList<>(); | ||
| while (postings.hasNext()) { | ||
| final Identifiable id = postings.next(); | ||
| if (id instanceof FullTextPostingRID s) | ||
| termPostings.add(s); | ||
| } | ||
|
|
||
| final long df = termPostings.size(); | ||
| if (df == 0) | ||
| continue; | ||
|
|
||
| final double idf = BM25Scorer.idf(totalDocs, df); | ||
| for (final FullTextPostingRID s : termPostings) { | ||
| final double contribution = BM25Scorer.termScore(idf, s.tf, s.docLength, avgdl, k1, b) * boost; | ||
| scoreMap.merge(s.getIdentity(), (float) contribution, Float::sum); | ||
| } |
There was a problem hiding this comment.
The IndexCursor returned by underlyingIndex.get() should be closed to release page pins and locks in the storage engine. Failing to close it can lead to resource leaks and page cache exhaustion.
final IndexCursor postings = underlyingIndex.get(new String[] { storedKey });
try {
// Collect the postings first so the document frequency (and therefore the IDF) is known before scoring.
final List<FullTextPostingRID> termPostings = new ArrayList<>();
while (postings.hasNext()) {
final Identifiable id = postings.next();
if (id instanceof FullTextPostingRID s)
termPostings.add(s);
}
final long df = termPostings.size();
if (df == 0)
continue;
final double idf = BM25Scorer.idf(totalDocs, df);
for (final FullTextPostingRID s : termPostings) {
final double contribution = BM25Scorer.termScore(idf, s.tf, s.docLength, avgdl, k1, b) * boost;
scoreMap.merge(s.getIdentity(), (float) contribution, Float::sum);
}
} finally {
postings.close();
}| private void ensureCounters() { | ||
| if (ftMetadata == null) | ||
| return; | ||
| if (ftMetadata.getTotalDocs() > 0) | ||
| return; | ||
| final String typeName = getTypeName(); | ||
| if (typeName == null) | ||
| return; | ||
| final DatabaseInternal db = underlyingIndex.getMutableIndex().getDatabase(); | ||
| if (db.countType(typeName, false) <= 0) | ||
| return; // empty type: the (0,0) counters are correct | ||
| recomputeBM25Counters(); | ||
| } |
There was a problem hiding this comment.
The ensureCounters() method is called on every query and is not synchronized. If multiple queries run concurrently on a freshly opened database where counters are not yet valid, they will all concurrently trigger recomputeBM25Counters(), causing concurrent full-table scans and concurrent configuration file writes. Double-checked locking should be used to prevent this concurrency hazard without hurting read performance.
| private void ensureCounters() { | |
| if (ftMetadata == null) | |
| return; | |
| if (ftMetadata.getTotalDocs() > 0) | |
| return; | |
| final String typeName = getTypeName(); | |
| if (typeName == null) | |
| return; | |
| final DatabaseInternal db = underlyingIndex.getMutableIndex().getDatabase(); | |
| if (db.countType(typeName, false) <= 0) | |
| return; // empty type: the (0,0) counters are correct | |
| recomputeBM25Counters(); | |
| } | |
| private void ensureCounters() { | |
| if (ftMetadata == null) | |
| return; | |
| if (ftMetadata.getTotalDocs() > 0) | |
| return; | |
| final String typeName = getTypeName(); | |
| if (typeName == null) | |
| return; | |
| final DatabaseInternal db = underlyingIndex.getMutableIndex().getDatabase(); | |
| if (db.countType(typeName, false) <= 0) | |
| return; // empty type: the (0,0) counters are correct | |
| synchronized (this) { | |
| if (ftMetadata.getTotalDocs() > 0) | |
| return; | |
| recomputeBM25Counters(); | |
| } | |
| } |
| final Iterator<com.arcadedb.database.Record> it = db.iterateType(typeName, true); | ||
| while (it.hasNext()) { | ||
| final com.arcadedb.database.Record record = it.next(); | ||
| if (!(record instanceof Document doc)) | ||
| continue; | ||
| int len = 0; | ||
| for (final String p : props) { | ||
| final Object v = doc.get(p); | ||
| if (v != null) | ||
| len += analyzeText(indexAnalyzer, new Object[] { v }).size(); | ||
| } | ||
| ++docs; | ||
| sumLen += len; | ||
| } |
There was a problem hiding this comment.
The iterator returned by db.iterateType() may be a ResultSet or Cursor that implements AutoCloseable. It should be closed in a finally block to prevent resource leaks.
final Iterator<com.arcadedb.database.Record> it = db.iterateType(typeName, true);
try {
while (it.hasNext()) {
final com.arcadedb.database.Record record = it.next();
if (!(record instanceof Document doc))
continue;
int len = 0;
for (final String p : props) {
final Object v = doc.get(p);
if (v != null)
len += analyzeText(indexAnalyzer, new Object[] { v }).size();
}
++docs;
sumLen += len;
}
} finally {
if (it instanceof AutoCloseable closeable) {
try {
closeable.close();
} catch (Exception e) {
// ignore
}
}
}| for (final Term term : terms) { | ||
| recordScoringToken(term.text(), 1.0f); |
There was a problem hiding this comment.
In collectPhraseMatches, the scoring token is recorded using the unqualified term text and a hardcoded 1.0f boost. This ignores the field prefix and the configured field boost for phrase queries on specific fields (e.g., title:"java programming"). It should use buildSearchKey and boostFor to correctly apply field-specific scoring.
| for (final Term term : terms) { | |
| recordScoringToken(term.text(), 1.0f); | |
| for (final Term term : terms) { | |
| final String field = term.field(); | |
| recordScoringToken(buildSearchKey(field, term.text()), boostFor(field)); |
Code Review: feat/bm25-fulltext-scoringSolid feature addition with well-structured code and good test coverage. A few correctness and thread-safety concerns worth addressing before merge. Correctness Bugs1. Per-bucket In both Similarly, 2. final Map<RID, AtomicInteger> scoreMap = new HashMap<>();
collectMatches(query, scoreMap, new HashSet<>());
return index.explainScoring(scoringTokens);
Thread-Safety3. Unsynchronized corpus counter mutation (medium severity)
public void addDocument(final long docLength) {
++totalDocs; // no synchronization
sumDocLength += docLength;
}ArcadeDB runs concurrent transactions. Two threads simultaneously indexing documents into the same type will race on Compaction Fix4. Partial write to continuation page - orphaned bytes with no comment When The fix itself looks correct and the compaction test provides good coverage. Minor Issues5.
6. The method threads from Strengths
|
…lain - BM25 is scored per bucket (per-shard, like Elasticsearch): the document frequency is read from a single bucket's postings, so N and avgdl must be per-bucket too. The fallback (was countType, type-wide) and recompute (was iterateType, whole type) now use the associated bucket's count/records, removing the systematic IDF bias when a type has multiple buckets. Documented the per-bucket scoring model. - EXPLAIN/PROFILE scoring metadata: collect scoring tokens in a tokens-only mode that skips per-document score accumulation, so no document set is materialized just to read the term/idf/df breakdown. - test: BM25 ranking across multiple buckets.
|
Thanks for the review - both addressed in 1. df vs N mismatch. Fixed (not just documented). BM25 is scored per bucket (per-shard, like Elasticsearch), so
2. Full-text suite (137) + SQL full-text function suite green. |
Code Review - PR #4695: Native BM25 Full-Text ScoringOverviewThis is a substantial, well-structured addition. The design correctly separates concerns (pure math in The review below focuses on things that could bite in production. Potential Bugs1.
if (ftMetadata.getTotalDocs() > 0)
return; // skips recompute even when countersValid == falseIf a BM25 index is re-opened with persisted counters that are stale (e.g. written mid-batch, off by some documents), if (ftMetadata.getTotalDocs() > 0 && ftMetadata.isCountersValid())
return;2. Documentation contradicts the code on the compaction bug
3. private float boostFor(final String field) {
if (field != null && !field.isEmpty() && !"content".equals(field) && metadata != null)
return metadata.getFieldBoost(field);
return 1.0f;
}The Thread Safety4.
Memory / Performance5. All postings for every query term loaded into memory Both final List<FullTextPostingRID> termPostings = new ArrayList<>();
while (postings.hasNext()) {
final Identifiable id = postings.next();
if (id instanceof FullTextPostingRID s)
termPostings.add(s);
}
final long df = termPostings.size();For a common term in a large collection (say, "the" in millions of documents) this can exhaust the heap. A two-pass or streaming approach (first scan to count 6. This is called from Design / Clarity7. The int 8. Two independent BM25 scoring paths that must stay in sync
9. for (final Index bucketIndex : typeIndex.getIndexesOnBuckets())
if (bucketIndex instanceof final LSMTreeFullTextIndex ftIndex)
return new FullTextQueryExecutor(ftIndex).explainScoring(queryString);For a multi-bucket type the EXPLAIN output only reflects one shard's statistics. This should at least be noted as a comment so users don't mistake per-bucket IDF for global IDF. Minor10. No validation of BM25 parameters
11. Inline comment uses The Positives
|
- Make the BM25 corpus counters thread-safe: totalDocs/sumDocLength are now AtomicLong (countersValid volatile). Concurrent transactions indexing into the same bucket share the per-bucket metadata and were racing on bare long fields. - Clarify in the compaction fix that no bytes are orphaned on the continuation page: the key's entry lives only in the scratch buffer until putByteArray, so the continuation page is flushed with just the previous keys' entries. - Comment the EXPLAIN/PROFILE delegation chain at the FetchFromIndexedFunctionStep call site. - Docs: note the $score type change (now Float, incl. CLASSIC where it was an Integer match count) as the one user-visible change for existing indexes; fold the compaction issue into a "fixed" section.
|
Addressed in 3. Unsynchronized corpus counters. Fixed. 4. Continuation-page "orphaned bytes". Good catch to flag, but there are actually no orphaned bytes - the page switch happens before the commit. The key's serialized entry lives only in the scratch buffer ( 5. 6. Delegation-chain comment. Added at the Full-text + SQL full-text suites green (182). |
Code Review: feat/bm25-fulltext-scoringThis is a substantial, well-conceived feature addition. The BM25 implementation is clearly structured, the storage design (carrying tf/docLength inline via Correctness1. In Integer score = (Integer) result.getProperty("$score");will get a 2. In 3. BM25 parameter bounds not validated
final double norm = 1.0 - b + b * (docLen / safeAvgdl);
return idf * (tf * (k1 + 1.0)) / (tf + k1 * norm);With if (k1 <= 0) throw new IllegalArgumentException("bm25_k1 must be > 0, got: " + k1);
if (b < 0 || b > 1) throw new IllegalArgumentException("bm25_b must be in [0,1], got: " + b);These should also be validated in the SQL 4. Corpus counters can drift on transaction rollback
5. Silent precision loss in // IndexCursorEntry.java
this.score = Math.round(floatScore); // e.g. 2.73 -> 3
this.floatScore = floatScore; // 2.73 preserved hereThe legacy Performance6. Double iteration over posting list in In long df = 0;
while (postings.hasNext()) {
if (postings.next() instanceof FullTextPostingRID s) {
df++;
// update docAccumulator inline
}
}
// apply IDF using df after the passThis halves allocations for high-cardinality postings. Thread Safety7. In Testing8. Missing tests for error/edge cases The test suite is strong for the happy path but is missing:
Minor / Nits9. Inconsistent counter validity predicates
10. // FullTextQueryExecutor.java
if (field != null && !field.isEmpty() && !"content".equals(field) && metadata != null)
return metadata.getFieldBoost(field);The string 11. Docs file in wrong location
Summary
Overall this is high-quality work - the BM25 math is correct, the backward-compatibility gating via Reviewed by Claude (Sonnet 4.6) |
…er, validation - IDF consistency (multi-bucket): the FullTextIndexMetadata (and its corpus counters) is shared across a type's bucket indexes, so the counters are type-wide. Since document frequency is read per bucket, N for IDF now comes from the bucket's live record count (matching df scope); the shared counters feed only the average document length. Recompute scans the whole type (not a single bucket, which would corrupt the shared counters). - Unify the two BM25 scoring paths (direct get() and Lucene-syntax executor) into one streaming computeBM25Scores helper: df is stream-counted without materializing the posting list, and only candidate/result postings are held - bounding memory for high-frequency terms and removing duplicated formula code. - ensureCounters() now keys off countersValid (not totalDocs > 0), so a fresh empty index is not rescanned on every query; lazy recompute is in-memory only (no saveConfiguration on the read path). - Validate BM25 params (k1 >= 0, b in [0,1]) in the setters and the METADATA path; reject misconfiguration at index creation. - Centralize the QueryParser default-field sentinel as DEFAULT_FIELD with the collision limitation documented. - Per-field maps -> ConcurrentHashMap (read on the query path, iterated by writeToJSON). Document corpus-counter drift on rollback / analyzer change. - IndexCursorEntry.score: document the BM25 rounding/precision loss (use floatScore). Comment that EXPLAIN reports one representative bucket's stats. - tests: BM25 param validation, document removal + recompute (incl. empty type).
|
Addressed in 1. 2. 3. BM25 param bounds. Validated in 4. Counter drift on rollback. Documented on the counter mutators (see #2). 5. 6. Double iteration / allocation. Both BM25 paths now share one streaming 7. 8. Edge-case tests. Added: invalid params; document removal + 9. 10. 11. Docs location. One design note re #1 (multi-bucket): the IDF fix found that the metadata - and thus the corpus counters - is shared type-wide across a type's bucket indexes, so |
Code Review: feat/bm25-fulltext-scoringOverviewThis is a substantial and well-designed PR that adds native Okapi BM25 ranking to FULL_TEXT indexes. The implementation is layered cleanly: math isolated in BM25Scorer, posting statistics carried inline via FullTextPostingRID, serialization gated by a storeTermFrequency flag, and the corpus counters maintained in FullTextIndexMetadata. The compaction fix (dropped postings when a token's list spans multiple pages) is an important correctness fix that happens to land alongside BM25 but is independent. Positives
Issues and Suggestions1. Two-pass posting scan doubles I/O (Performance)computeBM25Scores in LSMTreeFullTextIndex opens each token's posting cursor twice: once to count df and compute idf, then again to accumulate per-document scores. For a common term (e.g. "data" in the compaction test with 600 postings), this is 1200 cursor reads per term per query. The trade-off - avoiding materializing the whole posting list - is documented. A low-effort improvement: if a df count is accessible through the LSM layer without a full scan (the compacted index already has page-level counts in its root), the first pass could be avoided. Even a 2. Corpus counter drift on rollback (Correctness concern, documented)addDocument/removeDocument are called at put/remove time, BEFORE the transaction commits, and are not reversed on rollback. The PR documents this clearly. The drift affects only avgdl (a BM25 length normalizer), not IDF, so ranking degrades gradually rather than catastrophically. However, there is currently no automated periodic recompute - recomputeBM25Counters() is public and called in tests but not wired into any background job. Consider scheduling a periodic recompute (similar to MaterializedViewScheduler) or flagging countersValid = false on rollback so the lazy ensureCounters() path kicks in on the next query. 3. $score type change is a breaking API change (User-visible)Changing $score from Integer to Float (even for CLASSIC indexes) is a breaking change for application code that reads $score via result.getProperty("$score", Integer.class) or casts it directly. The PR docs mention it, but it should be explicitly called out in the release notes. The test classicSimilarityKeepsCoordinationScoring validates the float values but does not pin the returned type - worth adding a check that the returned type is Float, not Integer, so the change is explicitly tested. 4. setSimilarity does not validate unknown similarity namespublic void setSimilarity(final String similarity) {
this.similarity = similarity != null ? similarity.toUpperCase() : SIMILARITY_BM25;
}An index created with METADATA {"similarity": "LUCENE"} would silently behave as CLASSIC (isBM25() returns false). Consider adding validation: if (!SIMILARITY_BM25.equals(upper) && !SIMILARITY_CLASSIC.equals(upper))
throw new IllegalArgumentException("Unknown similarity: " + similarity + ". Valid values: BM25, CLASSIC");5. No test for CLASSIC compaction fixThe compaction bug fix in LSMTreeIndexCompacted is described as independent of BM25 - it affected CLASSIC too. FullTextBM25CompactionTest covers it for BM25 but there is no test that verifies CLASSIC postings survive compaction after the fix. A regression test for CLASSIC would close the coverage gap and document the historical break. 6. EXPLAIN reports only the first bucket (Documentation gap)getScoringExplain in SQLFunctionSearchIndex iterates getIndexesOnBuckets() and returns the first full-text bucket's statistics. For a type with many buckets, IDF can differ significantly between them (per the resolveTotalDocs design). Nothing in the output itself makes this sampling clear. Suggest adding a "note" field in the explain JSON (e.g. "stats from one bucket; BM25 is scored per bucket") so users are not surprised by mismatches between the explained IDF and actual scores. 7. countDocuments multiplies by rids.length (Potential over-count)countDocuments(rids.length, docLen);put accepts a RID[] array. For a full-text index the common case is one RID per indexed document, but if multiple RIDs are ever passed the counter over-counts by rids.length. It is worth asserting (or at least documenting) that rids.length == 1 is expected for the full-text BM25 path, or handling the multi-RID case explicitly. 8. Missing @tag on some FullTextBM25Test methodsbm25RankingHoldsAcrossMultipleBuckets inserts 41 documents across 4 buckets, and bm25ConfigAndCountersSurviveRestart inserts 13 documents and reopens the database. Per the project's convention, tests with non-trivial setup/data volumes should be tagged @tag("slow") at the method level. 9. Minor: fully-qualified return type in explainScoringpublic com.arcadedb.serializer.json.JSONObject explainScoring(final String queryString) {The project convention is to import classes and use short names. JSONObject is already imported in other files in this package. SummaryThe implementation is solid and the design choices (two-pass scan, per-bucket IDF, lazy counter recompute) are well-reasoned and documented. The highest-priority items before merge are:
Items 2 and 6 (counter drift on rollback, EXPLAIN multi-bucket note) are acceptable with the current documentation but worth addressing in a follow-up. |
…ty validation, tests - computeBM25Scores: when a candidate set is given (the SEARCH_INDEX path) score in a SINGLE pass, counting df while collecting only candidate postings - halves cursor I/O for high-frequency terms vs the previous two-pass. The no-candidate path stays two-pass to keep memory bounded. Added a TODO to source df from a per-key index count if the LSM layer ever exposes one. - setSimilarity rejects unknown names (e.g. "LUCENE") instead of silently falling back to CLASSIC; the METADATA path routes through it. - EXPLAIN scoring JSON carries a "note" stating the statistics are per-bucket, so the explained IDF is not mistaken for a global value. - countDocuments: document that rids.length is 1 per document on the standard path. - Docs: corpus-counter drift / no-background-recompute behavior; $score is Float. - Use the imported JSONObject short name in explainScoring. - tests: CLASSIC postings survive compaction (independent regression); $score type pinned to Float for CLASSIC; unknown-similarity rejection; @tag("slow") on the multi-bucket and restart tests.
|
Addressed in 1. Two-pass scan. The common SEARCH_INDEX path (candidate set known) is now single-pass: it counts 2. Counter drift / no periodic recompute. Documented explicitly (counters feed only 3. 4. Unknown similarity. 5. CLASSIC compaction. Added 6. EXPLAIN per-bucket. The scoring JSON now includes a 7. 8. 9. Fully-qualified type. Full-text + function suites green (185). |
|
Code review posted - see below for full analysis |
Code Review: Native BM25 full-text scoringThis is a substantial, well-engineered feature. The BM25 math is isolated and unit-tested, and backward compatibility is carefully preserved. Correctness concernsCorpus counters not reversed on rollback: addDocument/removeDocument fire before the transaction commits. A failed transaction permanently inflates totalDocs/sumDocLength with no automatic repair - only manual recomputeBM25Counters() fixes it. Consider: (1) annotating countDocuments() with the rollback-drift risk, and (2) adding a test that rolls back and verifies the counter repair. |
|
Code Review: Native BM25 full-text scoring This is a substantial, well-engineered feature. The overall design is sound, the BM25 math is isolated and unit-tested, and backward compatibility is carefully preserved. CORRECTNESS CONCERNS Corpus counters not reversed on rollback: addDocument/removeDocument fire before the transaction commits. A failed insert that rolls back permanently inflates totalDocs/sumDocLength with no automatic repair - only manual recomputeBM25Counters() fixes it. Consider: (1) annotating countDocuments() with the rollback-drift risk; (2) adding a test that rolls back and verifies counter repair. Double scan of posting list for unbounded BM25 queries: In computeBM25Scores with candidates == null, each token is iterated TWICE - once for df, once for scoring. For common terms with large posting lists this doubles read I/O. The TODO is correct; if deferred, document the expected impact. associatedBucket() swallows all exceptions: Catching Exception and returning null hides legitimate failures. resolveTotalDocs() falls back to 1, silently biasing IDF. Tighten to SchemaException or at minimum log at WARNING. DESIGN / API CONCERNS Breaking change - score type widened from Integer to Float for CLASSIC indexes: Runtime break for code casting to Integer. The classicSimilarityKeepsCoordinationScoring test pins the new type correctly. Consider noting this prominently in release notes. DEFAULT_FIELD collision on multi-property indexes: A property literally named "content" silently loses field boosts. Consider renaming the sentinel (e.g., default) or logging a WARNING at creation time. Mutable instance fields in FullTextQueryExecutor: scoringTokens, collectingExclusion, tokensOnly, currentBoost are not reset between calls. The one-executor-per-search invariant is not enforced. Resetting at the top of search() and explainScoring() would prevent silent corruption if an executor were reused. PERFORMANCE NOTES computeCorpusCounters scans entire type on first query: Cold-start lazy recompute can be slow on large collections. Consider persisting countersValid = true eagerly at schema save, or logging the recompute at INFO. explainScoring iterates postings for every query term: Surprising cost for large indexes. A warning comment would help. MINOR NITS Deletion markers wrapped in FullTextPostingRID: In readEntryValue, markers with negative bucket IDs are wrapped with tf=0/docLength=0. Future instanceof checks could misinterpret them. A rid.getBucketId() >= 0 guard before constructing FullTextPostingRID would make intent explicit. Fragile hasSize(601) in compaction test: Fragile to tokenization/bucket changes. Prefer isGreaterThanOrEqualTo(600) plus containsKey("rare"). WHAT IS DONE WELL
Overall this is ready to merge after the correctness concerns are addressed (or explicitly accepted as known trade-offs with follow-up issues filed). Review by Claude Sonnet 4.6 |
…ft test - readEntryValue: keep deletion markers (negative bucket id) as a plain RID instead of wrapping them in FullTextPostingRID, so nothing mistakes a marker for a scorable posting (the tf/docLength varints are still read to stay aligned). - FullTextQueryExecutor: reset per-query matching state (scoringTokens, collectingExclusion, tokensOnly, currentBoost) at the top of search() and explainScoring() - defensive against executor reuse. - associatedBucket(): log a WARNING instead of silently returning null (IDF then falls back to N=1); log the cold-start corpus recompute at INFO (it full-scans the type). - countDocuments: document the rollback-drift risk (counters feed only avgdl; recomputeBM25Counters repairs). - EXPLAIN explainScoring: note the per-term posting scan cost. - docs: fix stale RIDWithStats -> FullTextPostingRID; document the single-pass (candidate) vs two-pass (unbounded) scoring and the bounded-memory trade-off. - tests: rolled-back insert is not indexed and recompute repairs counters; relaxed the fragile hasSize(601/600) checks to >= + containsKey.
|
Addressed in Correctness
Design / API
Performance
Nits
Full-text + function suites green (186); non-full-text LSM smoke (72) confirms the gated |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 9 |
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.
Code Review: Native BM25 Full-Text Scoring (#4695)This is a substantial and well-engineered PR. The design is solid, the documentation is thorough, and the compaction bug fix is a genuine correctness fix independent of BM25. Here is a breakdown by area. Strengths
Issues1. Corpus counter drift on rollback (functional correctness)
Suggestion: either (a) document explicitly when the user should rebuild, or (b) wire a post-rollback callback that clears 2. Stale counters survive restart with
|
…ness self-heal - computeBM25Scores accumulates in double and narrows to float only when building the cursor entry, removing per-term float rounding error. - Restart staleness: persisted corpus counters can lag the on-disk data (docs indexed after the last schema save). On the first BM25 query of a session the counters are validated once with a cheap live document count (countType) and rebuilt only if they disagree - a clean restart pays nothing, a stale one self-heals. (staleChecked is transient.) - EXPLAIN scoring JSON now includes the bucket name alongside the per-bucket note. - BM25Scorer: removed the trailing blank line.
|
Addressed in 4. Float accumulation precision. 2. Stale counters surviving restart. Implemented self-healing, but a bit cheaper than always invalidating on load (which would force a full re-scan on every restart even when the counters are fresh - costly for large indexes). On the first BM25 query of a session the persisted counters are validated once against a cheap live document count ( 6. EXPLAIN bucket attribution. The scoring JSON now includes the 8 (minor). Removed the trailing blank line in Deferred with rationale (flagged for your call):
Full-text + function suites green (186). |
Code Review: feat/bm25-fulltext-scoringThis is a substantial, well-engineered PR. The design is sound, backward-compatibility handling is thoughtful, and test coverage is good. Below are findings ranging from correctness risks to minor style notes. OverviewAdds Okapi BM25 ranking to Correctness Issues1. TOCTOU on In if (!ftMetadata.isStaleChecked()) {
ftMetadata.markStaleChecked();
final long liveCount = ...countType(typeName, false);
if (liveCount != ftMetadata.getTotalDocs())
computeCorpusCounters(false); // full type scan
}
Suggestion: switch 2. In Performance Considerations3. Two-pass posting scan for unbounded queries on common terms In 4. Acceptable for interactive EXPLAIN. Consider logging a warning when the token count is high, or documenting the scan cost in user-facing docs so operators know to expect it on large indexes. Design / API5. Breaking change: Documented in the PR description and the doc file, and correctly pinned by a test. However, user code that casts 6. A multi-property index containing a field literally named 7. Dual Both fields are public. Callers reading Minor / Style8.
9. Test SQL built via string concatenation In the test 10. Documentation file location The PR adds Compaction FixThe fix in if (writtenValues == 0 || (firstIteration && startedOnContinuation && writtenValues < values.length))The key insight - that a key must not start on a continuation page it will then overflow - is clearly articulated. The Test Coverage
One gap: a test for SummaryPriority items:
Everything else is low priority or informational. The core design is solid. 🤖 Generated with Claude Code |
…ing, empty-corpus test - ensureCounters: claim the one-per-session staleness validation via AtomicBoolean compareAndSet, so concurrent first-queries across a type's shared bucket indexes cannot all run the live count + rescan (fixes the TOCTOU on the old read/write pair). Dropped the misleading `transient` (the class is not Serializable). - Warn at index creation when a boost is configured for a field literally named "content" on a multi-property index (it collides with the query parser's default field and would be silently ignored). - test: EXPLAIN on an empty corpus exercises the df==0 branch and must not fail.
…() multi-term FINE log, release notes - LSMTreeIndexCompacted: note that writeEntryMultipleValues clears the scratch buffer at the start of its loop, so the fresh-page retry re-serializes from a clean buffer (the partial content is discarded, not appended) - makes the compaction-fix invariant explicit at the retry site - FullTextIndexMetadata constructor: note it defaults to BM25 and that defaultBM25() is just a self-documenting alias - getBM25: log at FINE for a multi-term direct get() (it does 2*T posting scans) so operators debugging a slow query can switch to SEARCH_INDEX - RELEASE notes: add the REBUILD INDEX * WITH <setting> first-setting-dropped fix to the bug-fix list Verified false alarm: collectingExclusion IS wired - set true in collectTermsForExclusion and checked in recordScoringToken, so MUST_NOT terms are correctly excluded from scoring tokens. Acknowledged (no change): storeTermFrequency page-header byte (on-disk format change, out of scope; documented); multi-bucket BM25 already covered by bm25RankingHoldsAcrossMultipleBuckets (BUCKETS 4); the once-per-session drift INFO log fires at most once per session (not per query); comment density matches the heavily-commented storage layer and encodes design rationale. Full-text + CONTAINSTEXT suites green (209).
|
Code Review: Native BM25 Full-Text Scoring (PR 4695) - see full review in following comment |
|
Code Review for PR 4695 - see next comment for full review |
|
Code Review: Native BM25 Full-Text Scoring (PR 4695) This is a substantial, well-engineered feature addition. The design is carefully considered and thoroughly documented. Overview Adds Okapi BM25 relevance scoring to FULL_TEXT indexes as the new default; existing indexes keep CLASSIC scoring (zero behavior change on upgrade). Includes field boosts, query-time caret boosts, dollar-score exposure, EXPLAIN/PROFILE annotations, a persistence fix for analyzer config, and a pre-existing compaction bug fix. Strengths
Tests are comprehensive: BM25ScorerTest (exact math, edge cases), FullTextBM25Test (ranking, boosts, CLASSIC fallback, restart persistence, EXPLAIN metadata), FullTextBM25CompactionTest (compaction regression), GraphDatabaseCheckerDiagnosticsTest (supernode fan-out collapse). Issues 1. writeEntryValues - partial write before overflow check In LSMTreeIndexAbstract.writeEntryValues, when storeTermFrequency is on, both the RID and the two varints are written to the scratch buffer before the overflow check. When the i-th entry causes overflow, 'return i' tells the caller 'i complete entries' but the scratch buffer holds i+1 entries worth of bytes. The compaction path re-serializes from scratch on overflow so this extra data is discarded and correctness is preserved. The pre-existing code had the same structure, so the BM25 varints add at most ~20 bytes of extra over-write - no bug, but worth a comment to make the invariant clear for future readers. 2. FullTextQueryExecutor stateful instance fields The class now carries per-query mutable state (scoringTokens, collectingExclusion, tokensOnly, currentBoost). The Javadoc says a new executor is created per search, but resetState() exists specifically because the class can be reused. This dual-mode design should be clarified: either document that the class is not thread-safe and must not be reused across threads, or enforce single-use by making the state constructor-initialized only and removing resetState(). 3. remove() docLen mismatch on field-null delta In LSMTreeFullTextIndex.remove(), docLen is recomputed from the keys passed at remove time. If a field was non-null at index time but null at remove time (or if the analyzer changed), ftMetadata.removeDocument(docLen) under-decrements sumDocLength, drifting avgdl. This is acknowledged in the comment, but worth surfacing in the operational notes: any schema migration that changes an analyzer or nulls out a field before deletion should be followed by REBUILD INDEX ... WITH statsOnly = true. 4. Missing @tag(slow) candidates Per project conventions, tests with noticeably long runtimes should be tagged @tag(slow). FullTextBM25Test includes restart-persistence and large corpus scenarios that may warrant checking. 5. Breaking change scope: IndexCursorEntry identity change This is documented as an extension/plugin API change, but IndexCursorEntry is a public class. Any user code placing results from index.get() into a Set expecting score-differentiated dedup will silently see different behavior. The release note is clear, but consider flagging that plain Set callers (not just plugin authors) are affected. Minor Observations
Summary The main actionable items are: (1) clarify FullTextQueryExecutor thread-safety contract, (2) document the writeEntryValues scratch-buffer over-write invariant, (3) add a docLen-mismatch note to the operational docs. The compaction bug fix alone is worth the merge. This is a high-quality feature addition. Generated with Claude Code |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 37 |
🟢 Coverage 89.69% diff coverage · -7.31% coverage variation
Metric Results Coverage variation ✅ -7.31% coverage variation Diff coverage ✅ 89.69% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (e8fee25) 130891 97215 74.27% Head commit (95586a1) 163306 (+32415) 109355 (+12140) 66.96% (-7.31%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#4695) 708 635 89.69% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
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.
…overflow/throttle comments, operational notes - FullTextQueryExecutor: correct the class Javadoc - it is NOT thread-safe (it carries per-query mutable state); create a new instance per search and do not share across threads. resetState() is a defensive guard against sequential reuse on one thread, not support for concurrent reuse. (Replaces the incorrect 'This class is thread-safe' line.) - writeEntryValues: document that the overflow check runs after the whole entry (RID + tf/docLength) is buffered, so an entry is never split; the trailing over-written entry on overflow is discarded because callers re-serialize the kept subset from a cleared buffer before committing - lastExpansionWarnMs: note the throttle is JVM-wide / shared across indexes (a wildcard on one index can suppress the warning for another for the window) - acceptable for a diagnostic - docs/4687: operational note that deleting after an analyzer change or field-nulling can drift avgdl; repair with REBUILD INDEX ... statsOnly - RELEASE notes: broaden the IndexCursorEntry note to any Set/map-key caller (e.g. Index.get() results), not just plugin authors Verified (no change): collectingExclusion IS wired (set in collectTermsForExclusion, checked in recordScoringToken); @tag(slow) coverage is appropriate (the heavy restart + multi-bucket tests are tagged; the rest run sub-second); writeToJSON already carries the epsilon comment; defaultConstantsStayInSyncWithMetadata self-enforces the duplicated defaults. The trackMissingReference/maxWarnings note is in non-BM25 code (CHECK DATABASE), out of scope for this PR. Full-text + function suites green (204).
Code Review: feat/bm25-fulltext-scoring (#4695)This is a substantial, well-designed PR. The BM25 implementation is mathematically sound, the compaction bug fix is important and correctly reasoned, and the test coverage is excellent. Below are my findings, grouped by severity. Architecture & Design
Correctness IssuesMedium: Double-negation in When the code recurses into the children of a // Current (in collectTermsForExclusion):
for (final BooleanClause clause : ((BooleanQuery) query).clauses()) {
collectTermsForExclusion(clause.query(), excluded); // ignores clause.getOccur()
}Medium: O(N) full index scan for pure-NOT queries
Low: Scalar fields setBm25K1(metadata.getFloat("bm25_k1", bm25K1)); // uses stale value as default if key absent
setBm25B(metadata.getFloat("bm25_b", bm25B));Per-field maps are cleared ( Low: Phrase ordering not enforced in The comment honestly acknowledges this: Code Quality
The flag is correctly propagated when creating a new compacted index and when splitting. The Inconsistent Modern pattern variables (
Having both
When an EXPLAIN query has more than 64 terms, the scoring breakdown is truncated. This is reasonable, but there's no indication in the output that truncation occurred. A trailing Performance
The existing code uses
if (scoringTokens.containsKey(storedKey))
scoringTokens.merge(storedKey, ...);This does two lookups. A single Serialization SafetyThe invariant in Test CoverageThe test suite is comprehensive and well-structured:
One gap: no test for the pure-NOT query full-scan path ( Another gap: no test for a Minor
Summary
Overall the implementation is solid. The compaction bug fix is particularly important and well-handled. Addressing the medium severity issues (especially the double-negation case) before merge would be my main recommendation. |
…egative warning, fromJSON reset, phrase docs Correctness: - collectTermsForExclusion: skip nested MUST_NOT clauses when collecting exclusion terms. A double negation like 'java -(database -tutorial)' (= java AND (NOT database OR tutorial)) previously excluded 'tutorial' documents too, wrongly returning empty; now the double-negated term is not excluded. New regression test - FullTextIndexMetadata.fromJSON: default bm25_k1/bm25_b to the DEFAULT_BM25_* constants (not the current field values) so a key absent from the JSON resets to the default instead of carrying a stale value forward on a recycled instance Observability: - collectAllIndexedRids: a pure-negative query (only MUST_NOT) materializes the whole index to form the complement; log a throttled WARNING (60s, JVM-wide) when the universe exceeds PURE_NEGATIVE_WARN_THRESHOLD so operators notice the O(index) cost and add a positive clause Docs: - docs/4687: document that phrase queries are unordered (all-terms AND, no positional index) - '"java database"' and '"database java"' match the same documents Note: full nested-boolean negation (representing the positive contribution of a double-negated term) would require general boolean evaluation and is out of scope; the fix at least stops wrongly excluding such terms. Full-text + schema suites green (216; FullTextBM25Test 28).
…PLAIN omitted-term count - FullTextQueryExecutor: convert the remaining old-style instanceof+cast pairs in collectMatches/collectTermsForExclusion to Java pattern variables (instanceof final X y), matching the BoostQuery branch's style - consistent within the class - explainScoring: when the term breakdown is truncated at MAX_EXPLAIN_TERMS, also report termsOmitted and termsShown so a user debugging a complex query knows the view is partial (not just a bare termsTruncated flag). Test asserts termsOmitted=16 for an 80-term expansion Acknowledged (no change): storeTermFrequency split/compaction propagation confirmed solid; IndexCursorEntry keeps both score (int, used by all integer-scored indexes) and floatScore (full precision) public - a prior review chose Javadoc over @deprecated and the field is load-bearing across the index layer, so removing/deprecating it is out of proportion; the Javadoc already directs callers to floatScore when precision matters. Full-text + function suites green (205).
…osting toString, pure-NOT test - recordScoringToken: replace containsKey()+merge() (two hash lookups) with a single get() then a conditional put(); the cap still drops only new tokens once the limit is reached, and an existing token whose boost does not increase costs just one lookup - FullTextPostingRID: add toString() including tf/docLength so serialization/scoring issues are easy to spot in logs/debuggers - test: pureNegativeQueryReturnsComplement pins the pure-negative full-index-scan path (collectAllIndexedRids) - '-java' returns exactly the documents without 'java' Acknowledged (no change): the CLASSIC matching path's Map<RID,AtomicInteger> predates this PR - AtomicInteger is one alloc per doc with mutable increments, so the suggested HashMap<RID,Integer>+merge would actually box on every increment; a full conversion to primitive cells across the executor is a broad future cleanup, while the BM25 path I added already uses double[]. The writeEntryMultipleValues buffer-clear invariant is regression-guarded by FullTextBM25CompactionTest (removing the clear corrupts postings -> test fails) plus the Javadoc. The double-negation test (batch 20), the statsOnly SQL test (batch 4), and the phrase-ordering docs note (batch 20) already exist. Full-text + function suites green (206; FullTextBM25Test 29).
Code Review - BM25 Full-Text ScoringThis is a substantial, well-structured addition. The core BM25 math is sound, the backward compatibility is carefully thought through, and the PR description/docs are exceptional. Below are specific observations organized by priority. Breaking Changes - Needs Attention Before Merging1. The docs mention this but it's a runtime breaking change for any user doing 2. Removing score from identity is the right call for BM25 deduplication, but this is a silent behavioral change for third-party code using 3. CLASSIC cursor order reversal (ascending → descending) Any code doing Functional / Correctness ConcernsCorpus counter drift from rolled-back transactions private void countDocuments(final int numDocs, final int docLen) {Counters are incremented at index time, before commit. This is acknowledged and the session-validation repair mitigates it, but the asymmetry (rollbacks inflate counters, never deflate) means BM25 scores trend downward over time in workloads with many rollbacks. For write-heavy workloads this could matter. Consider logging a WARNING when the session-validation check finds a drift above some threshold (e.g. >10%) so operators know to run Schema/index file divergence is a silent misparse If
This is a pre-commit increment. If the caller uses the index for a BM25 Token expansion cap: cap is per-query, not per-subclause The Performance ConcernsDirect // No candidate set → two passes (one for df, one to accumulate)Production operators won't see this unless they enable debug logging. Multi-term BM25 queries via
The first BM25 query on a cold index does a full type scan under a lock on Minor Code Quality IssuesDual Having both
Consider deprecating
This sentinel string is long but clear. One concern: if a user names a real field
The method name suggests it "resolves" something but it's really just returning Test Coverage ObservationsThe test surface is comprehensive. A few gaps worth noting:
What's Done Well
SummaryThis PR is ready for merge with the critical items above addressed:
The compaction fix alone is worth merging even if BM25 were reverted, so if timeline is tight, consider whether that fix can ship independently. |
…note, df=0 test - ensureCounters: escalate the counter-drift log to WARNING (with a 'consider REBUILD INDEX ... statsOnly' hint) when the divergence exceeds 10% of the live count; small drift still self-heals quietly at INFO - countDocuments: document that the pre-commit increment means a BM25 get() later in the SAME transaction sees the just-inserted (uncommitted) document in avgDocLength - marginal, resolves at commit, not a correctness issue - BM25ScorerTest: add the df=0 edge case (term absent from corpus -> highest finite idf, above df=1) Verified false alarm: the MAX_EXPANDED_SCORING_TERMS cap IS query-wide (checked against the single shared scoringTokens map across all subclauses), not per-subclause. Acknowledged (no change, with reasoning): - schema/index divergence sanity check: a tf<=docLength constructor guard looked clean but compaction may sum tf for duplicate (key,RID) postings, so a hard throw could false-positive on legitimate data; a heuristic open-time check gives false confidence. The robust fix is an on-disk format byte (out of scope); divergence stays documented (disaster-recovery note) - two-pass get() at FINE: INFO would spam every multi-term get(); the path has no EXPLAIN (EXPLAIN is the candidate-based SEARCH_INDEX path). FINE is right - background pre-warm on open: a DB-lifecycle/HA change out of proportion; manual REBUILD INDEX ... statsOnly is documented - IndexCursorEntry.score @deprecated: inappropriate - score is the CORRECT field for every integer-scored index (LSM/hash/etc.), so deprecating would wrongly flag all those legitimate uses; the Javadoc already steers float-precision callers to floatScore (consistent with the earlier review decision) - reserved __arcadedb_ field-name ban: the targeted exact-sentinel rejection at FT index creation already prevents the collision; a schema-wide prefix ban is a separate broader policy - resolveTotalDocs naming kept (Javadoc is clear; 'validated' would mislead - it does not validate) Full-text + function suites green (206; BM25ScorerTest 12).
Code Review - BM25 Full-Text ScoringOverall this is a solid, well-thought-out feature. The BM25 math is correct, the persistence fix is needed, and the compaction bug fix is a genuine correctness improvement. Below are specific observations and a few concerns worth discussing before merge. BM25 Mathematics - CorrectThe IDF formula ( The Notable Concern: Compaction Test May Not Exercise the Split
Suggestion: Either assert a minimum document count that provably overflows a single 4096-byte page (calculating: 601 RIDs at ~14 bytes each (tf varint + 3-byte RID + extras) ~ 8,400 bytes, which should overflow one page), or reduce page size to a small value (e.g., 512 bytes) so the split is guaranteed on even a small dataset. A log or counter confirming the split path was hit would make the test intent clear. Notable Gap in Test Coverage: REBUILD INDEX * Offset BugThe REBUILD INDEX * WITH batchSize = 1000would silently use the default
|
| Area | Status |
|---|---|
| BM25 math | Correct |
| Persistence fix | Correct |
| Compaction fix | Correct, but test coverage for multi-page split is uncertain |
REBUILD INDEX * parser fix |
Correct, regression test gap |
| Thread safety | Sound |
| Backward compatibility | Documented |
| Test coverage | Good overall, two specific gaps noted above |
The two items I'd want addressed before merge are the compaction test certainty and the REBUILD INDEX * regression test. The rest are suggestions or notes for follow-up issues.
…REBUILD INDEX * setting parse - FullTextBM25CompactionTest: drop the page size from 4096 to 1024 so the multi-page posting-list split is GUARANTEED, not incidental - 600 RIDs at ~6-8 bytes each (~3.6-4.8 KB) provably overflow a 1024-byte page several times, which is the spanning-multiple-compacted-pages condition the fix targets. Updated the class doc + sizing math comment (applies to both the BM25 and CLASSIC compaction tests) - RebuildIndexStatementTestParserTest: add wildcardCapturesFirstSettingKey - parse 'REBUILD INDEX * WITH batchSize = 1000' and assert the setting actually reaches the AST settings map (checkRightSyntax only proves the grammar accepts it). Pins the firstSettingKeyIndex offset fix that previously dropped the first WITH setting on the * form; also sanity-checks the named form - SQLFunctionSearchIndex: set $score via allResults.getOrDefault(rid, 0f) so it stays non-null even if the entry vanished between containsKey and get (cannot happen on today's single-threaded SQL path; makes the defensive intent explicit) - docs/4687: note that EXPLAIN/PROFILE of a wildcard term walks the term index synchronously (the breakdown is capped but the expansion scan is not) - avoid broad-wildcard PROFILE in tight monitoring loops Verified already-present: both expansion/pure-negative warning throttles already carry the JVM-wide/cross-index scope comment. Full-text + function + parser suites green (208).
Code Review - Native BM25 Full-Text Scoring (#4687)This is a substantial, well-engineered PR. The BM25 implementation is correct and the design decisions are thoughtful and well-documented. OverviewThe PR adds native Okapi BM25 ranking to FULL_TEXT indexes: BM25 scoring with configurable k1/b and per-field boosts, query-time caret boosts via Lucene parser, StrengthsExcellent design documentation. The inline comments explaining design decisions (per-bucket N/df vs. type-wide avgdl, why the no-candidate path does two posting passes, the JMM happens-before reasoning in Backward compatibility is handled correctly. Existing indexes keep CLASSIC scoring on upgrade. The
Compaction fix is correct. The Thread-safety is carefully considered. The Issues and Suggestions1.
|
… reserved-name guard, clearer errors - LSMTreeFullTextIndex: warn at OPEN time (not only when the first query triggers it) when a BM25 index loads with no valid corpus counters, so operators can pre-warm with REBUILD INDEX ... statsOnly before serving traffic - centralize the reserved-property-name check (collision with the parser default-field sentinel) into a public static checkReservedPropertyNames(); call it from BOTH the creation factory and the schema-reload path in LocalSchema, so a hand-edited/restored schema cannot reintroduce the collision undetected - RebuildIndexStatement: clearer error for REBUILD INDEX <classic> WITH statsOnly - 'only BM25 full-text indexes keep corpus statistics; switch to BM25, or omit statsOnly to do a full rebuild' - docs/RELEASE-27.7.1.md: drop the section-header emoji Acknowledged (no change, with reasoning): - IndexCursorEntry.score @deprecated: still inappropriate - score is the correct field for every integer-scored index, so deprecating would wrongly flag all those legitimate uses; the breaking change is in the release notes + class Javadoc, which steer float-precision callers to floatScore - writeEntryValues overflow invariant re-verified: the only callers go through writeEntryMultipleValues, which clears the scratch buffer at the start of its loop and re-serializes the kept subset before committing - so trailing over-written bytes are always discarded (also regression-guarded by FullTextBM25CompactionTest at 1024-byte pages) - trackMissingReference double lookup / formatTopMissingReferences magic number: in CHECK DATABASE code (PR #4653), not part of this BM25 PR - out of scope Full-text + schema + parser suites green (494).
Code Review - PR #4695: Native BM25 Full-Text ScoringOverall this is a well-structured and substantial feature. The algorithm is correct, the storage design is clean, and the test suite is comprehensive. The style is consistent with the project conventions (AssertJ, JSONObject, no System.out, final keywords). Here are findings organized by severity. Major1.
2.
3. In the Minor4. The explain path calls 5. The comment flags that the array reuse is "SAFE ONLY because 6. Streams in The project style generally avoids streams in non-trivial code and prefers arrays/primitives for GC pressure. This is a diagnostic-only path, so it has no performance impact, but a manual SuggestionsS1. The test inserts 41 documents and runs a query over 4 buckets. This should be fast in CI. The S2. No SQL-level test for
S3. Startup log spam for large BM25 deployments
S4.
Style / Convention Observations (all good)
Overall the feature is solid. The three Major items above are the ones worth addressing before merge; the rest are low-risk polish items. |
…loc, docs, drop slow tag - RebuildIndexStatement.recomputeStatistics: instanceof-guard the IndexInternal cast on the named-index path so a custom Index that is not IndexInternal yields a clean 'does not support statistics recomputation' error instead of a bare ClassCastException - computeBM25Scores: allocate the single-element lookup key per token again (revert the shared-array reuse). The reuse depended on get() consuming the array synchronously - flagged twice as a footgun; the few tiny arrays per query are negligible GC, so safety wins - remove(): document the single-rid-per-call symmetry with put()'s per-rid addDocument loop (one document per remove call, so one removeDocument balances it; no caller passes N documents sharing a key) - FullTextQueryExecutor: document why SEARCH_INDEX result entries carry empty keys (a multi-token Lucene query has no single key tuple; the SQL path reads only RID + $score, never getKeys()) - drop @tag(slow) from bm25RankingHoldsAcrossMultipleBuckets (measured ~0.3s; the tag was inconsistent with the untagged 600-doc compaction test) Verified already-present: end-to-end SQL statsOnly test (statsRecomputed=1 + scores), and the descriptive forceCompaction().as(...) message. Acknowledged (no change): the EXPLAIN RHS re-execution is swallowed by try/catch and only re-evaluates literal args in practice; DatabaseChecker streams/magic-number are CHECK DATABASE code (out of scope). S3 (aggregate the open-time cold-counter warning): kept per-index because it names the specific cold index (more actionable than a count) and the state is a transient, bounded post-upgrade condition; aggregating would mean surgery in the large readConfiguration method. Full-text + parser suites green (208).
Code Review: feat/bm25-fulltext-scoring |
Code Review: feat/bm25-fulltext-scoringThis is a well-designed, thoroughly documented PR. The BM25 math is correct, the storage layer changes are sound, and the test coverage is comprehensive. A few items worth discussing before merge. Behavior Changes That Affect Existing Users1. CLASSIC path result order is now reversed (descending) In 2. The PR correctly notes this is a behavioral change (documented in the class Javadoc). The reasoning is sound - score should not be part of identity - but this is a public-API class and downstream code could be using it in Sets or Maps where this change silently affects deduplication. Worth a changelog note. Design / Architecture Observations3. This flag lives only in-memory, derived from the schema at load time. The code comment correctly explains the happens-before guarantee during schema load, but it creates a risk: if the schema file and index files become inconsistent (manual schema edits, disaster recovery, partial restores), the index would silently misinterpret its own bytes. The page format version in the header is the traditional guard for this. Given the low frequency of this scenario and the existing documentation, acceptable as-is, but worth tracking as a known limitation. 4. The PR adds 5. Two-pass streaming in the no-candidate path The Correctness Notes6. Compaction fix is correct The 7. The 8. Duplicate default constants
Minor Style Notes
Test Coverage Assessment
Missing coverage to consider:
SummaryThe BM25 implementation is solid: correct math, good thread-safety model, sensible defaults, backward-compatible upgrade path, and a real compaction bug fix. The main things worth addressing before merge:
The code is otherwise merge-ready. Review generated with Claude Code |
…0 in /studio [skip ci] Bumps [webpack-cli](https://github.com/webpack/webpack-cli) from 6.0.1 to 7.0.0. Release notes *Sourced from [webpack-cli's releases](https://github.com/webpack/webpack-cli/releases).* > webpack-cli@7.0.0 > ----------------- > > ### Major Changes > > * The minimum supported version of Node.js is `20.9.0`. (by [`@alexander-akait`](https://github.com/alexander-akait) in [ArcadeData#4677](https://redirect.github.com/webpack/webpack-cli/pull/4677)) > * Use dynamic import to load `webpack.config.js`, fallback to interpret only when configuration can't be load by dynamic import. Using dynamic imports allows you to take advantage of Node.js's built-in TypeScript support. (by [`@alexander-akait`](https://github.com/alexander-akait) in [ArcadeData#4677](https://redirect.github.com/webpack/webpack-cli/pull/4677)) > * Removed the `--node-env` argument in favor of the `--config-node-env` argument. (by [`@alexander-akait`](https://github.com/alexander-akait) in [ArcadeData#4677](https://redirect.github.com/webpack/webpack-cli/pull/4677)) > * The `version` command only output versions right now. (by [`@alexander-akait`](https://github.com/alexander-akait) in [ArcadeData#4677](https://redirect.github.com/webpack/webpack-cli/pull/4677)) > * Removed deprecated API, no action required unless you use `import cli from "webpack-cli";`/`const cli = require("webpack-cli");`. (by [`@alexander-akait`](https://github.com/alexander-akait) in [ArcadeData#4677](https://redirect.github.com/webpack/webpack-cli/pull/4677)) > > ### Patch Changes > > * Allow configuration freezing. (by [`@alexander-akait`](https://github.com/alexander-akait) in [ArcadeData#4677](https://redirect.github.com/webpack/webpack-cli/pull/4677)) > * Use graceful shutdown when file system cache is enabled. (by [`@alexander-akait`](https://github.com/alexander-akait) in [ArcadeData#4677](https://redirect.github.com/webpack/webpack-cli/pull/4677)) > * Performance improved. (by [`@alexander-akait`](https://github.com/alexander-akait) in [ArcadeData#4677](https://redirect.github.com/webpack/webpack-cli/pull/4677)) Changelog *Sourced from [webpack-cli's changelog](https://github.com/webpack/webpack-cli/blob/main/CHANGELOG.md).* > 7.0.0 > ----- > > ### Major Changes > > * The minimum supported version of Node.js is `20.9.0`. (by [`@alexander-akait`](https://github.com/alexander-akait) in [ArcadeData#4677](https://redirect.github.com/webpack/webpack-cli/pull/4677)) > * Use dynamic import to load `webpack.config.js`, fallback to interpret only when configuration can't be load by dynamic import. Using dynamic imports allows you to take advantage of Node.js's built-in TypeScript support. (by [`@alexander-akait`](https://github.com/alexander-akait) in [ArcadeData#4677](https://redirect.github.com/webpack/webpack-cli/pull/4677)) > * Removed the `--node-env` argument in favor of the `--config-node-env` argument. (by [`@alexander-akait`](https://github.com/alexander-akait) in [ArcadeData#4677](https://redirect.github.com/webpack/webpack-cli/pull/4677)) > * The `version` command only output versions right now. (by [`@alexander-akait`](https://github.com/alexander-akait) in [ArcadeData#4677](https://redirect.github.com/webpack/webpack-cli/pull/4677)) > * Removed deprecated API, no action required unless you use `import cli from "webpack-cli";`/`const cli = require("webpack-cli");`. (by [`@alexander-akait`](https://github.com/alexander-akait) in [ArcadeData#4677](https://redirect.github.com/webpack/webpack-cli/pull/4677)) > > ### Patch Changes > > * Allow configuration freezing. (by [`@alexander-akait`](https://github.com/alexander-akait) in [ArcadeData#4677](https://redirect.github.com/webpack/webpack-cli/pull/4677)) > * Use graceful shutdown when file system cache is enabled. (by [`@alexander-akait`](https://github.com/alexander-akait) in [ArcadeData#4677](https://redirect.github.com/webpack/webpack-cli/pull/4677)) > * Performance improved. (by [`@alexander-akait`](https://github.com/alexander-akait) in [ArcadeData#4677](https://redirect.github.com/webpack/webpack-cli/pull/4677)) Commits * [`0b116f7`](webpack/webpack-cli@0b116f7) chore(release): new release ([ArcadeData#4679](https://redirect.github.com/webpack/webpack-cli/issues/4679)) * [`e0b2f07`](webpack/webpack-cli@e0b2f07) test: improve * [`5328fcb`](webpack/webpack-cli@5328fcb) chore(deps): bump pnpm/action-setup in the dependencies group ([ArcadeData#4699](https://redirect.github.com/webpack/webpack-cli/issues/4699)) * [`4b6f0e1`](webpack/webpack-cli@4b6f0e1) chore(deps): update ([ArcadeData#4696](https://redirect.github.com/webpack/webpack-cli/issues/4696)) * [`47fc332`](webpack/webpack-cli@47fc332) test: more ([ArcadeData#4695](https://redirect.github.com/webpack/webpack-cli/issues/4695)) * [`a199bc3`](webpack/webpack-cli@a199bc3) test: refactor config format test + more ([ArcadeData#4684](https://redirect.github.com/webpack/webpack-cli/issues/4684)) * [`20bc478`](webpack/webpack-cli@20bc478) refactor: code * [`529352d`](webpack/webpack-cli@529352d) docs: update ([ArcadeData#4692](https://redirect.github.com/webpack/webpack-cli/issues/4692)) * [`a01f01b`](webpack/webpack-cli@a01f01b) chore: fix coverage * [`e434e98`](webpack/webpack-cli@e434e98) refactor: make cli faster ([ArcadeData#4690](https://redirect.github.com/webpack/webpack-cli/issues/4690)) * Additional commits viewable in [compare view](https://github.com/webpack/webpack-cli/compare/webpack-cli@6.0.1...webpack-cli@7.0.0) Maintainer changes This version was pushed to npm by [GitHub Actions](<https://www.npmjs.com/~GitHub> Actions), a new releaser for webpack-cli since your current version. [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- Dependabot commands and options You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
…LAIN/PROFILE) (#4695) * feat: [#4687] native BM25 full-text scoring (field boosts, caret, EXPLAIN/PROFILE) Add Okapi BM25 ranking to FULL_TEXT indexes. BM25 (TF/IDF + document-length normalization) becomes the default similarity for newly created full-text indexes; existing indexes keep the legacy term-coordination (CLASSIC) scoring, which stays available via METADATA {"similarity":"CLASSIC"}. - Per-posting term frequency + document length are stored inline through a RID subclass (FullTextPostingRID), gated by a storeTermFrequency flag so every non-full-text LSM index keeps the byte-identical RID-only format. - Configurable k1/b, per-field boosts (metadata) and Lucene-style caret boosts (e.g. title:java^3), combinable inside AND/OR/NOT and grouped queries. The effective weight is caret * field_boost. - $score exposes the float BM25 relevance on every matching row. EXPLAIN/PROFILE annotate the full-text fetch step with the similarity, k1/b, corpus stats (N, avgdl) and each query term's df/idf/boost. - Corpus counters (N, sum of document lengths) and analyzer configuration now persist across restart; previously toJSON dropped the metadata and silently reverted custom analyzers to StandardAnalyzer. Fix (pre-existing, also affected CLASSIC): full-text index compaction dropped postings when a single token's value list spanned multiple compacted pages. The compacted root is a positional sparse index that cannot index one leaf page under two keys, so a key's values left on a shared continuation page became unreachable. Overflowing keys now start on a fresh page they fully own. * fix: [#4687] address review - per-bucket BM25 stats + tokens-only explain - BM25 is scored per bucket (per-shard, like Elasticsearch): the document frequency is read from a single bucket's postings, so N and avgdl must be per-bucket too. The fallback (was countType, type-wide) and recompute (was iterateType, whole type) now use the associated bucket's count/records, removing the systematic IDF bias when a type has multiple buckets. Documented the per-bucket scoring model. - EXPLAIN/PROFILE scoring metadata: collect scoring tokens in a tokens-only mode that skips per-document score accumulation, so no document set is materialized just to read the term/idf/df breakdown. - test: BM25 ranking across multiple buckets. * fix: [#4687] address review - thread-safe corpus counters + doc/comments - Make the BM25 corpus counters thread-safe: totalDocs/sumDocLength are now AtomicLong (countersValid volatile). Concurrent transactions indexing into the same bucket share the per-bucket metadata and were racing on bare long fields. - Clarify in the compaction fix that no bytes are orphaned on the continuation page: the key's entry lives only in the scratch buffer until putByteArray, so the continuation page is flushed with just the previous keys' entries. - Comment the EXPLAIN/PROFILE delegation chain at the FetchFromIndexedFunctionStep call site. - Docs: note the $score type change (now Float, incl. CLASSIC where it was an Integer match count) as the one user-visible change for existing indexes; fold the compaction issue into a "fixed" section. * fix: [#4687] address review - unbiased per-bucket IDF, streaming scorer, validation - IDF consistency (multi-bucket): the FullTextIndexMetadata (and its corpus counters) is shared across a type's bucket indexes, so the counters are type-wide. Since document frequency is read per bucket, N for IDF now comes from the bucket's live record count (matching df scope); the shared counters feed only the average document length. Recompute scans the whole type (not a single bucket, which would corrupt the shared counters). - Unify the two BM25 scoring paths (direct get() and Lucene-syntax executor) into one streaming computeBM25Scores helper: df is stream-counted without materializing the posting list, and only candidate/result postings are held - bounding memory for high-frequency terms and removing duplicated formula code. - ensureCounters() now keys off countersValid (not totalDocs > 0), so a fresh empty index is not rescanned on every query; lazy recompute is in-memory only (no saveConfiguration on the read path). - Validate BM25 params (k1 >= 0, b in [0,1]) in the setters and the METADATA path; reject misconfiguration at index creation. - Centralize the QueryParser default-field sentinel as DEFAULT_FIELD with the collision limitation documented. - Per-field maps -> ConcurrentHashMap (read on the query path, iterated by writeToJSON). Document corpus-counter drift on rollback / analyzer change. - IndexCursorEntry.score: document the BM25 rounding/precision loss (use floatScore). Comment that EXPLAIN reports one representative bucket's stats. - tests: BM25 param validation, document removal + recompute (incl. empty type). * fix: [#4687] address review - single-pass candidate scoring, similarity validation, tests - computeBM25Scores: when a candidate set is given (the SEARCH_INDEX path) score in a SINGLE pass, counting df while collecting only candidate postings - halves cursor I/O for high-frequency terms vs the previous two-pass. The no-candidate path stays two-pass to keep memory bounded. Added a TODO to source df from a per-key index count if the LSM layer ever exposes one. - setSimilarity rejects unknown names (e.g. "LUCENE") instead of silently falling back to CLASSIC; the METADATA path routes through it. - EXPLAIN scoring JSON carries a "note" stating the statistics are per-bucket, so the explained IDF is not mistaken for a global value. - countDocuments: document that rids.length is 1 per document on the standard path. - Docs: corpus-counter drift / no-background-recompute behavior; $score is Float. - Use the imported JSONObject short name in explainScoring. - tests: CLASSIC postings survive compaction (independent regression); $score type pinned to Float for CLASSIC; unknown-similarity rejection; @tag("slow") on the multi-bucket and restart tests. * fix: [#4687] address review - logging, marker guard, state reset, drift test - readEntryValue: keep deletion markers (negative bucket id) as a plain RID instead of wrapping them in FullTextPostingRID, so nothing mistakes a marker for a scorable posting (the tf/docLength varints are still read to stay aligned). - FullTextQueryExecutor: reset per-query matching state (scoringTokens, collectingExclusion, tokensOnly, currentBoost) at the top of search() and explainScoring() - defensive against executor reuse. - associatedBucket(): log a WARNING instead of silently returning null (IDF then falls back to N=1); log the cold-start corpus recompute at INFO (it full-scans the type). - countDocuments: document the rollback-drift risk (counters feed only avgdl; recomputeBM25Counters repairs). - EXPLAIN explainScoring: note the per-term posting scan cost. - docs: fix stale RIDWithStats -> FullTextPostingRID; document the single-pass (candidate) vs two-pass (unbounded) scoring and the bounded-memory trade-off. - tests: rolled-back insert is not indexed and recompute repairs counters; relaxed the fragile hasSize(601/600) checks to >= + containsKey. * fix: [#4687] address review - double-precision scoring, restart staleness self-heal - computeBM25Scores accumulates in double and narrows to float only when building the cursor entry, removing per-term float rounding error. - Restart staleness: persisted corpus counters can lag the on-disk data (docs indexed after the last schema save). On the first BM25 query of a session the counters are validated once with a cheap live document count (countType) and rebuilt only if they disagree - a clean restart pays nothing, a stale one self-heals. (staleChecked is transient.) - EXPLAIN scoring JSON now includes the bucket name alongside the per-bucket note. - BM25Scorer: removed the trailing blank line. * fix: [#4687] address review - CAS staleness check, content-boost warning, empty-corpus test - ensureCounters: claim the one-per-session staleness validation via AtomicBoolean compareAndSet, so concurrent first-queries across a type's shared bucket indexes cannot all run the live count + rescan (fixes the TOCTOU on the old read/write pair). Dropped the misleading `transient` (the class is not Serializable). - Warn at index creation when a boost is configured for a field literally named "content" on a multi-property index (it collides with the query parser's default field and would be silently ignored). - test: EXPLAIN on an empty corpus exercises the df==0 branch and must not fail. * docs/test: [#4687] address review - phrase-boost & format-fragility notes, removal-stats test - Document that phrase-query terms do not receive the configured per-field boost (matched against the unprefixed token; an enclosing caret boost still applies). - Document that storeTermFrequency is schema-derived, not stored in the page header, so schema and index files must stay consistent. - Doc: note the deliberate mixed scope (per-bucket N/df, type-wide avgdl) and how it differs from Elasticsearch/Lucene shard-local avgdl. - test: removal updates BM25 statistics (df/N) so survivors' IDF reflects the deleted document. * fix: [#4687] BM25 review batch - df deletion-marker guard, volatile flag, allocation/sort hoists, test gaps - skip deletion markers (negative bucket id) when counting document frequency in both the candidate and no-candidate scoring passes so deleted postings no longer inflate df/IDF - make storeTermFrequency volatile: set once post-construction but read on every read/write path, ensuring visibility so a reader never misparses value bytes on a stale false - hoist the per-token candidate hits list out of the token loop (reuse with clear()) to avoid per-token allocations - guard the candidate result sort with size() > 1 - replace fully-qualified com.arcadedb.database.Record with an import - document the remove() docLen-recompute drift (analyzer/field changes) and that recomputeBM25Counters() repairs it - tests: static-import assertThatThrownBy/within; add single-document scoring (avgdl edge) and content-field-name vs default-field-sentinel collision tests * fix: [#4687] BM25 review batch 2 - float score plumbing, classic ordering, perf, test gaps - FetchFromIndexStep: read cursor.getFloatScore() into a float field so full-text BM25 scores below 1.0 are no longer truncated to 0 and suppressed (the CONTAINSTEXT path); integer-scored indexes are unaffected - LSMTreeFullTextIndex.get() CLASSIC path: sort most-relevant-first (was ascending, returning least-relevant first) with a deterministic RID tiebreaker, and actually honor the limit via subList (was only used as a list capacity hint) - IndexCursorEntry equals/hashCode: identity is (record, keys) only; the relevance score is a derived value and including it could let the same document appear twice in a result Set when scored differently - explainScoring: skip deletion markers when counting df so the explained df/idf matches what computeBM25Scores uses - FullTextQueryExecutor: collect candidate RIDs via a new raw LSMTreeFullTextIndex.getPostings() instead of index.get(), avoiding running (and discarding) the full BM25 scoring pipeline once per term before scoreCandidatesBM25 scores them - align the scoreCandidatesBM25 limit sentinel (limit > -1) with getBM25 and the classic path - remove the redundant startKey != null check in iterateAndMatch (rangeScan already encodes it) - BM25 single-property indexing: exclude null tokens from document length while still flowing them to the underlying put so NULL_STRATEGY is enforced Tests: - add UPDATE-path regression (old posting removed, new added, no length double-count vs recompute) - add builder-API coverage for withBM25()/withFieldBoost() (metadata + end-to-end ranking/boost) - assert persisted similarity/k1/b and corpus counters directly after restart - add CLASSIC cursor ordering + limit regression - replace brittle float exact-equality with isCloseTo - drop @tag(slow) from FullTextBM25CompactionTest: it is a sub-second correctness regression that must run in CI * fix: [#4687] BM25 review batch 3 - posting validation, invariant docs, explain logging - FullTextPostingRID: reject negative tf/docLength in the constructor so corrupt statistics fail fast instead of producing a nonsensical BM25 contribution; add a regression test - writeTermFrequency: document the alignment invariant - a given index instance writes and reads every page (leaf and compacted root) with the same storeTermFrequency flag, so a root-page pointer's zero varints are always consumed back symmetrically; the pointer may be reconstructed as FullTextPostingRID(0,0) but only its position (target page number) is read, so it is harmless - avgDocLength: document the concurrency approximation boundary (widest skew is one in-flight document, ~avgdl/n, only affects the length-normalization denominator; recomputeBM25Counters() gives an exact value) - FetchFromIndexedFunctionStep: log the EXPLAIN/PROFILE scoring-metadata failure at FINE instead of silently swallowing it * fix: [#4687] BM25 review batch 4 - content-field collision fix, MoreLikeThis IDF, SQL stats repair, release notes Correctness: - fix the DEFAULT_FIELD collision: the query-parser default field is now a non-identifier sentinel and 'unqualified' is resolved index-aware (isUnqualified): the sentinel OR the sole property of a single-property index. A multi-property index with a real 'content' field now scores and boosts content:term correctly (was silently treated as unqualified). Removes the now-obsolete creation-time content-boost warning. - MoreLikeThis: use the live per-bucket document count for IDF instead of the max document-frequency proxy (which understated IDF for shared terms and skewed term selection). Robustness: - TypeFullTextIndexBuilder: guard the FullTextIndexMetadata casts (ftMetadata() helper + a clear error in withMetadata(IndexMetadata)) so misuse fails with an actionable message instead of a bare ClassCastException. - document the writeEntryMultipleValues no-partial-page-write invariant that the compaction continuation-page fix relies on. - BM25Scorer.idf: document the df>N edge (log argument always > 0; idf >= 0 when df <= N; a stale df>N gives a small harmless negative idf). - avgDocLength: document the lock-free approximation boundary. - staleChecked: document that it is intentionally never reset and recompute/rebuild is the in-session recovery path. Feature: - REBUILD INDEX <name|*> WITH statsOnly = true: recompute BM25 corpus counters by rescanning live data, without a full reindex (IndexInternal.recomputeStatistics() default no-op, overridden by LSMTreeFullTextIndex and fanned out by TypeIndex). Repairs counter drift from SQL. Misc: - stale-restart recompute now logs the resulting document count / avgdl for operators. - new test files use the © copyright header for consistency. - RELEASE-27.7.1.md: document the two breaking changes ($score Integer->Float; CLASSIC cursor order now descending) with migration notes. Tests: content-field collision + boost, MoreLikeThis unaffected, builder guards, statsOnly SQL repair (and CLASSIC rejection); full-text + index packages green (637 index tests). * fix: [#4687] BM25 review batch 5 - cold-start rescan guard, sort tiebreaker, REBUILD INDEX * fix, explain/JSON polish - ensureCounters: serialize the cold-start counter rebuild with a double-checked lock on the shared metadata, so concurrent first-queries across a type's bucket indexes don't all run a full type scan - getBM25: add the RID tiebreaker to the result sort so equal-scored documents have a deterministic order, consistent with scoreCandidatesBM25 and the CLASSIC path - fix a pre-existing AST bug: REBUILD INDEX * WITH <setting> silently dropped the first setting (the STAR form has no index-name identifier, but the builder still skipped identifier(0)); affected batchSize/maxAttempts too, now also statsOnly - SQLFunctionSearchIndex.getScoringExplain: on a multi-bucket type, state explicitly that the EXPLAIN/PROFILE statistics are the FIRST of N buckets so per-bucket df/IDF is not mistaken for type-wide - FullTextIndexMetadata.writeToJSON: emit bm25_k1/bm25_b only when tuned away from the defaults (named DEFAULT_BM25_K1/B constants); add defaultBM25(...) factory to make the new-index default-metadata intent explicit - release notes: call out the CLASSIC cursor-order change explicitly as affecting CLASSIC indexes Tests: REBUILD INDEX * {statsOnly} wildcard form; default-vs-tuned k1/b JSON round-trip. Full-text + schema + parser/DDL suites green. * docs: [#4687] BM25 review batch 6 - getter convention, equals/recompute docs, operational notes - FullTextPostingRID: replace public tf/docLength fields with getTf()/getDocLength() getters, matching the RID/DatabaseRID convention; update the (few) call sites - IndexCursorEntry: document the equals/hashCode change (identity is (record, keys) only; score deliberately excluded since #4687) so the semantic shift is discoverable - TypeIndex.recomputeStatistics: document why returning on the first success is correct (all bucket sub-indexes of a type share one similarity + metadata object) - docs/4687: add an Operational notes / known limitations section - rollback counter drift + one-time rescan (repair via REBUILD INDEX ... statsOnly), type-wide avgdl vs per-bucket N/df bias on unbalanced multi-bucket types, and the two-pass I/O on the direct (non-SEARCH_INDEX) lookup path No behavior change (getters are equivalent; the rest is documentation). Full-text + schema suites green. * fix: [#4687] BM25 review batch 7 - reject negative per-field boosts - FullTextIndexMetadata.setFieldBoost: reject boost < 0 (a negative boost would produce negative BM25 term contributions and invert ranking); route the METADATA {...} parse path through the setter so '<field>_boost' gets the same validation as the builder, consistent with the existing k1/b validation - test: negative content_boost is rejected at index creation Full-text + schema suites green. * fix: [#4687] BM25 review batch 8 - load-flag visibility, REBUILD setting NPE, encapsulation, docs, tests Correctness: - RebuildIndexStatement: render WITH-setting values via Expression.toString() instead of .value.toString(). The integer literal's .value is null in this context; the bug was latent (the * form never parsed settings until the prior fix, and parser tests only check syntax) and surfaced as an NPE on 'rebuild index * with batchSize = 1000' - LSMTreeIndexAbstract: make storeTermFrequency private (was protected); subclass reads it via isStoreTermFrequency(). Document that it is published during single-threaded schema load (happens-before any concurrent reader) - FullTextPostingRID: mark final so a subclass cannot pass the instanceof checks with different statistics Polish: - SQLFunctionSearchIndex: rename bm25BucketCount -> ftBucketCount, add !isBM25() guard, fix JSONObject import ordering, document the Float::sum one-bucket-per-RID invariant - RebuildIndexStatement: simplify cast to 'instanceof TypeIndex ti' - rename terse pattern var s -> posting in writeTermFrequency - document: global-vs-per-field tf semantics on multi-field indexes; TODO(perf) on the no-candidate double scan; setCounters/staleChecked session semantics; set-once metadata fields need no volatile; the -1 bucketId sentinel in LocalSchema reload; FullTextPostingRID package placement Tests: avgdl=0 guard stays finite; stale df>N yields small negative idf that dampens (not inverts) ranking; null indexed field under BM25 is skipped without inflating length. Full-text + schema suites green; full index package re-run clean after the NPE fix. * perf: [#4687] BM25 review batch 9 - kill scoring-loop allocations, top-K selection, guards Performance (CLAUDE.md low-GC): - computeBM25Scores accumulates into a per-RID double[1] cell instead of a boxed Double: a document matched by T query terms was boxed T times via Map.merge(Double::sum); now it is a primitive += into a cell allocated once per unique document - reuse a single-element String[] lookup-key buffer across query terms instead of allocating one per term (each underlyingIndex.get() consumes it synchronously) - top-K selection: getBM25 and scoreCandidatesBM25 now share buildScoredCursor(), which uses a bounded min-heap (O(N log K)) when a limit smaller than the result set is requested instead of a full O(N log N) sort - the common ORDER BY $score DESC LIMIT k shape; falls back to a full sort otherwise. Preserves the descending + RID-tiebreaker order Correctness / API: - REBUILD INDEX <name> WITH statsOnly: report "not found" for an unknown index instead of NPE - setBm25K1: document the k1=0 edge (degenerates BM25 to pure IDF / binary presence) - getPostings: make package-private (pre-analyzed key, bypasses analysis; only FullTextQueryExecutor should call it) Tests: BM25 get honors limit returning top-K in descending order (heap path); statsOnly on an unknown index reports not-found. Full-text + schema suites green; full index package (641 tests) green. * fix: [#4687] BM25 review batch 10 - null-token posting, default-detect epsilon, term-expansion cap, docs/tests Correctness: - single-property putWithStats: a null indexed value no longer becomes a FullTextPostingRID with a bogus tf - the null token is kept out of the stats and, if NULL_STRATEGY indexes it, written as a plain RID (tf=0/docLength=0). NULL_STRATEGY skip/error/index behavior unchanged - FullTextIndexMetadata.writeToJSON: detect default k1/b with an epsilon instead of != (1.2f/0.75f are inexact in float32; a JSON round-tripped value could otherwise be persisted as non-default forever) Hardening: - FullTextQueryExecutor: cap the number of distinct BM25 scoring tokens (MAX_EXPANDED_SCORING_TERMS = 4096) so a pathological wildcard/fuzzy expansion cannot add unbounded posting-list scans to the re-rank pass; matching is unaffected (result set stays correct), excess terms just stop contributing to the score, logged once per query Docs: - BM25Scorer.termScore: note the tf<=0 guard also defends the architecturally-impossible root-pointer RID - setCounters: note the volatile-write happens-before edge that publishes the counters - resolveAvgDocLength: clarify the 1.0 fallback also covers an empty corpus - FullTextBM25CompactionTest: correct the 'tiny page size' comment (scenario is driven by document volume at the default page size) Tests: unqualified query aggregates tf across fields (global tf = sum) while a field-qualified query sees only that field's tf. Full-text + schema suites green (FullTextBM25Test 24). * Partial commit * fix: [#4687] BM25 review batch 11 - fromJSON field-config reset, null similarity, doc clarity, test strength Correctness: - FullTextIndexMetadata.fromJSON: clear fieldAnalyzers/fieldBoosts before re-parsing so a reload onto a reused instance replaces the per-field config instead of merging stale entries (these maps are now final ConcurrentHashMaps, no longer swapped wholesale) - setSimilarity(null) now throws IllegalArgumentException instead of silently resetting to BM25, consistent with setBm25K1/setBm25B (fromJSON already guards with has('similarity'), so no caller passes null) Performance: - cache getPropertyNames() in FullTextQueryExecutor (consulted per matched term by isUnqualified) instead of calling it each time Docs / clarity: - getBM25: document that the direct index.get() path does NOT support Lucene query syntax (caret boosts, boolean, phrase, wildcard) - use SEARCH_INDEX for that - ensureCounters stale-check: note both sides are type-wide (shared counter vs countType), so it does not spuriously rescan multi-bucket types; IDF's N is the deliberately-different per-bucket scope - DEFAULT_BM25_K1/B: note they mirror BM25Scorer by value and why they are not a direct reference (package dependency direction) Tests: - idf assertions now use asymmetric df (10, 80) so the N - df numerator term is actually exercised (df = N/2 would mask a dropped N) - fromJSON replaces stale per-field config; setSimilarity(null) rejected Full-text + schema suites green (229). * fix: [#4687] BM25 review batch 12 - drift logging, EXPLAIN term cap, doc/comment clarity - ensureCounters: log at INFO when the session stale-check finds the persisted counters diverged from the live count (so operators notice rollback-induced drift without reading EXPLAIN) - explainScoring: cap the per-term posting scan at MAX_EXPLAIN_TERMS (64) and emit "termsTruncated": true beyond it, so an EXPLAIN of a huge wildcard/fuzzy expansion cannot become a very long scan - avgDocLength: document the intentional type-wide-avgdl vs per-bucket-N/df scope mismatch (and why it must not be 'fixed') so future maintainers understand the trade-off - ensureCounters synchronized block: note it is the only lock needed (counters are AtomicLong elsewhere); it just serializes the cold-start rebuild - docs/4687: add a disaster-recovery note that BM25 index files must be kept in sync with their schema (storeTermFrequency is schema-derived, not page-stored) Test: EXPLAIN of a wildcard expanding past the cap reports termsTruncated. Already covered (verified, no change): $score-is-Float CLASSIC assertion exists; the two-pass TODO already gives the SEARCH_INDEX tip; no Set<IndexCursorEntry> in the codebase relies on score in identity (all add score=1); DEFAULT_BM25_K1/B already carry a keep-in-sync comment. Full-text + schema suites green (225). * docs: [#4687] BM25 review batch 13 - per-bucket/cold-start/drift docs, caret hint, sync test - docs/4687: document that $score is per-bucket (not globally calibrated across a multi-bucket type's buckets); that the first BM25 query on a cold index does a one-time full scan and how to pre-warm with REBUILD INDEX ... statsOnly; and that avgdl drift is only document-count-validated (replace-without-count-change can drift it, repaired by statsOnly recompute) - RELEASE-27.7.1.md: add the IndexCursorEntry equals/hashCode identity change as an extension-API migration note - getBM25: log at FINE when a direct-path query contains a caret, since the direct lookup silently ignores Lucene syntax - BM25ScorerTest: assert FullTextIndexMetadata.DEFAULT_BM25_K1/B equal BM25Scorer.DEFAULT_K1/B so the intentionally-duplicated constants stay in sync (self-enforcing, no compile dependency) Decisions (no change, with reasoning): kept the cold-start rebuild under synchronized - the non-blocking alternatives either distort ranking (concurrent queries scoring with avgdl=1.0) or do N redundant full scans; a one-time post-upgrade stall, pre-warmable via statsOnly, is the soundest trade-off for a search feature. statsOnly name-vs-* asymmetry left as-is (explicit target warrants an explicit error; wildcard is best-effort). Confirmed ensureCounters() precedes resolveAvgDocLength() on the explain path; the double-scan TODO already points at SEARCH_INDEX. Full-text + schema suites green (226; BM25ScorerTest 11). * fix: [#4687] BM25 review batch 14 - reserved property name, varint cast doc, two-pass rationale, operator docs - reject a property whose name equals the reserved default-field sentinel at full-text index creation (would be ambiguous on a multi-property index); fail fast instead of mis-scoring silently. New test. - readEntryValue: document the tf/docLength varint->int narrowing (token counts fit in a signed int; widen here and at the write side if a larger statistic is ever stored) - computeBM25Scores: replace the no-candidate 'TODO halve I/O' with the actual design rationale - the two-pass streaming is a deliberate memory-vs-I/O trade-off favouring bounded peak memory (single-pass would hold a common term's whole posting list alongside the score map); this path is the direct index.get() API, while the primary SQL SEARCH_INDEX path is already single-pass. The clean single-pass needs an LSM per-key entry count (larger change, tracked separately) - RELEASE-27.7.1.md: add an operator note on REBUILD INDEX ... statsOnly for repairing/pre-warming corpus counters after bulk import, heavy rollbacks, or a pre-BM25 migration Verified already-covered (no change): rollback-drift repair test and REBUILD INDEX * statsOnly wildcard test already exist; sumDocLength can't be cheaply validated in the session stale-check (would need a full scan) - documented; storeTermFrequency page-header flag and per-bucket-N/type-wide-avgdl remain documented design choices; IndexCursorEntry identity change already in the release notes. Full-text + schema suites green (232; FullTextBM25Test 26). * perf: [#4687] BM25 review batch 15 - throttle expansion warning, bias avgdl high under concurrency - FullTextQueryExecutor: throttle the scoring-token-cap WARNING to once per 60s across the JVM (static AtomicLong + CAS) instead of once per query. A new executor is created per query, so a repeated huge wildcard would otherwise log on every execution; this matches the engine's 60s saturation-warning pattern - FullTextIndexMetadata: order the corpus-counter writes so a concurrent avgDocLength() read is only ever consistent or biased HIGH (under-penalizing), never momentarily low. addDocument now writes sumDocLength before totalDocs (the counter avgDocLength reads first); by the JMM, observing the new totalDocs implies the new sumDocLength. removeDocument keeps totalDocs-first for the same safe-side bias. Documented the guarantee on avgDocLength() - LSMTreeFullTextIndex: strengthen the storedKey-reuse comment to warn that it is safe only because get() consumes the array synchronously - a future async/lazy read path would need to revisit it Acknowledged (no change): the no-candidate two-pass is a documented memory-vs-I/O trade-off on the non-primary index.get() path; double-checked locking on the shared metadata is correct; counter-drift maintenance via REBUILD INDEX ... statsOnly already documented and surfaced in the release notes. Full-text + schema suites green (227). * docs: [#4687] BM25 review batch 16 - explain get()/CONTAINSTEXT token semantics, fromJSON counter note, tests - getBM25 Javadoc: explain that the direct get() path is intentionally token-based (not Lucene-parsed) because it also backs the SQL CONTAINSTEXT operator, whose argument is literal text - routing it through the Lucene parser would misread 'a-b'/'foo AND bar' and throwing would reject legitimate literal text. Corrected the caret description: StandardAnalyzer tokenizes 'java^3' into [java, 3], so it matches on 'java' with the ^3 silently ignored (not stripped to nothing) - fromJSON: comment that counters are restored by setting fields directly (NOT setCounters()), so the load path does not consume the once-per-session stale-check that self-heals counters lagging the on-disk data after a restart - test: directGetWithLuceneSyntaxIsGracefulNotLuceneParsed documents the get() behavior (caret query matches via tokenization, no crash, no boost applied) Verified (no change): TempIndexCursor.getFloatScore returns the entry floatScore, which for CLASSIC is the integer coordination count widened to float (correct); no existing CHANGELOG/release-notes convention in the repo (only the GitHub-publish template), so RELEASE-27.7.1.md at root is reasonable. Decision: did NOT route get() through the Lucene executor nor throw on Lucene syntax - both would break CONTAINSTEXT, which shares this path and needs literal-token semantics. SEARCH_INDEX remains the deliberate home for Lucene syntax. Full-text + CONTAINSTEXT suites green (220; FullTextBM25Test 27). * docs: [#4687] BM25 review batch 17 - counter-drift follow-up note, get() perf note, release-notes location - countDocuments: document the deferred follow-up for rollback drift - deferring the update to an after-commit callback (TransactionContext.addAfterCommitCallback exists) would avoid it, but is left out because the counter must stay consistent across an HA cluster (the current update is replayed on every node; an after-commit callback fires only on the committing node and would diverge replicas). Point at recomputeBM25Counters() / REBUILD INDEX ... statsOnly as the repair - getBM25 Javadoc: add the performance note that the direct path does 2*T posting scans (vs single-pass SEARCH_INDEX) - RELEASE notes: state explicitly that the CLASSIC cursor-order change also affects direct Index.get() callers, not just SQL - move RELEASE-27.7.1.md to docs/ (no root release-notes convention exists; co-located with the design doc) Verified (no change): resolveStringParam at EXPLAIN time is safe - a variable that resolves to null yields a null query and getScoringExplain returns null (no SCORING line), and an execute() exception is caught by FetchFromIndexedFunctionStep (FINE); DEFAULT_FIELD stays package-private (the reserved-name test uses the literal string, not the constant); IndexCursorEntry contract change already in the release notes; EXPLAIN executor-per-call is bounded by the term cap. Full-text + CONTAINSTEXT suites green (209). * docs: [#4687] BM25 review batch 18 - compaction/factory comments, get() multi-term FINE log, release notes - LSMTreeIndexCompacted: note that writeEntryMultipleValues clears the scratch buffer at the start of its loop, so the fresh-page retry re-serializes from a clean buffer (the partial content is discarded, not appended) - makes the compaction-fix invariant explicit at the retry site - FullTextIndexMetadata constructor: note it defaults to BM25 and that defaultBM25() is just a self-documenting alias - getBM25: log at FINE for a multi-term direct get() (it does 2*T posting scans) so operators debugging a slow query can switch to SEARCH_INDEX - RELEASE notes: add the REBUILD INDEX * WITH <setting> first-setting-dropped fix to the bug-fix list Verified false alarm: collectingExclusion IS wired - set true in collectTermsForExclusion and checked in recordScoringToken, so MUST_NOT terms are correctly excluded from scoring tokens. Acknowledged (no change): storeTermFrequency page-header byte (on-disk format change, out of scope; documented); multi-bucket BM25 already covered by bm25RankingHoldsAcrossMultipleBuckets (BUCKETS 4); the once-per-session drift INFO log fires at most once per session (not per query); comment density matches the heavily-commented storage layer and encodes design rationale. Full-text + CONTAINSTEXT suites green (209). * docs: [#4687] BM25 review batch 19 - fix executor thread-safety doc, overflow/throttle comments, operational notes - FullTextQueryExecutor: correct the class Javadoc - it is NOT thread-safe (it carries per-query mutable state); create a new instance per search and do not share across threads. resetState() is a defensive guard against sequential reuse on one thread, not support for concurrent reuse. (Replaces the incorrect 'This class is thread-safe' line.) - writeEntryValues: document that the overflow check runs after the whole entry (RID + tf/docLength) is buffered, so an entry is never split; the trailing over-written entry on overflow is discarded because callers re-serialize the kept subset from a cleared buffer before committing - lastExpansionWarnMs: note the throttle is JVM-wide / shared across indexes (a wildcard on one index can suppress the warning for another for the window) - acceptable for a diagnostic - docs/4687: operational note that deleting after an analyzer change or field-nulling can drift avgdl; repair with REBUILD INDEX ... statsOnly - RELEASE notes: broaden the IndexCursorEntry note to any Set/map-key caller (e.g. Index.get() results), not just plugin authors Verified (no change): collectingExclusion IS wired (set in collectTermsForExclusion, checked in recordScoringToken); @tag(slow) coverage is appropriate (the heavy restart + multi-bucket tests are tagged; the rest run sub-second); writeToJSON already carries the epsilon comment; defaultConstantsStayInSyncWithMetadata self-enforces the duplicated defaults. The trackMissingReference/maxWarnings note is in non-BM25 code (CHECK DATABASE), out of scope for this PR. Full-text + function suites green (204). * fix: [#4687] BM25 review batch 20 - nested-negation exclusion, pure-negative warning, fromJSON reset, phrase docs Correctness: - collectTermsForExclusion: skip nested MUST_NOT clauses when collecting exclusion terms. A double negation like 'java -(database -tutorial)' (= java AND (NOT database OR tutorial)) previously excluded 'tutorial' documents too, wrongly returning empty; now the double-negated term is not excluded. New regression test - FullTextIndexMetadata.fromJSON: default bm25_k1/bm25_b to the DEFAULT_BM25_* constants (not the current field values) so a key absent from the JSON resets to the default instead of carrying a stale value forward on a recycled instance Observability: - collectAllIndexedRids: a pure-negative query (only MUST_NOT) materializes the whole index to form the complement; log a throttled WARNING (60s, JVM-wide) when the universe exceeds PURE_NEGATIVE_WARN_THRESHOLD so operators notice the O(index) cost and add a positive clause Docs: - docs/4687: document that phrase queries are unordered (all-terms AND, no positional index) - '"java database"' and '"database java"' match the same documents Note: full nested-boolean negation (representing the positive contribution of a double-negated term) would require general boolean evaluation and is out of scope; the fix at least stops wrongly excluding such terms. Full-text + schema suites green (216; FullTextBM25Test 28). * style: [#4687] BM25 review batch 21 - consistent pattern matching, EXPLAIN omitted-term count - FullTextQueryExecutor: convert the remaining old-style instanceof+cast pairs in collectMatches/collectTermsForExclusion to Java pattern variables (instanceof final X y), matching the BoostQuery branch's style - consistent within the class - explainScoring: when the term breakdown is truncated at MAX_EXPLAIN_TERMS, also report termsOmitted and termsShown so a user debugging a complex query knows the view is partial (not just a bare termsTruncated flag). Test asserts termsOmitted=16 for an 80-term expansion Acknowledged (no change): storeTermFrequency split/compaction propagation confirmed solid; IndexCursorEntry keeps both score (int, used by all integer-scored indexes) and floatScore (full precision) public - a prior review chose Javadoc over @deprecated and the field is load-bearing across the index layer, so removing/deprecating it is out of proportion; the Javadoc already directs callers to floatScore when precision matters. Full-text + function suites green (205). * perf: [#4687] BM25 review batch 22 - single-lookup token recording, posting toString, pure-NOT test - recordScoringToken: replace containsKey()+merge() (two hash lookups) with a single get() then a conditional put(); the cap still drops only new tokens once the limit is reached, and an existing token whose boost does not increase costs just one lookup - FullTextPostingRID: add toString() including tf/docLength so serialization/scoring issues are easy to spot in logs/debuggers - test: pureNegativeQueryReturnsComplement pins the pure-negative full-index-scan path (collectAllIndexedRids) - '-java' returns exactly the documents without 'java' Acknowledged (no change): the CLASSIC matching path's Map<RID,AtomicInteger> predates this PR - AtomicInteger is one alloc per doc with mutable increments, so the suggested HashMap<RID,Integer>+merge would actually box on every increment; a full conversion to primitive cells across the executor is a broad future cleanup, while the BM25 path I added already uses double[]. The writeEntryMultipleValues buffer-clear invariant is regression-guarded by FullTextBM25CompactionTest (removing the clear corrupts postings -> test fails) plus the Javadoc. The double-negation test (batch 20), the statsOnly SQL test (batch 4), and the phrase-ordering docs note (batch 20) already exist. Full-text + function suites green (206; FullTextBM25Test 29). * fix: [#4687] BM25 review batch 23 - drift WARNING threshold, same-tx note, df=0 test - ensureCounters: escalate the counter-drift log to WARNING (with a 'consider REBUILD INDEX ... statsOnly' hint) when the divergence exceeds 10% of the live count; small drift still self-heals quietly at INFO - countDocuments: document that the pre-commit increment means a BM25 get() later in the SAME transaction sees the just-inserted (uncommitted) document in avgDocLength - marginal, resolves at commit, not a correctness issue - BM25ScorerTest: add the df=0 edge case (term absent from corpus -> highest finite idf, above df=1) Verified false alarm: the MAX_EXPANDED_SCORING_TERMS cap IS query-wide (checked against the single shared scoringTokens map across all subclauses), not per-subclause. Acknowledged (no change, with reasoning): - schema/index divergence sanity check: a tf<=docLength constructor guard looked clean but compaction may sum tf for duplicate (key,RID) postings, so a hard throw could false-positive on legitimate data; a heuristic open-time check gives false confidence. The robust fix is an on-disk format byte (out of scope); divergence stays documented (disaster-recovery note) - two-pass get() at FINE: INFO would spam every multi-term get(); the path has no EXPLAIN (EXPLAIN is the candidate-based SEARCH_INDEX path). FINE is right - background pre-warm on open: a DB-lifecycle/HA change out of proportion; manual REBUILD INDEX ... statsOnly is documented - IndexCursorEntry.score @deprecated: inappropriate - score is the CORRECT field for every integer-scored index (LSM/hash/etc.), so deprecating would wrongly flag all those legitimate uses; the Javadoc already steers float-precision callers to floatScore (consistent with the earlier review decision) - reserved __arcadedb_ field-name ban: the targeted exact-sentinel rejection at FT index creation already prevents the collision; a schema-wide prefix ban is a separate broader policy - resolveTotalDocs naming kept (Javadoc is clear; 'validated' would mislead - it does not validate) Full-text + function suites green (206; BM25ScorerTest 12). * test: [#4687] BM25 review batch 24 - guarantee compaction split, pin REBUILD INDEX * setting parse - FullTextBM25CompactionTest: drop the page size from 4096 to 1024 so the multi-page posting-list split is GUARANTEED, not incidental - 600 RIDs at ~6-8 bytes each (~3.6-4.8 KB) provably overflow a 1024-byte page several times, which is the spanning-multiple-compacted-pages condition the fix targets. Updated the class doc + sizing math comment (applies to both the BM25 and CLASSIC compaction tests) - RebuildIndexStatementTestParserTest: add wildcardCapturesFirstSettingKey - parse 'REBUILD INDEX * WITH batchSize = 1000' and assert the setting actually reaches the AST settings map (checkRightSyntax only proves the grammar accepts it). Pins the firstSettingKeyIndex offset fix that previously dropped the first WITH setting on the * form; also sanity-checks the named form - SQLFunctionSearchIndex: set $score via allResults.getOrDefault(rid, 0f) so it stays non-null even if the entry vanished between containsKey and get (cannot happen on today's single-threaded SQL path; makes the defensive intent explicit) - docs/4687: note that EXPLAIN/PROFILE of a wildcard term walks the term index synchronously (the breakdown is capped but the expansion scan is not) - avoid broad-wildcard PROFILE in tight monitoring loops Verified already-present: both expansion/pure-negative warning throttles already carry the JVM-wide/cross-index scope comment. Full-text + function + parser suites green (208). * fix: [#4687] BM25 review batch 25 - open-time counter warning, reload reserved-name guard, clearer errors - LSMTreeFullTextIndex: warn at OPEN time (not only when the first query triggers it) when a BM25 index loads with no valid corpus counters, so operators can pre-warm with REBUILD INDEX ... statsOnly before serving traffic - centralize the reserved-property-name check (collision with the parser default-field sentinel) into a public static checkReservedPropertyNames(); call it from BOTH the creation factory and the schema-reload path in LocalSchema, so a hand-edited/restored schema cannot reintroduce the collision undetected - RebuildIndexStatement: clearer error for REBUILD INDEX <classic> WITH statsOnly - 'only BM25 full-text indexes keep corpus statistics; switch to BM25, or omit statsOnly to do a full rebuild' - docs/RELEASE-27.7.1.md: drop the section-header emoji Acknowledged (no change, with reasoning): - IndexCursorEntry.score @deprecated: still inappropriate - score is the correct field for every integer-scored index, so deprecating would wrongly flag all those legitimate uses; the breaking change is in the release notes + class Javadoc, which steer float-precision callers to floatScore - writeEntryValues overflow invariant re-verified: the only callers go through writeEntryMultipleValues, which clears the scratch buffer at the start of its loop and re-serializes the kept subset before committing - so trailing over-written bytes are always discarded (also regression-guarded by FullTextBM25CompactionTest at 1024-byte pages) - trackMissingReference double lookup / formatTopMissingReferences magic number: in CHECK DATABASE code (PR #4653), not part of this BM25 PR - out of scope Full-text + schema + parser suites green (494). * fix: [#4687] BM25 review batch 26 - IndexInternal guard, safer key alloc, docs, drop slow tag - RebuildIndexStatement.recomputeStatistics: instanceof-guard the IndexInternal cast on the named-index path so a custom Index that is not IndexInternal yields a clean 'does not support statistics recomputation' error instead of a bare ClassCastException - computeBM25Scores: allocate the single-element lookup key per token again (revert the shared-array reuse). The reuse depended on get() consuming the array synchronously - flagged twice as a footgun; the few tiny arrays per query are negligible GC, so safety wins - remove(): document the single-rid-per-call symmetry with put()'s per-rid addDocument loop (one document per remove call, so one removeDocument balances it; no caller passes N documents sharing a key) - FullTextQueryExecutor: document why SEARCH_INDEX result entries carry empty keys (a multi-token Lucene query has no single key tuple; the SQL path reads only RID + $score, never getKeys()) - drop @tag(slow) from bm25RankingHoldsAcrossMultipleBuckets (measured ~0.3s; the tag was inconsistent with the untagged 600-doc compaction test) Verified already-present: end-to-end SQL statsOnly test (statsRecomputed=1 + scores), and the descriptive forceCompaction().as(...) message. Acknowledged (no change): the EXPLAIN RHS re-execution is swallowed by try/catch and only re-evaluates literal args in practice; DatabaseChecker streams/magic-number are CHECK DATABASE code (out of scope). S3 (aggregate the open-time cold-counter warning): kept per-index because it names the specific cold index (more actionable than a count) and the state is a transient, bounded post-upgrade condition; aggregating would mean surgery in the large readConfiguration method. Full-text + parser suites green (208). (cherry picked from commit a478394)
1 participant
Closes #4687.
Summary
Adds native Okapi BM25 ranking to
FULL_TEXTindexes. BM25 (TF/IDF + document-length normalization) is now the default similarity for newly created full-text indexes; existing indexes keep the legacy term-coordination scoring (CLASSIC), which stays available viaMETADATA {"similarity":"CLASSIC"}. Zero behavior change on upgrade.What's included
k1/b(defaults 1.2 / 0.75, matching Elasticsearch).effective = caret × field_boost):METADATA {"title_boost": 3.0}title:java^3, usable insideAND/OR/NOTand(group)^n/"phrase"^n.$scoreexposes the float BM25 relevance on every matching row (no extra call needed):FETCH FROM INDEXED FUNCTIONstep with the BM25 similarity,k1/b, corpus stats (totalDocs,avgDocLength) and each query term'sdf/idf/boost(query-level "why these scores").toJSONdropped the metadata and silently reverted custom analyzers toStandardAnalyzer).Storage design
Per-posting term frequency + document length are carried inline through
FullTextPostingRID extends DatabaseRID, so they ride the entire existing RID-typed pipeline (transaction staging, commit replay, compaction, cursors) with no signature changes. The value (de)serialization is gated by astoreTermFrequencyflag derived from the persisted similarity, so every non-full-text LSM index keeps the byte-identical RID-only format. Existing full-text indexes open and score asCLASSIC; getting BM25 on old data requires a rebuild.Pre-existing bug fixed (also affected CLASSIC)
Full-text index compaction dropped postings when a single token's value list spanned multiple compacted pages. The compacted root is a positional sparse index that can't index one leaf page under two keys, so a key's values left on a shared continuation page became unreachable on read. Overflowing keys now start on a fresh page they fully own. This was independent of BM25 (reproduced identically with
CLASSIC).Testing
BM25ScorerTest(exact math),FullTextBM25Test(IDF ranking, length norm, field + caret boosts in AND/OR/NOT, CLASSIC fallback, restart persistence, EXPLAIN metadata),FullTextBM25CompactionTest(postings + scores survive compaction at tiny page sizes).com.arcadedb.index.**+com.arcadedb.graph.**(869) and full-text + all SQL function/method + select/explain suites (1339) pass; existing coordination-scoring tests pinned toCLASSIC.