DEV-1587: per-engine in-memory query result cache#222
Conversation
Opt-in per-call via execute(query, cache=True); per-engine, in-memory. TTL (lazy on read) + Cube-style refresh keys scanned by engine.refresh(). Adds evict/clear_cache/cache_size and sync wrappers. Refactors execute into _normalize_input + DB-free _prepare_pipeline shared by execute, evict, and refresh re-execution. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…er-query-cache-in-slayer # Conflicts: # CLAUDE.md # mkdocs.yml # slayer/engine/query_engine.py
Runnable, self-contained notebook demonstrating miss/hit, staleness, refresh keys, TTL, and eviction on a temp SQLite database. Wire it into the Tutorials nav; cross-link with the concept doc. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR adds an opt-in, per- ChangesQuery Cache Feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Engine as SlayerQueryEngine
participant Cache as QueryCache
participant DB as Data source
Caller->>Engine: execute(query, cache=True)
Engine->>Engine: _prepare_pipeline (SQL + ds_fingerprint)
Engine->>Cache: get(make_key(sql, ds_fingerprint))
alt cache hit
Cache-->>Engine: cached entry
Engine-->>Caller: deep-copied response
else cache miss
Engine->>DB: scan refresh-key baselines
Engine->>DB: execute SQL
DB-->>Engine: result
Engine->>Cache: put(key, new entry)
Engine-->>Caller: response
end
sequenceDiagram
participant Caller
participant Engine as SlayerQueryEngine
participant Cache as QueryCache
participant DB as Data source
Caller->>Engine: refresh()
Engine->>Cache: snapshot()
Engine->>DB: scan refresh-key expressions
DB-->>Engine: current values
alt TTL expired or value changed
Engine->>Engine: _refresh_reexec
Engine->>DB: re-execute SQL
Engine->>Cache: commit_replace(new entry)
else unchanged
Engine-->>Caller: unchanged
end
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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 (4)
tests/test_query_cache.py (1)
596-602: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBroad
pytest.raises(Exception)— acceptable given unknown SQL client error type.SonarCloud flags the wildcard exception type. Since the exact exception raised by the SQL client on a bad column reference isn't guaranteed/stable across dialects, narrowing this could make the test brittle; only worth tightening if the client exposes a dedicated exception type.
🤖 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 `@tests/test_query_cache.py` around lines 596 - 602, The test around _build_engine and engine.execute is using a wildcard pytest.raises(Exception), which SonarCloud flags. Tighten the assertion to the most specific exception type exposed by the SQL client for an invalid column reference, and update the surrounding failure setup in the cache refresh-key test accordingly; if no stable dedicated type exists, keep the test behavior focused on the engine.execute failure while avoiding an unnecessary broad catch.Source: Linters/SAST tools
docs/concepts/query-cache.md (1)
67-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the write-time scan-failure contract.
CLAUDE.md notes that a baseline-scan failure on a cache miss propagates out of
execute(cache=True)(nothing stored); the same failure atrefresh()time is continue-on-error instead. This asymmetry (miss raises, refresh swallows) is user-facing and surprising — worth adding to the "Staleness" section so users configuringrefresh_keysknow a typo'd table/expression can raise on the very first cached call.🤖 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 `@docs/concepts/query-cache.md` around lines 67 - 91, The Staleness section in the query cache docs should explicitly document the write-time baseline-scan failure behavior for refresh keys. Update the explanation around execute(cache=True) and engine.refresh() to note that a scan failure on a cache miss propagates and prevents storing the entry, while the same failure during refresh is handled as continue-on-error. Keep this tied to the refresh_keys and engine.refresh() wording so users understand that bad table names or expressions can raise on the first cached call.slayer/engine/cache.py (1)
269-279: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPass multi-arg helpers by keyword (repo convention).
Several internal calls here and elsewhere in this module use positional args for multi-parameter functions (Lines 240, 250, 273, 276, 277). The engine layer already standardizes on keyword-only signatures (e.g.
commit_replace(self, *, ...)), so consider aligning these helpers/calls for consistency. Notemake_keyis also invoked positionally fromquery_engine.py, so making its signature keyword-only requires updating that caller.As per coding guidelines: "Use keyword arguments for functions with more than one parameter."
🤖 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 `@slayer/engine/cache.py` around lines 269 - 279, The module still uses positional arguments for multi-parameter helper calls, which is inconsistent with the repo’s keyword-argument convention. Update the relevant helper signatures in cache.py, especially make_key and any similar multi-arg methods used around applicable_keys, to require keyword-only arguments where appropriate, and change all internal call sites in this module to pass those parameters by name. If make_key is made keyword-only, also update its positional caller in query_engine.py so the call matches the new signature.Source: Coding guidelines
slayer/engine/query_engine.py (1)
971-1025: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSonarCloud gate is failing on this function (cognitive complexity 28 > 15).
The per-alias metadata build stacks six near-identical
for … if alias in public_aliases …blocks. Extracting a small helper that maps an enriched field kind →FieldMetadata(and reusing it across dimensions/time-dims/measures/expressions/transforms/cross-model) would collapse the branching and clear the gate.🤖 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 `@slayer/engine/query_engine.py` around lines 971 - 1025, The _build_response_metadata function is too cognitively complex because it repeats nearly identical alias-filtered loops for dimensions, time_dimensions, measures, expressions, transforms, and cross_model_measures. Refactor this by extracting a small helper (or a couple of helpers) that takes an enriched field collection and returns the matching FieldMetadata keyed by alias, then reuse it inside _build_response_metadata. Keep the existing public_aliases filtering and format inference logic intact while collapsing the branching and duplication so the function passes the SonarCloud complexity gate.Source: Pipeline failures
🤖 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 `@slayer/engine/cache.py`:
- Around line 232-240: The filtering in parse_referenced_tables() is incorrectly
dropping exp.Table entries by comparing bare .name against CTE/subquery aliases,
which can skip real tables that happen to share wrapper names like _inner or
_outer. Update the logic in cache.py to distinguish wrapper aliases from
physical tables, or match on normalized table identity instead of bare names, so
only actual CTE/subquery references are excluded. Keep the change localized to
parse_referenced_tables() and its excluded/exp.Table handling.
In `@slayer/engine/query_engine.py`:
- Around line 903-909: The _prepare_pipeline docstring claims the flow is
DB-free, but _apply_policy can still trigger live database access through
_column_present when data_filters are configured. Update _prepare_pipeline
and/or _apply_policy so policy evaluation is skipped on DB-free paths like evict
and refresh, or change the docstring to reflect the database check; use the
_prepare_pipeline, _apply_policy, and _column_present symbols to keep the
behavior and documentation consistent.
---
Nitpick comments:
In `@docs/concepts/query-cache.md`:
- Around line 67-91: The Staleness section in the query cache docs should
explicitly document the write-time baseline-scan failure behavior for refresh
keys. Update the explanation around execute(cache=True) and engine.refresh() to
note that a scan failure on a cache miss propagates and prevents storing the
entry, while the same failure during refresh is handled as continue-on-error.
Keep this tied to the refresh_keys and engine.refresh() wording so users
understand that bad table names or expressions can raise on the first cached
call.
In `@slayer/engine/cache.py`:
- Around line 269-279: The module still uses positional arguments for
multi-parameter helper calls, which is inconsistent with the repo’s
keyword-argument convention. Update the relevant helper signatures in cache.py,
especially make_key and any similar multi-arg methods used around
applicable_keys, to require keyword-only arguments where appropriate, and change
all internal call sites in this module to pass those parameters by name. If
make_key is made keyword-only, also update its positional caller in
query_engine.py so the call matches the new signature.
In `@slayer/engine/query_engine.py`:
- Around line 971-1025: The _build_response_metadata function is too cognitively
complex because it repeats nearly identical alias-filtered loops for dimensions,
time_dimensions, measures, expressions, transforms, and cross_model_measures.
Refactor this by extracting a small helper (or a couple of helpers) that takes
an enriched field collection and returns the matching FieldMetadata keyed by
alias, then reuse it inside _build_response_metadata. Keep the existing
public_aliases filtering and format inference logic intact while collapsing the
branching and duplication so the function passes the SonarCloud complexity gate.
In `@tests/test_query_cache.py`:
- Around line 596-602: The test around _build_engine and engine.execute is using
a wildcard pytest.raises(Exception), which SonarCloud flags. Tighten the
assertion to the most specific exception type exposed by the SQL client for an
invalid column reference, and update the surrounding failure setup in the cache
refresh-key test accordingly; if no stable dedicated type exists, keep the test
behavior focused on the engine.execute failure while avoiding an unnecessary
broad catch.
🪄 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: CHILL
Plan: Pro
Run ID: ee9b52f7-641e-4ef1-b21b-773a07485c6d
📒 Files selected for processing (7)
CLAUDE.mddocs/concepts/query-cache.mddocs/examples/12_query_cache/query_cache_nb.ipynbslayer/engine/cache.pyslayer/engine/query_engine.pytests/test_query_cache.pyzensical.toml
…ig, scope-based table detection - Apply the forced-filter policy to refresh-key scans so the baseline is computed over the same tenant-scoped rows as the cached data query (Codex): thread datasource through _scan_refresh_keys/_scan_one_table. - Freeze CacheConfig + coerce refresh_keys to a tuple so in-place mutation can't silently bypass the cache-clearing setter (Codex). - Detect physical tables via sqlglot traverse_scope instead of excluding by bare name, so a table sharing a CTE alias name is still matched (CodeRabbit). - Correct the _prepare_pipeline / evict 'DB-free' docstrings: under a policy, column-presence introspection runs (CodeRabbit). - Tests: pytest.approx for float sums (Sonar S1244), narrow + hoist the baseline-failure exception assertion (Sonar S5778/S5958), NOSONAR S3776 on _build_response_metadata; add RLS-scoped-scan and frozen-config tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A schema/catalog-qualified table can never be a CTE (CTE names are unqualified), so only bare names need the scope-shadow check. Fixes a physical table sharing a CTE's bare name (e.g. public.cte) being skipped (Codex follow-up). Add a regression test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A cache entry is bound to the datasource it originally resolved against (part of the key). refresh() already scans that datasource's refresh keys, so re-execution must pin to it via entry.resolved_data_source rather than re-resolving through a possibly-changed priority list and migrating the entry to a different datasource (Codex follow-up). Add a priority-drift regression test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
1 participant
Summary
Adds an opt-in, per-
SlayerQueryEngine, in-memory query result cache modelled on Cube's in-memory cache. Python-API-only (no REST / MCP / CLI / Flight / pg-facade /SlayerClientsurface).execute(query, cache=True)/execute_sync(query, cache=True, data_source=...)— serve from / store in the cache for any input shape (SlayerQuery/ dict / multi-stage list / run-by-name str). Ignored fordry_run/explain.CacheConfig(ttl_seconds=..., refresh_keys=[(table, select_expression), ...])on the engine constructor; thecache_configsetter clears the cache.engine.refresh(). Baselines captured before the data query on a miss.evict/evict_sync(DB-free, never opens a client),clear_cache,cache_size,refresh/refresh_syncreturning a bucketedRefreshResult.New module
slayer/engine/cache.py(QueryCache,CacheConfig,RefreshResult,RefreshError,RefreshKeyValue,_CacheEntry— all Pydantic).execute()was refactored into_normalize_input+ a DB-free_prepare_pipelineshared by execute, evict, and refresh re-execution.Key design points
sha256(final_sql + "|" + connection_string + "|" + runtime_fingerprint)— datasource identity, not the bare name.(catalog, db, name)with dialect folding, and intersects with configured refresh-key tables (wildcard-matched).refresh()re-prepares each stale entry from its original input (picks up model /source_queriesedits), re-keys on SQL change, resets TTL, and commits identity-guarded so a concurrent evict/clear/newer-execute is never clobbered or resurrected. Continue-on-failure.asyncio.Lockguards only in-memory dict ops; every DB await is outside the lock. Deep-copy on store and on hit-return (and onoriginal_input/variables) so callers can't poison the cache.Docs
docs/concepts/query-cache.md(concept reference) +CLAUDE.md"Query cache" section, wired intozensical.tomlnav.docs/examples/12_query_cache/query_cache_nb.ipynbwalking through miss → hit, staleness,refresh(), TTL, and eviction.Tests
tests/test_query_cache.py— 42 cases: pureQueryCache(key stability, TTL, CTE-excluding table parse, cross-dialect wildcard matching, scan-SQL building, identity-guarded commit) and real in-process SQLite engine integration (miss→hit serves stale, TTL lazy re-exec, refresh buckets, multi-key-per-table, unreferenced-table no-op, re-prepare-from-input + re-key, continue-on-error, write-time baseline propagation, mutation isolation, per-engine isolation, multi-stage + run-by-name, sync wrappers, evict-never-connects, identity-guarded refresh commit).Full non-integration suite green (6492 passed) after merging
main(which brought RLS / session-policy — the policy SQL-rewrite composes cleanly with the cache).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation