feat(index): per-query index cache hit/miss stats - #7862
Conversation
github-actions
Bot
added
A-python
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIndex cache hit and miss tracking now flows from cache lookups through index metrics and execution summaries into Python and Java scan statistics. Cache APIs return hit indicators, index implementations record them, and language bindings expose the resulting counters. ChangesIndex cache metrics
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant IndexLookup
participant LanceCache
participant MetricsCollector
participant ExecutionSummaryCounts
participant ScanStatistics
participant ScanStats
IndexLookup->>LanceCache: request cached index page
LanceCache-->>IndexLookup: value and was_cached flag
IndexLookup->>MetricsCollector: record hit or miss
MetricsCollector->>ExecutionSummaryCounts: aggregate cache counters
ExecutionSummaryCounts->>ScanStatistics: populate Python scan statistics
ExecutionSummaryCounts->>ScanStats: construct Java scan statistics
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
github-actions
Bot
added
A-python
dad9e47 to
3f83250
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
java/src/main/java/org/lance/ipc/ScanStats.java (1)
38-89: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDocument the added Java binding API.
Add Javadoc for the primary constructor and getters describing per-page semantics and that the deprecated constructor defaults both counters to
0. As per coding guidelines, “Copy Rust documentation about defaults, constraints, and invariants into Javadoc for binding classes.”Also applies to: 122-128
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@java/src/main/java/org/lance/ipc/ScanStats.java` around lines 38 - 89, Add Javadoc to the primary ScanStats constructor and its getters documenting per-page semantics, defaults, constraints, and invariants from the corresponding Rust documentation, including the meaning of each index-cache counter. Update the deprecated ScanStats constructor documentation to state that indexCacheHits and indexCacheMisses default to 0, and ensure the getter documentation covers these defaults and semantics.Source: Coding guidelines
🟡 Other comments (1)
rust/lance-index-core/src/metrics.rs-63-67 (1)
63-67: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDescribe misses as cache non-hits, not successful storage loads.
The lookup sites record a miss for every non-hit, including loader errors. Update the wording to say the lookup was not served from cache and loading was attempted.
rust/lance-index-core/src/metrics.rs#L63-L67: define a miss as a cache non-hit / loader invocation.rust/lance-datafusion/src/exec.rs#L513-L515: use the same non-hit semantics.python/src/scanner.rs#L71-L72: mirror the Rust wording.python/python/lance/lance/__init__.pyi#L891-L892: mirror the Rust wording.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance-index-core/src/metrics.rs` around lines 63 - 67, Update the documentation for record_index_cache_misses and its corresponding miss definitions to describe cache non-hits or loader invocations, not successful storage loads. Apply the same wording at rust/lance-index-core/src/metrics.rs#L63-L67, rust/lance-datafusion/src/exec.rs#L513-L515, python/src/scanner.rs#L71-L72, and python/python/lance/lance/__init__.pyi#L891-L892; clarify that a miss occurs when the lookup is not served from cache and loading is attempted, including loader errors.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/lance-core/src/cache/mod.rs`:
- Line 420: Update the cache lookup return paths around the downcast operations
to handle type mismatches without panicking. Replace the unwrap-based downcasts
with fallible error propagation that includes the cache key and expected value
type, applying the same contextual handling to both affected locations while
preserving the existing cached-status return.
- Around line 382-389: Add compiling async documentation examples and
cross-links for all listed public APIs: in rust/lance-core/src/cache/mod.rs
lines 382-389, demonstrate cold and warm (value, was_cached) results and link
CacheKey and LanceCache; in rust/lance-core/src/cache/mod.rs lines 535-538, add
the equivalent WeakLanceCache example including unavailable-cache behavior; in
rust/lance-datafusion/src/utils.rs lines 247-248, document the metric-key
contract and link collector/accessor usage; in
rust/lance-index-core/src/metrics.rs lines 52-72, link trait callbacks to
LocalMetricsCollector and provide usage guidance; in
rust/lance-datafusion/src/exec.rs lines 502-533, show absent metrics and
hit-ratio calculation; and in python/src/scanner.rs lines 69-72, document the
exposed Rust binding fields with a synchronized usage example.
---
Outside diff comments:
In `@java/src/main/java/org/lance/ipc/ScanStats.java`:
- Around line 38-89: Add Javadoc to the primary ScanStats constructor and its
getters documenting per-page semantics, defaults, constraints, and invariants
from the corresponding Rust documentation, including the meaning of each
index-cache counter. Update the deprecated ScanStats constructor documentation
to state that indexCacheHits and indexCacheMisses default to 0, and ensure the
getter documentation covers these defaults and semantics.
---
Other comments:
In `@rust/lance-index-core/src/metrics.rs`:
- Around line 63-67: Update the documentation for record_index_cache_misses and
its corresponding miss definitions to describe cache non-hits or loader
invocations, not successful storage loads. Apply the same wording at
rust/lance-index-core/src/metrics.rs#L63-L67,
rust/lance-datafusion/src/exec.rs#L513-L515, python/src/scanner.rs#L71-L72, and
python/python/lance/lance/__init__.pyi#L891-L892; clarify that a miss occurs
when the lookup is not served from cache and loading is attempted, including
loader errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: cffc8a4a-0bb1-475e-9ec6-5c72e94bf3a1
📒 Files selected for processing (12)
java/lance-jni/src/blocking_scanner.rsjava/src/main/java/org/lance/ipc/ScanStats.javapython/python/lance/lance/__init__.pyipython/src/scanner.rsrust/lance-core/src/cache/mod.rsrust/lance-datafusion/src/exec.rsrust/lance-datafusion/src/utils.rsrust/lance-index-core/src/metrics.rsrust/lance-index/src/scalar/btree.rsrust/lance-index/src/scalar/inverted/index.rsrust/lance/src/index/vector/ivf/v2.rsrust/lance/src/io/exec/utils.rs
| /// Same as [`get_or_insert_with_key`](Self::get_or_insert_with_key), but | ||
| /// also returns whether the entry was served from cache (`true` = hit, | ||
| /// `false` = miss / loader ran). | ||
| /// | ||
| /// Prefer this over rolling a caller-side `Arc<AtomicBool>` when the | ||
| /// caller needs to record per-query cache hit/miss counters — the backend | ||
| /// already tracks this bit internally and this method just exposes it. | ||
| pub async fn get_or_insert_with_key_hit<K, F, Fut>( |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add examples and cross-links for the new public Rust APIs.
rust/lance-core/src/cache/mod.rs#L382-L389: add a compiling async example showing cold and warm(value, was_cached)results and links toCacheKeyandLanceCache.rust/lance-core/src/cache/mod.rs#L535-L538: add an equivalentWeakLanceCacheexample, including its unavailable-cache behavior.rust/lance-datafusion/src/utils.rs#L247-L248: document the metric-key contract and link to its collector/accessor usage.rust/lance-index-core/src/metrics.rs#L52-L72: link the trait callbacks toLocalMetricsCollectorand include usage guidance.rust/lance-datafusion/src/exec.rs#L502-L533: add an example covering absent metrics and hit-ratio calculation.python/src/scanner.rs#L69-L72: document the exposed Rust binding fields with a synchronized usage example.
📍 Affects 5 files
rust/lance-core/src/cache/mod.rs#L382-L389(this comment)rust/lance-core/src/cache/mod.rs#L535-L538rust/lance-datafusion/src/utils.rs#L247-L248rust/lance-index-core/src/metrics.rs#L52-L72rust/lance-datafusion/src/exec.rs#L502-L533python/src/scanner.rs#L69-L72
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/lance-core/src/cache/mod.rs` around lines 382 - 389, Add compiling async
documentation examples and cross-links for all listed public APIs: in
rust/lance-core/src/cache/mod.rs lines 382-389, demonstrate cold and warm
(value, was_cached) results and link CacheKey and LanceCache; in
rust/lance-core/src/cache/mod.rs lines 535-538, add the equivalent
WeakLanceCache example including unavailable-cache behavior; in
rust/lance-datafusion/src/utils.rs lines 247-248, document the metric-key
contract and link collector/accessor usage; in
rust/lance-index-core/src/metrics.rs lines 52-72, link trait callbacks to
LocalMetricsCollector and provide usage guidance; in
rust/lance-datafusion/src/exec.rs lines 502-533, show absent metrics and
hit-ratio calculation; and in python/src/scanner.rs lines 69-72, document the
exposed Rust binding fields with a synchronized usage example.
Source: Coding guidelines
| } | ||
|
|
||
| Ok(entry.downcast::<K::ValueType>().unwrap()) | ||
| Ok((entry.downcast::<K::ValueType>().unwrap(), was_cached)) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Return a contextual error for cache-entry type mismatches.
A malformed or mismatched backend entry makes these downcast calls panic. Convert the failure into an error containing the cache key and expected type instead.
Also applies to: 563-563
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/lance-core/src/cache/mod.rs` at line 420, Update the cache lookup return
paths around the downcast operations to handle type mismatches without
panicking. Replace the unwrap-based downcasts with fallible error propagation
that includes the cache key and expected value type, applying the same
contextual handling to both affected locations while preserving the existing
cached-status return.
Source: Coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
rust/lance-core/src/cache/mod.rs (2)
382-396: 📐 Maintainability & Code Quality | 🟠 MajorComplete the required public Rust API documentation.
These new public APIs still need compiling examples and cross-links that remain synchronized with their actual signatures and semantics.
rust/lance-core/src/cache/mod.rs#L382-L396: show cold, warm, and coalesced-load behavior.rust/lance-core/src/cache/mod.rs#L543-L546: show the unavailable-cache fallback.rust/lance-index-core/src/metrics.rs#L52-L73: document callback semantics and link toLocalMetricsCollector.rust/lance-index-core/src/metrics.rs#L133-L141: show getter usage with recording and forwarding.rust/lance-datafusion/src/exec.rs#L502-L535: show absent metrics and hit-ratio behavior.As per coding guidelines, public Rust APIs must document semantic behavior with synchronized examples and links to relevant structs and methods.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance-core/src/cache/mod.rs` around lines 382 - 396, The public API documentation is incomplete and lacks synchronized, compiling examples. In rust/lance-core/src/cache/mod.rs:382-396, document get_or_insert_with_key’s cold, warm, and coalesced-load behavior; in rust/lance-core/src/cache/mod.rs:543-546, show the unavailable-cache fallback. In rust/lance-index-core/src/metrics.rs:52-73, document callback semantics and link to LocalMetricsCollector; in rust/lance-index-core/src/metrics.rs:133-141, demonstrate getter usage with recording and forwarding; and in rust/lance-datafusion/src/exec.rs:502-535, document absent metrics and hit-ratio behavior, keeping examples aligned with each API’s actual signature.Source: Coding guidelines
428-428: 🩺 Stability & Availability | 🟠 MajorReplace unchecked cache-entry downcasts with contextual errors.
Both new completion paths can panic when the backend returns an entry of the wrong concrete type. Propagate a fallible error containing the cache key and expected type instead.
As per coding guidelines, library code must not use
.unwrap()for fallible operations, and errors must include full context.Also applies to: 572-572
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance-core/src/cache/mod.rs` at line 428, Replace the unchecked downcasts in both new completion paths with fallible error handling. Propagate an error when the cache entry cannot downcast to K::ValueType, including the cache key and expected concrete type in the error context. Preserve the existing successful return of the typed entry and was_cached flag.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/lance-index-core/src/metrics.rs`:
- Around line 116-121: Update LocalMetricsCollector so index_cache_hits and
index_cache_misses do not make the public struct’s construction or destructuring
inaccessible outside the crate. Preserve the existing public three-field layout,
or provide an equivalent compatibility layer that allows downstream callers to
keep using it while retaining access to cumulative cache metrics.
---
Duplicate comments:
In `@rust/lance-core/src/cache/mod.rs`:
- Around line 382-396: The public API documentation is incomplete and lacks
synchronized, compiling examples. In rust/lance-core/src/cache/mod.rs:382-396,
document get_or_insert_with_key’s cold, warm, and coalesced-load behavior; in
rust/lance-core/src/cache/mod.rs:543-546, show the unavailable-cache fallback.
In rust/lance-index-core/src/metrics.rs:52-73, document callback semantics and
link to LocalMetricsCollector; in rust/lance-index-core/src/metrics.rs:133-141,
demonstrate getter usage with recording and forwarding; and in
rust/lance-datafusion/src/exec.rs:502-535, document absent metrics and hit-ratio
behavior, keeping examples aligned with each API’s actual signature.
- Line 428: Replace the unchecked downcasts in both new completion paths with
fallible error handling. Propagate an error when the cache entry cannot downcast
to K::ValueType, including the cache key and expected concrete type in the error
context. Preserve the existing successful return of the typed entry and
was_cached flag.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 54e4e8d9-edbe-424e-b169-9a865179bd92
📒 Files selected for processing (3)
rust/lance-core/src/cache/mod.rsrust/lance-datafusion/src/exec.rsrust/lance-index-core/src/metrics.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/lance-index/src/scalar/btree.rs (1)
1644-1647: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not count failed page loads as cache misses.
The
_arm records a miss for everyErr, even when the cache lookup orread_pagefailed and no page was loaded. This inflatesindex_cache_missesfor failed searches; record misses only forOk((_, false))and add a regression test for the failed-load path.Proposed fix
match &result { Ok((_, true)) => metrics.record_index_cache_hit(), - _ => metrics.record_index_cache_miss(), + Ok((_, false)) => metrics.record_index_cache_miss(), + Err(_) => {} }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance-index/src/scalar/btree.rs` around lines 1644 - 1647, Update the result handling near the index cache metrics so only Ok((_, false)) records an index cache miss, while Ok((_, true)) records a hit and Err does not update miss metrics. Add a regression test covering a failed cache lookup or read_page path and verify it does not increment index_cache_misses.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@rust/lance-index/src/scalar/btree.rs`:
- Around line 1644-1647: Update the result handling near the index cache metrics
so only Ok((_, false)) records an index cache miss, while Ok((_, true)) records
a hit and Err does not update miss metrics. Add a regression test covering a
failed cache lookup or read_page path and verify it does not increment
index_cache_misses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: d6e7a8d8-1e1b-4de5-9006-4f7202dd430e
📒 Files selected for processing (1)
rust/lance-index/src/scalar/btree.rs
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
java/src/test/java/org/lance/ScannerTest.java-215-218 (1)
215-218: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the documented zero defaults exactly.
These
>= 0checks would pass even if JNI returned incorrect positive counters. SinceScanStatsdefaults both fields to0Lwhen no cache metrics exist, assert the exact values.Proposed fix
- assertTrue(stats.getIndexCacheHits() >= 0); - assertTrue(stats.getIndexCacheMisses() >= 0); + assertEquals(0L, stats.getIndexCacheHits()); + assertEquals(0L, stats.getIndexCacheMisses());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@java/src/test/java/org/lance/ScannerTest.java` around lines 215 - 218, Update the assertions for getIndexCacheHits() and getIndexCacheMisses() in ScannerTest to require the documented default value of exactly 0L, replacing the non-negative checks while preserving the existing JNI marshalling coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Other comments:
In `@java/src/test/java/org/lance/ScannerTest.java`:
- Around line 215-218: Update the assertions for getIndexCacheHits() and
getIndexCacheMisses() in ScannerTest to require the documented default value of
exactly 0L, replacing the non-negative checks while preserving the existing JNI
marshalling coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 232bbd4a-fddd-464c-9de2-63eb61a31512
📒 Files selected for processing (3)
java/src/main/java/org/lance/ipc/ScanStats.javajava/src/test/java/org/lance/ScannerTest.javapython/python/tests/test_scalar_index.py
Xuanwo
left a comment
Xuanwo
left a comment
There was a problem hiding this comment.
I found two issues that need to be addressed before the new per-query cache statistics are reliable.
-
The inverted-index callbacks are dropped on the main FTS scan path.
PostingListReader::posting_listrecords the new callbacks, butsearch_segmentssuppliesFtsIndexMetrics, whoseMetricsCollectorimplementation does not forwardrecord_index_cache_hits/record_index_cache_missesto its innerIndexMetrics; the default implementations are no-ops. As a result, indexed FTS posting-cache activity is reported as0/0, despite the advertised Inverted coverage. Forwarding these methods and adding a cold/warm FTS scan assertion could keep this path covered. -
The public metric contract currently describes resident in-memory hits, while
get_or_insert_with_key_hitreportstruefor coalesced followers of a cold load as well. Those followers are therefore counted as hits even though the same lookup group had to load from storage. The accessors also say0means the scan did not touch any index cache, but active cache boundaries such as Bitmap, NGram, RTree, legacy IVF, and phrase positions are not instrumented. Could we first choose whether the metric means “loader skipped” or “resident at lookup arrival”, then either cover the full advertised page-cache scope or narrow the cross-language documentation and0/0semantics?
|
Thanks @Xuanwo, both points landed as real gaps.
2a. served from cache vs loader skipped semantic — chose "loader skipped", documented everywhere. 2b. Coverage vs the "0 means no cache touched" claim — instrumented the four biggest gaps. Broadened the instrumented boundary set from BTree / IVF (v2) / inverted posting-list to also cover:
Phrase positions (PositionKey), legacy v1 IVF partitions, HNSW graph pages, and quantizer codebooks remain uninstrumented and are now explicitly called out in the user-facing docstrings on all three languages, so 0 no longer implies "the scan did not touch any cache". I'd like to tackle those in a follow-up because a few of them don't currently have a MetricsCollector in scope at the fetch site and would require signature changes I'd rather isolate. Let me know if you'd prefer any of the follow-ups (CacheOutcome three-state, remaining boundaries, wrapper-forwarding automation) folded back into this PR. |
Xuanwo
left a comment
Xuanwo
left a comment
There was a problem hiding this comment.
The wrapper forwarding and documented scope are improved, but two query paths still report counts that diverge from the selected "loader executed / loader skipped" contract.
| return Ok(metadata.lengths[token_id as usize] as usize); | ||
| } | ||
| let (_, length) = self.posting_metadata_for_token(token_id).await?; | ||
| let (_, length) = self.posting_metadata_for_token(token_id, None).await?; |
There was a problem hiding this comment.
The default FTS execution still drops this metadata cache lookup from the per-query counters. MatchQueryExec::execute creates FtsIndexMetrics before build_global_bm25_scorer, but that scorer calls bm25_stats_for_terms -> posting_len_for_token, which reaches this line with None. On a cold v2 query this loads and populates PostingMetadataKey without recording a miss; on the warm query the corresponding hit is also unrecorded. The new FTS test only checks that some posting-list count is non-zero, so it cannot detect this per-term/per-partition undercount. Threading the exec collector through the scorer/stat path, with an exact cold/warm metadata assertion, could keep the documented metadata boundary query-local.
|
|
||
| // Record that we're loading a partition from disk | ||
| if let Some(metrics) = metrics { | ||
| metrics.record_index_cache_miss(); |
There was a problem hiding this comment.
This records a cache miss even when no bitmap page exists and no loader runs. For an equality/IN value absent from index_map, execution reaches this line and then returns the empty result without storage I/O or caching it; repeated queries therefore keep adding false misses under the newly documented “loader ran” semantics. Checking index_map before recording the miss, with an absent-value regression assertion, could prevent a no-I/O scan from reporting a cache miss.
…ircuit Address review from Xuanwo on lance-format#7862 and the two same-shape gaps found while auditing per-query index cache metrics. - inverted: extend `posting_len_for_token`, `df_for_term`, `bm25_scorer_for_final_tokens`, `bm25_stats_for_terms`, `bm25_base_scorer`, and `build_global_bm25_scorer` to accept an `Option<&dyn MetricsCollector>` so the exec-side `FtsIndexMetrics` is threaded into every per-token metadata cache lookup. `MatchQueryExec`, `FlatMatchFilterExec`, and `FlatMatchQueryExec` now pass their collector to `build_global_bm25_scorer`, and the partition-local `bm25_search` fallback threads `Some(metrics.as_ref())` into the scorer path. Fixes the "default FTS execution drops metadata cache lookups" report on `posting_len_for_token(..., None)` and its multi- segment corollary in `bm25_stats_for_terms`. - bitmap: move the `index_map.contains_key` gate above the `record_index_cache_miss` in `load_bitmap`, so a value that is not in the index short-circuits before touching the counters. Range and IsIn queries now thread `Some(metrics)` into `load_bitmap` so bitmap page reads on those code paths finally show up in the per-query cache stats. - inverted phrase positions: upgrade `read_positions` to `get_or_insert_with_key_hit` and record hit/miss under the caller's metrics context. The phrase-query fall-through inside `posting_list` now reports the `PositionKey` cache boundary in per-query stats instead of being silently uninstrumented. - docs: refresh `ExecutionSummaryCounts::index_cache_hits`, the `ScanStatistics` Python stub, and `ScanStats` Javadoc to list the updated instrumented-boundary set (bitmap Range/IsIn, phrase positions), call out that IVF streaming and legacy v1 IVF paths report as a miss on every call, and note that a cold grouped posting-list lookup can record up to two misses for a single term. Tests: - Python: tighten `test_btree_index_cache_hit_miss_stats` to exact cold 1/0 and warm 0/1 counts; add `test_bitmap_index_cache_hit_miss_stats` covering IN cold/warm plus a `color = 'purple'` absent-value regression that asserts hits and misses stay at zero; add `test_phrase_query_cache_hit_miss_stats` that exercises the new `PositionKey` boundary through cold then warm phrase scans. Co-authored-by: TRAE CLI <noreply@bytedance.com>
…ircuit Address review from Xuanwo on lance-format#7862 and the two same-shape gaps found while auditing per-query index cache metrics. - inverted: extend `posting_len_for_token`, `df_for_term`, `bm25_scorer_for_final_tokens`, `bm25_stats_for_terms`, `bm25_base_scorer`, and `build_global_bm25_scorer` to accept an `Option<&dyn MetricsCollector>` so the exec-side `FtsIndexMetrics` is threaded into every per-token metadata cache lookup. `MatchQueryExec`, `FlatMatchFilterExec`, and `FlatMatchQueryExec` now pass their collector to `build_global_bm25_scorer`, and the partition-local `bm25_search` fallback threads `Some(metrics.as_ref())` into the scorer path. Fixes the "default FTS execution drops metadata cache lookups" report on `posting_len_for_token(..., None)` and its multi- segment corollary in `bm25_stats_for_terms`. - bitmap: move the `index_map.contains_key` gate above the `record_index_cache_miss` in `load_bitmap`, so a value that is not in the index short-circuits before touching the counters. Range and IsIn queries now thread `Some(metrics)` into `load_bitmap` so bitmap page reads on those code paths finally show up in the per-query cache stats. - inverted phrase positions: upgrade `read_positions` to `get_or_insert_with_key_hit` and record hit/miss under the caller's metrics context. The phrase-query fall-through inside `posting_list` now reports the `PositionKey` cache boundary in per-query stats instead of being silently uninstrumented. - docs: refresh `ExecutionSummaryCounts::index_cache_hits`, the `ScanStatistics` Python stub, and `ScanStats` Javadoc to list the updated instrumented-boundary set (bitmap Range/IsIn, phrase positions), call out that IVF streaming and legacy v1 IVF paths report as a miss on every call, and note that a cold grouped posting-list lookup can record up to two misses for a single term. Tests: - Python: tighten `test_btree_index_cache_hit_miss_stats` to exact cold 1/0 and warm 0/1 counts; add `test_bitmap_index_cache_hit_miss_stats` covering IN cold/warm plus a `color = 'purple'` absent-value regression that asserts hits and misses stay at zero; add `test_phrase_query_cache_hit_miss_stats` that exercises the new `PositionKey` boundary through cold then warm phrase scans. Co-authored-by: TRAE CLI <noreply@bytedance.com>
1c8478b to
cedb403
Compare
Surface per-page index cache hit and miss counts alongside the existing `iops` / `parts_loaded` / `index_comparisons` metrics, so users can tell whether a scan warmed from the in-memory index cache or paid the storage cost. Wiring: - Extend `MetricsCollector` with `record_index_cache_hit/miss` (default no-op) and mirror on `LocalMetricsCollector`, `IndexMetrics`, and `FtsIndexMetrics` (wrapper forwarding). - Add matching `Count` fields to `IndexMetrics` so the two metrics show up automatically in every `ExecutionPlan` node that already uses it. - Expose a hit-aware cache API `LanceCache::get_or_insert_with_key_hit` returning `(Arc<V>, was_cached)` and its `WeakLanceCache` counterpart, so per-query callers no longer have to wrap loaders in an extra `Arc<AtomicBool>`. - Aggregate into `ExecutionSummaryCounts` with an `index_cache_hit_ratio()` helper, propagate through Python `ScanStatistics` (pyi stubs updated) and Java `ScanStats` (10-arg constructor, 8-arg overload kept as `@Deprecated`). Instrumented boundaries in this release: BTree page, IVF partition (v2, write-cache scan path), inverted posting list (grouped and per-token) and its per-token metadata (`PostingMetadataKey`), inverted phrase positions (`PositionKey`), bitmap (Equals / Range / IsIn), ngram, and rtree page / null slot. Correctness fixes discovered during instrumentation: - Thread `Option<&dyn MetricsCollector>` through the whole BM25 stat path (`posting_len_for_token`, `df_for_term`, `bm25_scorer_for_final_tokens`, `bm25_stats_for_terms`, `bm25_base_scorer`, `build_global_bm25_scorer`) and forward it from `MatchQueryExec`, `FlatMatchFilterExec`, `FlatMatchQueryExec`, and the partition-local `bm25_search` fallback so per-token metadata cache lookups show up in per-query stats on every FTS path. - Bitmap: gate `load_bitmap` on `index_map.contains_key` before recording a miss, so a value not in the index short-circuits without polluting the counters, and thread `Some(metrics)` through the Range/IsIn code paths so their bitmap page reads finally count. - Inverted phrase positions: upgrade `read_positions` to `get_or_insert_with_key_hit` and record hit/miss under the caller's metrics context. - All cache-boundary sites record hit/miss even when the loader errors, matching the "loader ran" semantics documented on the accessors. Docs on `ExecutionSummaryCounts::index_cache_hits`, the `ScanStatistics` Python stub, and `ScanStats` Javadoc call out the instrumented boundary set, note that IVF streaming and legacy v1 IVF paths report as a miss on every call, and warn that a cold grouped posting-list lookup can record up to two misses for a single term. HNSW graph pages and quantizer codebooks are still uninstrumented and are noted as follow-up work. Tests: - `lance-index-core`: `local_metrics_collector_forwards_cache_counts`, `no_op_metrics_collector_ignores_cache_counts`. - `lance-index`: `test_page_cache_hit_miss_counts` in `btree.rs` runs a cold scan then a warm scan and asserts the expected 0/1 and 1/0 hit/miss counts; a targeted `get_or_insert_with_key_hit` test pins the `(value, was_cached)` contract. - Python: `test_btree_index_cache_hit_miss_stats` uses exact 1/0 and 0/1 counts; `test_bitmap_index_cache_hit_miss_stats` covers cold/warm IN queries plus a `color = 'purple'` absent-value regression that asserts hits/misses stay at zero; `test_phrase_query_cache_hit_miss_stats` exercises the `PositionKey` boundary through cold then warm phrase scans; `test_fts_index_cache_hit_miss_stats` proves the FTS wrapper forwarding. - Java: `ScannerTest.getStats()` reads `ScanStats.getIndexCacheHits()` / `getIndexCacheMisses()` to prove the 10-arg constructor wired up by the JNI patch does not throw and defaults to zero.
cedb403 to
893dc73
Compare
|
@Xuanwo, I have addressed your concerns. Please take a look. |
…build lance-format#7862 threads a `MetricsCollector` through `bm25_stats_for_terms`, `df_for_term`, and `posting_len_for_token` so a query's index cache hits and misses are attributed to it. The single-column path passes one; the cross-field path had nowhere to put it and discarded the metrics in three places: the V3 arm of `bm25_row_stats_for_terms` delegated with `None`, and `row_stats_for_terms` passed `None` to `posting_len_for_token` and a no-op collector to `posting_list`. So `EXPLAIN ANALYZE` on a cold `combined_fields` query undercounted `index_cache_misses` by terms x columns x partitions, and `index_cache_hit_ratio` was not comparable with the same query expressed as `match`. Accuracy only: collector methods default to no-ops, so nothing depended on the threading. `build_combined_bm25_scorer`, `flat_combined_fields_search_stream`, `bm25_row_stats_for_terms`, and `row_stats_for_terms` now take the collector in the position upstream uses, and both the indexed and flat execs pass their own. None of these signatures exist on main, so no released API changes. Also correct `DocSet::row_ids_strictly_ascending`'s doc comment, which claimed frag-reuse partitions carrying tombstoned rows "are correctly rejected here". The function only checks ascension, and `TOMBSTONE_ROW` is the maximum address, so a set whose last document is tombstoned still reports `true`. It is harmless for a reason worth recording: the query-side legacy loader drops deleted rows rather than tombstoning them, tombstones only appear in build-side sets, and `combined_fields_search` forces the fallback for legacy partitions regardless. Finally remove two guards for the deferred-row_id `DocSet` that lance-format#7863 deleted with `lazy_docset.rs`. The `has_row_ids()` conjunct could only be false for a zero-document set, since every non-test constructor fills `row_ids`, and the `row_ids.is_empty()` branch in `num_distinct_rows` became unreachable once that conjunct went. Zero-document sets now answer `true` and `0`, agreeing with the modern representation, which is what current writers emit.
…build lance-format#7862 threads a `MetricsCollector` through `bm25_stats_for_terms`, `df_for_term`, and `posting_len_for_token` so a query's index cache hits and misses are attributed to it. The single-column path passes one; the cross-field path had nowhere to put it and discarded the metrics in three places: the V3 arm of `bm25_row_stats_for_terms` delegated with `None`, and `row_stats_for_terms` passed `None` to `posting_len_for_token` and a no-op collector to `posting_list`. So `EXPLAIN ANALYZE` on a cold `combined_fields` query undercounted `index_cache_misses` by terms x columns x partitions, and `index_cache_hit_ratio` was not comparable with the same query expressed as `match`. Accuracy only: collector methods default to no-ops, so nothing depended on the threading. `build_combined_bm25_scorer`, `flat_combined_fields_search_stream`, `bm25_row_stats_for_terms`, and `row_stats_for_terms` now take the collector in the position upstream uses, and both the indexed and flat execs pass their own. None of these signatures exist on main, so no released API changes. Also correct `DocSet::row_ids_strictly_ascending`'s doc comment, which claimed frag-reuse partitions carrying tombstoned rows "are correctly rejected here". The function only checks ascension, and `TOMBSTONE_ROW` is the maximum address, so a set whose last document is tombstoned still reports `true`. It is harmless for a reason worth recording: the query-side legacy loader drops deleted rows rather than tombstoning them, tombstones only appear in build-side sets, and `combined_fields_search` forces the fallback for legacy partitions regardless. Finally remove two guards for the deferred-row_id `DocSet` that lance-format#7863 deleted with `lazy_docset.rs`. The `has_row_ids()` conjunct could only be false for a zero-document set, since every non-test constructor fills `row_ids`, and the `row_ids.is_empty()` branch in `num_distinct_rows` became unreachable once that conjunct went. Zero-document sets now answer `true` and `0`, agreeing with the modern representation, which is what current writers emit.
2 participants
What
Adds per-query index cache hit/miss statistics alongside the existing
iops,parts_loaded,index_comparisonsmetrics, so users can tell whether a scan warmed from the in-memory index cache or paid the storage cost.New fields surfaced in every
EXPLAIN ANALYZEoutput and in theExecutionSummaryCounts/ PythonScanStatistics/ JavaScanStatsstructures:index_cache_hits— page-level lookups served from cacheindex_cache_misses— page-level lookups that had to load from storageExecutionSummaryCounts::index_cache_hit_ratio()Why
There is a global hit/miss counter on
LanceCache, but it mixes all queries in a session. Users diagnosing cache warm-up, tuning cache size, or writing benchmarks need per-query attribution. This piggy-backs on the existingMetricsCollectorwire.How
MetricsCollectorwithrecord_index_cache_hit/miss(default no-op) and mirror onLocalMetricsCollector+dump_into.Countfields toIndexMetricsso the two metrics show up automatically in everyExecutionPlannode that already uses it.AtomicBoolsentinel aroundget_or_insert_with_key:BTreeIndex::lookup_pageIVFIndex::load_partition(bothwrite_cacheand read-only branches)InvertedPartition::posting_list(grouped + per-token fallback)ExecutionSummaryCountsand propagate through PythonScanStatisticsand JavaScanStats(Java constructor grows two extralongparameters; the JNI is updated).Follow-up
HNSW graph pages, quantizer codebooks, and inverted
read_positionsare intentionally left for a follow-up (their fetch sites don't have aMetricsCollectorreachable today or don't uniformly go throughget_or_insert_with_key), so this PR stays reviewable.Tests
lance-index-core:local_metrics_collector_forwards_cache_counts,no_op_metrics_collector_ignores_cache_countslance-index: newtest_page_cache_hit_miss_countsinbtree.rsthat runs a cold scan then a warm scan and asserts the expected 0/1 and 1/0 hit/miss countslance-core cachetests andlance-datafusionlib tests pass unchanged