Use predicate cache for all non-timestamp predicates - #6760
Conversation
Normalize timestamp ranges before installing the predicate cache node so partial and fully covered windows use the same residual predicate key. Preserve search-after cache-node distinction and test reuse in both window directions.
Avoid building and executing the predicate cache warmup query when the configured cache capacity is zero.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a447a98682
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ctx_clone | ||
| .doc_mapper | ||
| .query(split_schema, query_ast.clone(), false, predicate_cache) | ||
| .map_err(|error| TantivyError::InvalidArgument(error.to_string()))? | ||
| .0 |
There was a problem hiding this comment.
When a cache entry is larger than the configured capacity, or the LRU cache cannot evict a recently accessed entry, MemorySizedCache::put silently rejects it. In that case the preceding predicate_warmup_query.count() has already scanned the complete predicate, but rebuilding the query here produces another CacheFillerQuery miss and scans the same predicate again during the actual search; because no entry is retained, every repetition of such a time-bounded query pays two full scans. Preserve the freshly computed hit set for this request or otherwise avoid rebuilding into another miss when admission fails.
Useful? React with 👍 / 👎.
| &quickwit_storage::metrics::PREDICATE_CACHE, | ||
| ), | ||
| key_hasher: CacheKeyHasher::random(), | ||
| enabled: config.capacity().as_u64() > 0, |
There was a problem hiding this comment.
When the real predicate-cache capacity is zero but CacheConfig.virtual_caches contains a nonzero shadow cache, deriving enabled solely from the real capacity disables the entire predicate-cache path. The early returns in get and put, together with leaf_search_single_split omitting the cache context, prevent MemorySizedCache from forwarding accesses and insertions to its virtual caches—the mechanism explicitly used to simulate alternate cache configurations—so the resulting experiment records no meaningful workload. Treat configured virtual caches as enabled for simulation purposes, or otherwise continue feeding their metrics while leaving the real cache disabled.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
no, there should be a way to disable to functionality. Which is currently to set the predicate cache to 0.
| .and_then(|(cache, cache_split_id)| { | ||
| let timestamp_field = ctx.doc_mapper.timestamp_field_name()?; | ||
| let predicate_ast = time_bounded_cached_predicate(&query_ast, timestamp_field)?; | ||
| let predicate_key = serde_json::to_string(&predicate_ast).ok()?; |
There was a problem hiding this comment.
For a time-bounded TermSetQuery spanning multiple fields, terms_per_field is a HashMap, so reparsing the same request can produce a different JSON field order each time. Serializing that AST directly as the predicate key therefore makes identical predicates probabilistically miss the cache, creating duplicate entries and repeatedly executing the new full-split predicate warmup. Build the key from a canonical representation, such as sorting map keys before serialization, so identical requests reliably reuse their entries.
Useful? React with 👍 / 👎.
PSeitz
changed the title
2 participants
Summary