fix: CHECK DATABASE causes Java heap space OOM (6GB heap) - #4653
Merged
Conversation
Investigated and fixed by Leonardo for client issue https://github.com/ArcadeData/arcadedb-operations/issues/474.
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Complexity | 2 medium |
🟢 Metrics 7 complexity
Metric Results Complexity 7
🟢 Coverage 46.62% diff coverage · -6.27% coverage variation
Metric Results Coverage variation ✅ -6.27% coverage variation Diff coverage ✅ 46.62% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (fd8da4f) 130491 96084 73.63% Head commit (a133455) 162561 (+32070) 109506 (+13422) 67.36% (-6.27%) 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 (#4653) 148 69 46.62% 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.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4653 +/- ##
============================================
+ Coverage 64.57% 65.78% +1.21%
============================================
Files 1669 1671 +2
Lines 130491 130758 +267
Branches 27986 27996 +10
============================================
+ Hits 84259 86020 +1761
+ Misses 34442 32878 -1564
- Partials 11790 11860 +70 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
lvca
self-requested a review
June 19, 2026 14:53
Contributor
|
Tick the box to add this pull request to the merge queue (same as
|
…CK DATABASE updateStats() already accumulates the Long stats (including totalWarnings and totalCorruptedRecords) from each GraphDatabaseChecker call, so the explicit puts in checkEdges()/checkVertices() added them a second time, reporting 2x the real totals. Removed the redundant puts. Added checkTotalsAreNotDoubleCounted regression test (restricts the check to the edge type so a single pass is the only contributor and no cap is hit: the raw totals must equal the deduplicated collection sizes). Strengthened checkWarningCapPreventsOOM (corrupted collection capped, totalCorruptedRecords>3) and removed redundant same-package imports.
Code Review: fix: CHECK DATABASE causes Java heap space OOM (6GB heap)Overview This PR fixes a real OOM issue where The root cause diagnosis is correct and the approach is sound. A few issues worth addressing before merge: Bug / Logic
In final List<RID> corruptedRecords = new ArrayList<>();In Meanwhile Mitigation options:
Design
The same value is passed as both maxWarnings - currentWarnings, maxWarnings - currentCorruptedThese are conceptually different limits. In a highly corrupted database, the number of unique corrupted RIDs can be much larger than the number of human-readable warning strings. Consider two separate fields (
After multiple type passes, if duplicate warning strings are deduplicated by the result Minor / StyleVerbose Javadoc on test methods The project CLAUDE.md says: "don't add multi-paragraph docstrings or multi-line comment blocks - one short line max". Both new tests have multi-paragraph block comments. Reduce to a single-line Call-site comment about // NOTE: totalWarnings and totalCorruptedRecords are accumulated by updateStats() above...This comment appears at the call site rather than inside Overflow logging can still flood logs at high corruption rates When the cap is exceeded, every overflow warning is logged individually via What's Good
SummaryThe fix is correct for the reported OOM. The main thing to resolve before merge is the |
…corrupted set Address code review on PR #4653: totalCorrupted could over-count vs the deduplicated corruptedRecords set. Two causes: checkEdges used List<RID> while checkVertices used LinkedHashSet<RID>, and addCorrupted incremented the counter unconditionally even on duplicate adds (an edge dangling on both sides flags the same edgeRID twice). - checkEdges now uses LinkedHashSet<RID> (matching checkVertices) - addCorrupted counts totalCorrupted only when the item is genuinely new (Collection.add() returns true) while under the cap; over the cap it counts occurrences since dedup is no longer possible - guard the remaining-cap arithmetic with Math.max(0, ...) at both call sites - move the double-count caution into updateStats() Javadoc - add regression checkCorruptedCounterDedupsBothSidesDangling and trim the verbose test Javadocs to one-line comments Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
lvca
added a commit
that referenced
this pull request
Jun 24, 2026
… 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).
lvca
added a commit
that referenced
this pull request
Jun 24, 2026
…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).
robfrank
pushed a commit
that referenced
this pull request
Aug 14, 2026
* fix: CHECK DATABASE causes Java heap space OOM (6GB heap) Investigated and fixed by Leonardo for client issue ArcadeData/arcadedb-operations#474. * fix: avoid double-counting totalWarnings/totalCorruptedRecords in CHECK DATABASE updateStats() already accumulates the Long stats (including totalWarnings and totalCorruptedRecords) from each GraphDatabaseChecker call, so the explicit puts in checkEdges()/checkVertices() added them a second time, reporting 2x the real totals. Removed the redundant puts. Added checkTotalsAreNotDoubleCounted regression test (restricts the check to the edge type so a single pass is the only contributor and no cap is hit: the raw totals must equal the deduplicated collection sizes). Strengthened checkWarningCapPreventsOOM (corrupted collection capped, totalCorruptedRecords>3) and removed redundant same-package imports. * fix: dedup totalCorruptedRecords in CHECK DATABASE so it matches the corrupted set Address code review on PR #4653: totalCorrupted could over-count vs the deduplicated corruptedRecords set. Two causes: checkEdges used List<RID> while checkVertices used LinkedHashSet<RID>, and addCorrupted incremented the counter unconditionally even on duplicate adds (an edge dangling on both sides flags the same edgeRID twice). - checkEdges now uses LinkedHashSet<RID> (matching checkVertices) - addCorrupted counts totalCorrupted only when the item is genuinely new (Collection.add() returns true) while under the cap; over the cap it counts occurrences since dedup is no longer possible - guard the remaining-cap arithmetic with Math.max(0, ...) at both call sites - move the double-count caution into updateStats() Javadoc - add regression checkCorruptedCounterDedupsBothSidesDangling and trim the verbose test Javadocs to one-line comments Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Leonardo Page <l.page@arcadedata.com> Co-authored-by: Luca Garulli <lvca@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> (cherry picked from commit 62d0c22)
robfrank
pushed a commit
that referenced
this pull request
Aug 14, 2026
…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)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix for a customer-reported issue: CHECK DATABASE causes Java heap space OOM (6GB heap).
I traced the root cause, reproduced it with a test, and verified the fix. Internal tracking: https://github.com/ArcadeData/arcadedb-operations/issues/474
Please review before merging.