feat: lazy loading + instance-level caching for SchemaRegistry#16
feat: lazy loading + instance-level caching for SchemaRegistry#16
Conversation
There was a problem hiding this comment.
Pull request overview
Introduces lazy, per-index schema loading with instance-level caching in SchemaRegistry (and async equivalents) to eliminate repeated FT._LIST/FT.INFO overhead per SQL query, plus adds tests and a benchmark script to validate/measure the impact.
Changes:
- Make
SchemaRegistry.get_schema()lazily fetch schema viaFT.INFOon cache miss and reuse cached results;get_field_type()now goes throughget_schema(). - Add cache invalidation via
invalidate(index|None)for sync and async registries; addAsyncSchemaRegistry.ensure_schema()for async lazy loading. - Add integration tests for schema caching behavior and a benchmark script to measure Redis command counts/latency across modes.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 12 comments.
| File | Description |
|---|---|
sql_redis/schema.py |
Implements lazy schema fetch + caching, adds invalidation, and adds async ensure_schema() path. |
tests/test_schema_caching.py |
New integration tests validating lazy load + caching + invalidation behavior (sync). |
benchmarks/bench_schema_caching.py |
New benchmark to compare command counts/latency across eager/lazy/cached schema-loading modes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
sql_redis/schema.py:122
- SchemaRegistry.refresh() docstring says a missing index is removed from the registry, but _load_index_schema() now negative-caches missing indexes as an empty dict. Update the docstring (or behavior) so callers don’t assume the key is removed when the index is gone.
def refresh(self, index_name: str) -> None:
"""Refresh schema for a single index.
If the index no longer exists, removes it from the registry.
If the index is new, adds it to the registry.
"""
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3afae2b6b0
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3afae2b6b0
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
sql_redis/schema.py:125
SchemaRegistry.refresh()docstring says missing indexes are removed from the registry, but_load_index_schema()now negative-caches missing indexes by storing an empty dict. Either update the docstring to match the new behavior, or adjustrefresh()/_load_index_schema()so refresh of a deleted index removes it rather than leaving a cached{}entry.
"""Refresh schema for a single index.
If the index no longer exists, removes it from the registry.
If the index is new, adds it to the registry.
"""
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb7ae4ca74
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- get_schema() now fetches schema on first access via FT.INFO and caches it - Eliminates FT._LIST calls entirely from the SQL query path - Add invalidate() method for cache clearing (all or per-index) - Async support via ensure_schema() in AsyncSchemaRegistry - 14 integration tests covering caching, invalidation, and lazy loading
- Measures real Redis round-trips via CommandCounter wrapper - 4 modes: current redisvl, load_all baseline, lazy no-cache, lazy+cached - Configurable via CLI: --queries, --redis-url, --bg-indexes, --runs - Generates comparison charts with matplotlib (optional)
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- Add negative caching for missing indexes (sync + async): cache {} on
ResponseError to avoid repeated FT.INFO calls for non-existent indexes
- Add async concurrency guard in ensure_schema(): use _loading task map
to deduplicate concurrent FT.INFO requests for the same index
- Cancel in-flight tasks on invalidate(), load_all(), and refresh()
- Fix benchmark Executor reuse inconsistency in run_load_all()
- Fix docstring mode number mismatches in benchmark runners
- Fix ImportError message to cover matplotlib/numpy
- Add comprehensive async integration tests for ensure_schema(),
invalidate(), negative caching, and concurrency guard
… safe cleanup
- Wire AsyncExecutor.execute() to call ensure_schema() before translation,
so async path works without requiring load_all() upfront (matches sync behavior)
- Narrow negative-cache in _load_index_schema (sync + async) to only cache {}
when error contains 'no such index'; re-raise other ResponseErrors
- Limit benchmark setup_redis() cleanup to bench_*/bg_index_* prefixes
instead of dropping all indexes on the server
- Replace bare Exception catches in async test fixtures with redis.ResponseError
- Add async_caching_data fixture and TestAsyncLazySchemaWithExecutor tests
- Handle asyncio.CancelledError in ensure_schema() so invalidate() during an active ensure_schema() returns post-invalidate cache state instead of propagating CancelledError to callers - Only remove task from _loading if it's still the current one - Add Translator.translate_parsed(ParsedQuery) to accept pre-parsed queries - Wire AsyncExecutor.execute() to parse once and use translate_parsed(), eliminating redundant sqlglot parsing per async query - Remove unused SQLParser import from executor.py
… parse() API - ensure_schema(): check task.cancelled() to distinguish between internal task cancellation (from invalidate/refresh) and caller cancellation (e.g., asyncio.wait_for timeout). Only swallow internal cancellations; re-raise caller-driven CancelledError so callers abort properly. - process_pending_events(): exclude negative-cached (empty) schemas from cached_indexes set so they don't trigger spurious 'dropped' events or mask newly created indexes from being detected. - Add Translator.parse(sql) as public API for pre-parsing SQL, replacing private _parser access in AsyncExecutor.
- ensure_schema(): wrap shared task await with asyncio.shield() so caller cancellation (e.g. asyncio.wait_for timeout) doesn't cancel the shared FT.INFO task for other awaiters. invalidate()/refresh() still cancel via task.cancel() directly. - Update PR docs to reflect translator/executor changes: Translator.parse(), translate_parsed(), AsyncExecutor auto lazy-load, updated test counts.
Both sync and async _load_index_schema() now match 'unknown index' in addition to 'no such index' when deciding to negative-cache a missing index. Defensive against Redis version differences in FT.INFO error messages.
- Benchmark p95/p99: use statistics.quantiles() instead of manual index calculation that was off-by-one for small sample sizes - process_pending_events(): decode FT._LIST bytes to str before comparing with _schemas keys, consistent with load_all(). Fixes incorrect new/deleted detection when decode_responses=False.
… cancel
- Update sync and async refresh() docstrings to reflect that missing
indexes are now negative-cached as {} rather than removed.
- ensure_schema() finally block: only remove task from _loading if
task.done(), so caller cancellation doesn't break the shared in-flight
task for other awaiters when asyncio.shield() keeps it running.
…ndex, inclusive quantiles
- ensure_schema(): attach done-callback to clean up _loading when all
awaiters cancel and no finally block runs. Prevents leaked task entries.
- translate_parsed(): raise ValueError('Unknown index: ...') early when
get_schema() returns empty dict (negative cache), instead of deferring
the error to Redis execution time.
- Benchmark p95/p99: use method='inclusive' so statistics.quantiles()
works for small sample sizes (n < 100).
- Missing indexes are no longer cached as {}; each access retries FT.INFO,
allowing automatic recovery when an index is created after first miss.
- Replaced asyncio.ensure_future() with asyncio.create_task() to match
dict[str, asyncio.Task] annotation.
- Updated tests: TestMissingIndexRecovery verifies transient miss recovery
including late-created indexes.
- Cleaned up docstrings, comments, and PR notes.
19fb77b to
4aa968e
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
rbs333
left a comment
There was a problem hiding this comment.
This looks good to me! Only thing I would consider adding is a per query view to the benchmark since that contextualizes well but generally awesome
Problem
Every SQL query executed through
redisvl'sSearchIndex.query(SQLQuery(...))creates a newSchemaRegistryand callsload_all(), which issues:FT._LIST— enumerate all indexes on the serverFT.INFO— for every single index on the serverWith 10 indexes on the server, that's 12 Redis round-trips of pure overhead per query before the actual
FT.SEARCHorFT.AGGREGATEis even sent. At 1,000 queries, that's 12,000 wasted round-trips. At 10,000 queries, it's 120,000.On localhost this adds milliseconds. On cloud deployments with 1ms RTT, it adds seconds. Cross-region at 5ms RTT, it adds minutes.
The schema almost never changes between queries — this overhead is almost entirely waste.
Solution
Core: lazy loading + instance-level caching
get_schema()now loads on demand. Instead of requiringload_all()upfront,get_schema(index)issues a singleFT.INFOcall on first access for a given index and caches the result. Subsequent calls return the cached schema with zero I/O. This eliminatesFT._LISTentirely — we only fetch the schema for the index being queried, not all indexes on the server.invalidate()clears cached schemas (all or per-index), forcing a re-fetch on next access. Provides an escape hatch for stale schema scenarios without re-introducing the overhead.Negative caching. If
FT.INFOreturns "no such index" or "unknown index", the result is cached as{}to avoid repeated round-trips for missing indexes. The watcher (process_pending_events()) excludes these negative-cached entries from its comparisons to prevent spurious "dropped" events.Async:
ensure_schema()with concurrency guardAsyncSchemaRegistry.ensure_schema()provides the async lazy-load path:FT.INFOtask via_loadingdictasyncio.shield()so caller cancellation (e.g.asyncio.wait_fortimeout) doesn't cancel the shared load for other awaiters.invalidate()/refresh()/load_all()still cancel tasks directly viatask.cancel()shield(), if the shared task isn't cancelled butCancelledErroris raised, it's re-raised to properly abort the caller_loadingeven if all awaiters cancel (nofinallyblock runs). Thefinallyblock handles the common case; the callback is the backstoptranslate_parsed()raisesValueError("Unknown index: ...")whenget_schema()returns an empty dict, instead of deferring the error to Redis execution timeTranslator/executor changes
Translator.parse(sql)andTranslator.translate_parsed(parsed)— public methods that allow callers to parse SQL once and reuse the AST.translate(sql)delegates to these internally. No breaking changes.AsyncExecutor.execute()callsensure_schema()automatically before translation, usingparse()+translate_parsed()to avoid double-parsing. Async lazy-loading works without requiringload_all()upfront, matching sync behavior.What did NOT change
load_all()still works identically for callers that need itparser.pyoranalyzer.pyUsage
Sync — lazy loading with caching (recommended)
Async — lazy loading with caching
Backward compatible — load_all() still works
Benchmark Results
Setup
bench_products) with 5 fields and 10 documentsFour modes compared
SchemaRegistry+load_all()per query (current production behavior)load_all()once upfront, reuse registryget_schema()but new registry per queryget_schema()(target behavior)Results at 100 / 1,000 / 10,000 queries (11 total indexes)
Prediction validation (5,000 queries, 10 total indexes)
60,000 down to 5,001 round-trips (12x reduction). Wall-clock time drops from 11.6s to 1.9s (6.3x faster on localhost).
Per-query overhead
FT._LIST+ N ×FT.INFO+ queryFT.INFO+ queryFT.INFOtotal)Running the benchmark
Limitations
invalidate()must be called explicitly.