Use hash of the cache key to limit in-memory heap overhead for large search requests - #6727
Use hash of the cache key to limit in-memory heap overhead for large search requests#6727ncoiffier-celonis wants to merge 1 commit into
Conversation
be2b655 to
1490d85
Compare
1490d85 to
22026c9
Compare
![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: 22026c9a6b
ℹ️ 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".
| response | ||
| .partial_hits | ||
| .iter() | ||
| .all(|partial_hit| partial_hit.split_id == split_id) |
There was a problem hiding this comment.
When a hash collision returns a response from another split with no partial_hits, this all check is vacuously true, so get accepts the response and can expose incorrect counts or aggregation data from another index. This is especially relevant to the zero-hit entries this change targets; store the split identity alongside the encoded response and verify it independently of the hits.
AGENTS.md reference: AGENTS.md:L21-L22
Useful? React with 👍 / 👎.
| let key = CacheKeyHash::of(&(split_id.as_str(), query_ast_json.as_str())); | ||
| let encoded_result = self.content.get(&key)?; |
There was a problem hiding this comment.
When two (split_id, query_ast_json) keys collide, this returns the other entry without validating either original key. A same-split collision can therefore apply the wrong hit set because its segment ID still matches, while a cross-split collision with an empty hit set can make the term-absence path in leaf_search_single_split incorrectly prune the requested split without checking the returned segment ID. Preserve enough identity with the value to reject these collisions rather than treating them as cache hits.
AGENTS.md reference: AGENTS.md:L21-L22
Useful? React with 👍 / 👎.
1 participant
Description
Attempt to fix #5878 and #6719.
Store the hash of the cache key for the
partial_request_cache; this is particularly problematic forLeafSearchCachewhich uses the (large)SearchRequestas a cahce key, including the whole query and for queries that mostly return zero-hit splits.Hash collision can happen (for a 64MB cache size, the probability of collision is 1e-27). We're limiting the impact of collisions by ensuring that splits belong to the correct search request. In the case of collision, at least data isolation at the index level is preserved (worse case scenario, we would be seeing incorrect document id from the same split or some missing data).
How was this PR tested?
Unit tests only.