Conversation
🛡️ Jit Security Scan Results✅ No security findings were detected in this PR
Security scan by Jit
|
There was a problem hiding this comment.
Pull request overview
This PR updates RedisVL’s SQLQuery execution path to rely on sql-redis factory functions for executor/schema-registry construction, adds per-query sql_redis_options passthrough (notably schema cache strategy), and introduces executor caching + cache invalidation hooks across index lifecycle operations.
Changes:
- Add
sql_redis_optionstoSQLQueryand forward options intosql-redisexecutor creation (defaulting to"lazy"schema caching). - Cache
sql-redisexecutors insideSearchIndex/AsyncSearchIndex, keyed by normalized options, and invalidate the cache on connect/disconnect/create/delete/clear. - Add integration tests and documentation updates describing schema cache strategy behavior and the new option passthrough.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
redisvl/query/sql.py |
Adds sql_redis_options to SQLQuery and switches redis_query_string() to a factory-created executor path. |
redisvl/index/index.py |
Adds executor caching for SQL execution, plus cache invalidation on lifecycle operations for sync/async indexes. |
tests/integration/test_sql_redis_hash.py |
Adds integration coverage for default vs eager schema caching and cache invalidation (sync). |
tests/integration/test_sql_redis_json.py |
Adds integration coverage for default vs eager schema caching and cache invalidation (async). |
docs/user_guide/12_sql_to_redis_queries.ipynb |
Documents sql_redis_options usage in the SQL user guide notebook. |
docs/concepts/queries.md |
Documents sql_redis_options and schema_cache_strategy semantics in the concepts guide. |
docs/api/query.rst |
Adds API docs note describing sql_redis_options and common cache strategies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0f4a5c6. Configure here.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sql_redis_options = { | ||
| "schema_cache_strategy": "lazy", | ||
| **self.sql_redis_options, | ||
| } | ||
| executor = create_executor(redis_client, **sql_redis_options) | ||
|
|
||
| # Substitute non-bytes params in SQL before translation | ||
| sql = self._substitute_params(self.sql, self.params) | ||
|
|
||
| translated = translator.translate(sql) | ||
| translated = executor._translator.translate(sql) | ||
| return translated.to_command_string() |
There was a problem hiding this comment.
SQLQuery.redis_query_string() reaches into executor._translator (a private sql-redis attribute) to perform translation. This couples RedisVL to sql-redis internals and is likely to break on upstream refactors; prefer calling a public translate()/to_command_string() API on the executor (or add one upstream) instead of accessing a private field.
| Use ``=`` for exact phrase matching, ``LIKE`` for wildcard | ||
| matching, ``fuzzy()`` for typo-tolerant matching, and | ||
| ``fulltext()`` for tokenized search. |
There was a problem hiding this comment.
The docstring is slightly inconsistent about the = operator: above it says = does “exact phrase or exact-term matching”, but the later Note says to use = for “exact phrase matching” only. Consider updating the Note to include exact-term matching as well so users aren’t misled.
| Use ``=`` for exact phrase matching, ``LIKE`` for wildcard | |
| matching, ``fuzzy()`` for typo-tolerant matching, and | |
| ``fulltext()`` for tokenized search. | |
| Use ``=`` for exact phrase or exact-term matching, ``LIKE`` | |
| for wildcard matching, ``fuzzy()`` for typo-tolerant | |
| matching, and ``fulltext()`` for tokenized search. |
| def _sql_executor_cache_key(sql_redis_options: Dict[str, Any]) -> str: | ||
| """Build a stable cache key for sql-redis executor reuse.""" | ||
| return json.dumps(sql_redis_options, sort_keys=True, default=repr) |
There was a problem hiding this comment.
_sql_executor_cache_key() uses json.dumps(..., default=repr) to serialize options. repr() can include non-deterministic data (e.g., memory addresses) and can change across runs, which can cause unbounded cache growth or poor cache hit rates if non-JSON-serializable values are ever passed. Consider validating that sql_redis_options only contains JSON-serializable primitives (or explicitly normalizing known option types) and raising a clear error otherwise.
| def _sql_executor_cache_key(sql_redis_options: Dict[str, Any]) -> str: | |
| """Build a stable cache key for sql-redis executor reuse.""" | |
| return json.dumps(sql_redis_options, sort_keys=True, default=repr) | |
| def _normalize_sql_redis_option_value(value: Any) -> Any: | |
| """Normalize sql-redis options into deterministic JSON-serializable values.""" | |
| if value is None or isinstance(value, (str, int, float, bool)): | |
| return value | |
| if isinstance(value, dict): | |
| normalized: Dict[str, Any] = {} | |
| for key, item in value.items(): | |
| if not isinstance(key, str): | |
| raise TypeError( | |
| "sql_redis_options must use string keys for cache key generation; " | |
| f"got key of type {type(key).__name__}" | |
| ) | |
| normalized[key] = _normalize_sql_redis_option_value(item) | |
| return normalized | |
| if isinstance(value, (list, tuple)): | |
| return [_normalize_sql_redis_option_value(item) for item in value] | |
| raise TypeError( | |
| "sql_redis_options must contain only JSON-serializable primitive values " | |
| f"(nested dict/list/tuple structures are allowed); got value of type " | |
| f"{type(value).__name__}" | |
| ) | |
| def _sql_executor_cache_key(sql_redis_options: Dict[str, Any]) -> str: | |
| """Build a stable cache key for sql-redis executor reuse.""" | |
| normalized_options = _normalize_sql_redis_option_value(sql_redis_options) | |
| return json.dumps(normalized_options, sort_keys=True) |
| assert len(async_sql_index._sql_executors) == 1 | ||
| executor = next(iter(async_sql_index._sql_executors.values())) | ||
| assert async_sql_index.name in executor._schema_registry._schemas | ||
| assert other_index.name not in executor._schema_registry._schemas |
There was a problem hiding this comment.
These async tests assert on private sql-redis internals (executor._schema_registry._schemas and _sql_executors). This is likely to break when sql-redis refactors its implementation even if behavior is unchanged. Prefer asserting through a public sql-redis API or another stable observable (e.g., whether additional schema lookups occur) if available.

This PR updates SQLQuery execution to use a factory-based executor path and documents the new sql_redis_options behavior.
We chose the factory pattern so that sql-redis retains ownership of executor and schema-registry construction, rather than re-implementing that logic inside RedisVL. That keeps the boundary clean: RedisVL can pass configuration through, while sql-redis remains the source of truth for how schema registries are created, initialized, and evolved. This avoids duplicating registry logic in RedisVL and makes it easier for other applications to integrate with sql-redis without each client having to recreate that behavior themselves.
NOTE: this pr depends on a new release of sql-redis.
Note
Medium Risk
Changes SQL execution internals to use
sql-redisfactory executors with per-option caching and lifecycle invalidation, which could affect query behavior and resource usage. Also bumpssql-redisto>=0.4.0, so compatibility relies on the new upstream release.Overview
Updates
SQLQueryexecution to usesql-redis’screate_executor/create_async_executorfactory APIs and introduces per-index caching of executors keyed bysql_redis_options(defaulting toschema_cache_strategy="lazy"). Cached executors are now invalidated on index lifecycle events (connect/set_client, create/delete/clear, disconnect) to avoid stale schema state.Extends
SQLQueryto acceptsql_redis_optionspassthrough and documents new TEXT operator semantics forsql-redis >= 0.4.0(LIKE,fuzzy(),fulltext()), updating docs/notebook examples and integration tests accordingly. Dependency constraints are updated to requiresql-redis>=0.4.0(and lockfile refreshed).Reviewed by Cursor Bugbot for commit 439d6de. Bugbot is set up for automated code reviews on this repo. Configure here.