feat: add SQLQuery support to AsyncSearchIndex (#487)#521
Conversation
🛡️ Jit Security Scan Results✅ No security findings were detected in this PR
Security scan by Jit
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
There was a problem hiding this comment.
Pull request overview
Adds async parity with the sync SearchIndex.query() API by enabling SQLQuery execution through AsyncSearchIndex.query(), along with dependency and documentation updates.
Changes:
- Extend
AsyncSearchIndex.query()to accept and executeSQLQueryviasql-redisasync executor/registry. - Bump
sql-redisoptional dependency to>=0.2.0(and refresh lockfile). - Add integration tests and user-guide notebook updates demonstrating async SQL usage.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
redisvl/index/index.py |
Adds async SQL execution path and updates async query typing/docs to include SQLQuery. |
tests/integration/test_sql_redis_json.py |
Adds async integration coverage for SQL SELECT and aggregate COUNT queries. |
pyproject.toml |
Updates the sql-redis extra (and all) to require >=0.2.0. |
uv.lock |
Lockfile refresh to reflect dependency/version updates. |
docs/user_guide/12_sql_to_redis_queries.ipynb |
Adds an “Async Support” section demonstrating AsyncSearchIndex.query(SQLQuery(...)). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add async SQL query execution using sql-redis AsyncSchemaRegistry and AsyncExecutor - Update sql-redis dependency to >=0.2.0 - Add integration tests for async SQL SELECT and AGGREGATE queries
f3c7172 to
8a15f49
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 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.
| handles post-processing of the search. | ||
|
|
||
| Args: | ||
| query (Union[BaseQuery, AggregateQuery, HybridQuery]): The query to run. | ||
| query (Union[BaseQuery, AggregationQuery, HybridQuery]): The query to run. | ||
|
|
There was a problem hiding this comment.
The SearchIndex.query() docstring type annotation for query doesn’t include SQLQuery, but the method signature accepts it (and there’s an isinstance(query, SQLQuery) branch). Please update the docstring union/type description to include SQLQuery for accuracy.
| registry = AsyncSchemaRegistry(client) | ||
| await registry.load_all() # Loads index schemas from Redis asynchronously | ||
|
|
||
| executor = AsyncExecutor(client, registry) | ||
|
|
There was a problem hiding this comment.
AsyncSearchIndex._sql_query() builds a new AsyncSchemaRegistry, calls load_all(), and constructs a new AsyncExecutor on every SQL query. This adds extra Redis round-trips per query and can significantly reduce the concurrency/latency benefits this feature is aiming for. Consider caching the registry/executor on the index instance (lazy-init once) or loading only the current index schema if sql-redis supports it, with an explicit refresh path when needed.
| registry = AsyncSchemaRegistry(client) | |
| await registry.load_all() # Loads index schemas from Redis asynchronously | |
| executor = AsyncExecutor(client, registry) | |
| # Lazily initialize and cache the schema registry and executor to avoid | |
| # re-loading schemas and re-creating the executor on every SQL query. | |
| registry = getattr(self, "_sql_schema_registry", None) | |
| executor = getattr(self, "_sql_executor", None) | |
| cached_client = getattr(self, "_sql_client_for_sql", None) | |
| # (Re)create the registry/executor if they don't exist yet or if the | |
| # underlying client has changed. | |
| if registry is None or executor is None or cached_client is not client: | |
| registry = AsyncSchemaRegistry(client) | |
| # Loads index schemas from Redis asynchronously; do this once per | |
| # registry/client instead of once per query. | |
| await registry.load_all() | |
| executor = AsyncExecutor(client, registry) | |
| # Cache for reuse in subsequent SQL queries. | |
| self._sql_schema_registry = registry | |
| self._sql_executor = executor | |
| self._sql_client_for_sql = client |
| "# Create async index\n", | ||
| "async_index = AsyncSearchIndex.from_dict(schema, redis_url=\"redis://localhost:6379\")\n", | ||
| "\n", | ||
| "# Execute SQL query asynchronously\n", | ||
| "sql_query = SQLQuery(f\"SELECT user, age FROM {async_index.name} WHERE age > 30\")\n", | ||
| "results = await async_index.query(sql_query)\n", | ||
| "\n", |
There was a problem hiding this comment.
The new Async example creates an AsyncSearchIndex and immediately runs a SQL query, but it never calls await async_index.create(...) / loads data. As written, the snippet only works if the earlier sync section already created the index and loaded documents, which isn’t obvious when reading this section in isolation. Please either (a) add a short note that it reuses the previously-created index/data, or (b) include the minimal async setup steps so the section is self-contained.
rbs333
left a comment
There was a problem hiding this comment.
Tested locally - runs great 👍 Great add!
| decoded_rows.append(decoded_row) | ||
|
|
||
| return decoded_rows | ||
| return [convert_bytes(row) for row in result.rows] |
fixes #487
Validation:
Validate that the async SQL query implementation in RedisVL provides real concurrency benefits, not just an async wrapper around blocking calls.
Validation experiment
SearchIndex.query()in a loopawait AsyncSearchIndex.query()in a loopasyncio.gather()with multiple queriesResults
Outcome
Async sequential is 2x faster than sync, proving it's not just a wrapper around blocking calls
Using
asyncio.gather()provides 1.7x speedup over sequential async executionRedis is single-threaded, so server-side query execution is sequential; the benefit comes from overlapping client-side I/O wait time
Note
Medium Risk
Adds a new async SQL execution path in
AsyncSearchIndex.query()and bumps thesql-redisdependency, which could affect query behavior/decoding and introduce new runtime ImportError paths when the extra isn’t installed.Overview
AsyncSearchIndex.query()now acceptsSQLQueryand executes it viasql-redis’sAsyncSchemaRegistry/AsyncExecutor, returning rows afterconvert_bytesnormalization (mirroring the sync SQL path).The
sql-redisoptional extra is bumped to>=0.2.0, and integration coverage is expanded with async SQLSELECTandCOUNT(*)cases.Docs are updated with an Async Support example in
12_sql_to_redis_queries.ipynbdemonstrating runningSQLQueryagainstAsyncSearchIndex.Written by Cursor Bugbot for commit 8a15f49. This will update automatically on new commits. Configure here.