Fix O(N) time & space in QueryCache search when queryKey is present #9589
Conversation
WalkthroughAdds a trie-based prefix index and a registry of queryKeyHashFn refcounts to QueryCache, reworks find/findAll to use exact-hash and prefix lookups, propagates hash-function changes from Query.setOptions to the cache, and adds/adjusts tests to validate prefix filtering. No public API signatures (types) were changed except an internal QueryCache method addition. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant QueryCache
participant Index as Trie Index
participant Registry as HashFn Registry
participant Store as Query Map
rect rgb(245,248,255)
Caller->>QueryCache: find(filters)
alt filters.exact && filters.queryKey
QueryCache->>QueryCache: findExact(filters)
QueryCache->>Store: lookup(hashKey(filters.queryKey))
alt not found
loop try known hash fns
QueryCache->>Registry: try fn(filters.queryKey)
QueryCache->>Store: lookup by hash
end
end
QueryCache->>Caller: first match passing matchQuery
else prefix/loose match
QueryCache->>Index: iteratePrefix(filters.queryKey?)
Index-->>QueryCache: candidate iterable
QueryCache->>QueryCache: filter with matchQuery & predicate
QueryCache-->>Caller: first match / results
end
end
sequenceDiagram
autonumber
participant Query
participant QueryCache
participant Registry as HashFn Registry
Note over Query,QueryCache: Hash function change propagation
Query->>Query: setOptions(newOptions)
Query->>Query: detect change in queryKeyHashFn (old → new)
alt changed after initialization
Query->>QueryCache: onQueryKeyHashFunctionChanged(old, new)
QueryCache->>Registry: dec ref(old) / inc ref(new)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (5)
packages/query-core/src/__tests__/queryCache.test.tsx (1)
3-3: Use a type-only import for Query to satisfy consistent-type-imports.Query is only used as a type in this test. Switch to an import type to fix the ESLint error.
-import { Query, QueryCache, QueryClient, QueryObserver } from '..' +import { QueryCache, QueryClient, QueryObserver } from '..' +import type { Query } from '..'packages/query-core/src/queryCache.ts (4)
144-164: Typo in comment (“therefor” → “therefore”).Minor spelling fix improves readability for future maintainers.
- * - Entries who's TKey path leads exactly to this node, - * therefor their path is all primitives. + * - Entries whose TKey path leads exactly to this node, + * therefore their path is all primitives.
91-119: Encapsulated RefCountSet looks good; consider centralizing utilities.RefCountSet will likely be reused (you’re already using it for both trie leaves). Consider moving it (and MapTrieSet) into an internal data-structures module (e.g., src/internal/collections.ts) to keep queryCache.ts focused on cache concerns.
474-507: Optional: Fallback to O(N) scan if hashed lookup fails (preserves semantics when queryHash is overridden).If users supply a custom options.queryHash string that can’t be reproduced from queryKey and a known hashFn, hashed lookup will miss although legacy scanning would find it. Consider an O(N) fallback only when hashed lookup fails, to preserve correctness in rare cases.
let query = tryHashFn(hashKey) if (!query) { for (const hashFn of this.#knownHashFns) { query = tryHashFn(hashFn) if (query) { break } } } - return query as unknown as Query<TQueryFnData, TError, TData> | undefined + if (query) { + return query as unknown as Query<TQueryFnData, TError, TData> + } + // Fallback: preserve legacy behavior for non-standard hashes. + for (const q of this.#queries.values()) { + if (matchQuery({ ...filters, exact: true }, q)) { + return q as unknown as Query<TQueryFnData, TError, TData> + } + } + return undefined
462-466: Avoid intermediate array allocation in prefix path.Stream filter candidates to reduce peak memory when many queries share the prefix.
- const withPrefix = this.#keyIndex.getByPrefix(filters.queryKey) - const candidates = withPrefix ? Array.from(withPrefix) : [] - return candidates.filter((query) => matchQuery(filters, query)) + const withPrefix = this.#keyIndex.getByPrefix(filters.queryKey) + if (!withPrefix) return [] + const out: Array<Query> = [] + for (const q of withPrefix) { + if (matchQuery(filters, q)) out.push(q) + } + return out
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/query-core/src/__tests__/queryCache.test.tsx(2 hunks)packages/query-core/src/query.ts(1 hunks)packages/query-core/src/queryCache.ts(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/query-core/src/queryCache.ts (3)
packages/query-core/src/types.ts (2)
QueryKey(53-61)QueryKeyHashFunction(186-188)packages/query-core/src/query.ts (1)
Query(159-689)packages/query-core/src/utils.ts (3)
matchQuery(126-172)QueryFilters(18-43)hashKey(215-226)
🪛 ESLint
packages/query-core/src/__tests__/queryCache.test.tsx
[error] 3-3: Imports "Query" are only used as type.
(@typescript-eslint/consistent-type-imports)
🔇 Additional comments (15)
packages/query-core/src/query.ts (1)
205-221: Correct propagation of queryKeyHashFn changes (constructor-safe).Capturing oldOptions before merge and notifying the cache only when oldOptions exists avoids touching the cache during construction (when #cache isn’t initialized). The before/after diff on queryKeyHashFn is correct and will keep QueryCache.#knownHashFns in sync. Nicely done.
packages/query-core/src/__tests__/queryCache.test.tsx (1)
296-371: Good, targeted coverage for prefix-indexed findAll path.This verifies that only prefix candidates are visited by the predicate and that results are correct across exact match, primitive suffixes, and mixed suffixes. This gives confidence the trie walk doesn’t over-visit.
packages/query-core/src/queryCache.ts (13)
533-544: Public API for hashFn change tracking is minimal and correct.Refcount updates on before/after ensure the known set reflects active queries. This keeps hashed lookup robust when setOptions flips hash functions at runtime.
354-367: Index maintenance on add is correct; mirrors removal path.Adding to the trie and registering the current hashFn give the new lookup paths the needed data. This aligns with remove() and ensures GC is considered.
395-401: Balanced teardown on remove.You remove the query from #queries, the trie, and decrement the hashFn refcount. This prevents stale index entries and keeps known hashFns clean.
1-14: Imports align with the new design.Bringing in hashKey and QueryKeyHashFunction is required for hashed lookups. Good separation of types vs. values.
456-472: Filter fast-paths are sensible.
- exact + queryKey → findExact
- queryKey → trie prefix candidates
- else → fallback over all
This retains compatibility while optimizing common paths.
266-325: Depth-first iterator is sound; micro-optimizations considered.The “single source” early-return avoids generator creation when possible; the DFS order is deterministic. Good balance between perf and simplicity.
191-214: GC logic is correct and bounded.Bottom-up pruning stops at the first node that still holds data. This will keep the trie compact under churn.
215-242: Insertion path handles non-primitives correctly.Storing non-primitive suffix matches at the last primitive node avoids impossible lookups and enables correct prefix enumeration.
244-259: Removal mirrors insertion semantics.The nonPrimitiveSuffix branch correctly tests the first non-primitive at path.length - 1 and invokes GC; symmetrical with add().
121-142: Primitive guard is precise.Covers null/undefined and the JS primitive types explicitly. Clear and efficient.
432-435: Default exact=true for find is intentional and preserved.Matches existing external expectations for find(). With the fix above to pass defaulted filters, this remains consistent.
474-507: Sanity: try/catch around custom hash functions is necessary.This shields findExact from userland hashFn exceptions and prevents false negatives leaking errors into the public API. Good guard.
474-507: Clarify support for user-supplied queryHash infindExact
ThefindExactmethod only applies registered hash functions tofilters.queryKeyand never checks a user-providedqueryHash. SinceQueryOptionsincludes an optionalqueryHash?: stringthat callers can override, any bespoke hash that doesn’t match one of the known functions will be ignored (and the query won’t be found).Please decide how you’d like to handle this:
- If you intend to support direct overrides of
queryHash, add a fallback lookup—e.g. before or after the existing loop, trythis.get(filters.queryHash)whenfilters.queryHashis defined.- Otherwise, document in
findExact’s JSDoc (and in the public API docs) that only hashes produced by the built-in functions are supported.Let me know which approach you prefer so we can either implement the fallback or update the documentation accordingly.
|
Hmm the trie thing as implemented will change QueryCache from iterating in insertion order to iterating in a somewhat arbitrary order. For absolute correctness when iterating a prefix, we need to sort the prefix nodes into insertion order before yielding them. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/query-core/src/queryCache.ts (1)
619-652: Enforce exact semantics inside findExact to prevent mismatchesEven though callers currently pass
exact: true, this private helper should be self-defensive. If it were ever called withexactunset, a hash collision or misbehaving custom hash could slip through via non-exactmatchQuery.Apply this diff:
>( - filters: QueryFilters<any>, + filters: QueryFilters<any>, ): Query<TQueryFnData, TError, TData> | undefined { - const tryHashFn = (hashFn: QueryKeyHashFunction<any>) => { + // Force exact confirmation for hashed lookups. + const exactFilters = filters.exact ? filters : { ...filters, exact: true } + const tryHashFn = (hashFn: QueryKeyHashFunction<any>) => { try { - const query = this.get(hashFn(filters.queryKey)) - if (query && matchQuery(filters, query)) { + const query = this.get(hashFn(exactFilters.queryKey)) + if (query && matchQuery(exactFilters, query)) { // Confirmed the query actually uses the hash function we tried // and matches the non-queryKey filters return query } else { return undefined } } catch (error) { return undefined } }
🧹 Nitpick comments (3)
packages/query-core/src/queryCache.ts (3)
248-253: Simplify object-key matching; avoid array wrappersWrapping keys into single-element arrays before calling
partialMatchKeyis unnecessary and allocates. A direct object-to-object check is equivalent for this purpose.Apply this diff:
- const keyArray = [key] - for (const candidateKey of this.#objectMap.keys()) { - if (partialMatchKey([candidateKey], keyArray)) { + for (const candidateKey of this.#objectMap.keys()) { + if (partialMatchKey(candidateKey, key)) { return candidateKey } }
443-447: Fix wording in iteratePrefix docs: “suffix” → “prefix”The implementation traverses to the node for the provided key and enumerates its subtree; i.e., returned values have the provided key as a prefix, not a suffix.
Apply this diff:
- * Or the value's key is a suffix of the given key and contains a non-primitive key. + * Or the value's key extends the given key (i.e., has it as a prefix) and may contain non-primitive segments.
214-234: Optional: return an empty iterator instead ofundefined
values()returningundefinedwhen empty complicates callers with extra guards. Consider always returning an iterator (empty when there are no values) to matchMap.prototype.values()semantics and simplify usage.Minimal change:
- values(): Iterable<TValue> | undefined { - if (!this.#primitiveMap.size && !this.#objectMap.size) { - return undefined - } + values(): Iterable<TValue> { + if (!this.#primitiveMap.size && !this.#objectMap.size) { + // Empty iterator + return (function* () { /* noop */ })() + }Follow-ups: callers like
iterateSubtreeValueNodescan dropif (!children) { return }and iterate directly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/query-core/src/queryCache.ts(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/query-core/src/queryCache.ts (3)
packages/query-core/src/utils.ts (4)
partialMatchKey(232-246)matchQuery(126-172)QueryFilters(18-43)hashKey(215-226)packages/query-core/src/types.ts (2)
QueryKey(53-61)QueryKeyHashFunction(186-188)packages/query-core/src/query.ts (1)
Query(159-689)
🔇 Additional comments (1)
packages/query-core/src/queryCache.ts (1)
578-594: Sanity-check: preserving find() semanticsYou default
exacttotrueand route tofindExact. This matches historical behavior forfind(exact by default), but it’s easy to regress. Consider adding/retaining a unit test that assertsfind({ queryKey: ['k'] })is exact, whilefindAlldefaults to prefix/partial behavior.
| switch (t) { | ||
| case 'object': | ||
| case 'function': | ||
| return false | ||
| case 'string': | ||
| case 'number': | ||
| case 'boolean': | ||
| case 'bigint': | ||
| case 'symbol': | ||
| case 'undefined': | ||
| return true | ||
| default: | ||
| t satisfies never | ||
| return false | ||
| } |
There was a problem hiding this comment.
Remove invalid satisfies never; it will not compile here
t is a runtime string (result of typeof value), so t satisfies never is a type error in this position and breaks compilation. Default branch should just return false.
Apply this diff:
case 'undefined':
return true
default:
- t satisfies never
return false
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| switch (t) { | |
| case 'object': | |
| case 'function': | |
| return false | |
| case 'string': | |
| case 'number': | |
| case 'boolean': | |
| case 'bigint': | |
| case 'symbol': | |
| case 'undefined': | |
| return true | |
| default: | |
| t satisfies never | |
| return false | |
| } | |
| switch (t) { | |
| case 'object': | |
| case 'function': | |
| return false | |
| case 'string': | |
| case 'number': | |
| case 'boolean': | |
| case 'bigint': | |
| case 'symbol': | |
| case 'undefined': | |
| return true | |
| default: | |
| return false | |
| } |
🤖 Prompt for AI Agents
In packages/query-core/src/queryCache.ts around lines 132 to 146, remove the
invalid TypeScript expression "t satisfies never" from the default case and
simply return false there; since t is a runtime string from typeof, the
satisfies never is a compile-time type assertion that doesn't belong in
executable code — replace that line with a plain return false so the default
branch compiles cleanly.
There was a problem hiding this comment.
it typechecks fine and verifies statically that we handled all the typeof variants
There was a problem hiding this comment.
@justjake You're absolutely right, and I apologize for the incorrect assessment. The t satisfies never pattern is indeed a valid TypeScript idiom for exhaustiveness checking.
Since all possible return values of typeof are handled in the switch cases ("string" | "number" | "bigint" | "boolean" | "symbol" | "undefined" | "object" | "function"), the type of t in the default case should be never. The t satisfies never assertion provides compile-time verification that all variants are covered, which is exactly what you intended.
This is good defensive programming - if someone later adds a new case to handle but forgets to update the switch, TypeScript will catch it at compile time. Thank you for the clarification!
🐰💡 curious ears perk up, learning something new
✏️ Learnings added
Learnt from: justjake
PR: TanStack/query#9589
File: packages/query-core/src/queryCache.ts:132-146
Timestamp: 2025-08-25T19:16:59.321Z
Learning: The TypeScript pattern `t satisfies never` in a switch statement's default case is valid for exhaustiveness checking when all possible values of a union type are handled in the switch cases. For `typeof` operations, this ensures all 8 possible return values are covered: "string", "number", "bigint", "boolean", "symbol", "undefined", "object", "function".
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
Simplified the trie logic and guaranteed that iterating with a partial query key follows the same order as before this change, or when iterating with just a predicate. |
|
i discussed the trie-of-maps approach w/ @sophiebits and she pointed out that React uses trie-of-maps to implement its cache https://github.com/facebook/react/blob/0a1f1fcd5080320139bb51021b4325be65d6e2bd/packages/react/src/ReactCacheImpl.js#L74-L106 |
Fixes #9588
{ queryKey: [...], exact: true }filters by trying to look up the query by hashingfilters.queryKeywith all known hash functions, improving time spent from O(queries) to O(hash functions) which is usually O(1).{ queryKey: [...] }filters by indexing the queries by their queryKey in aMap-based prefix trie. Lookups in the trie have equivalent order and matching semantics as though the user's filters ran onqueryCache.getAll().filter(query => partialMatchKey(query.queryKey, filters.queryKey))instead of on all queries. This improves time spent from O(queries) to O(filter.queryKey length + queriesInPrefix + logN(queriesInPrefix). ThelogN(queriesInPrefix)is spent sorting the queries to ensure they're visited by the user's filter in insertion order.predicatefunction are likely safe to re-order arbitrarily as long as we return the results in the expected order; then if the filter returns 0 or 1 results sorting is skipped entirely.Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests