Conversation
- Add AsyncSchemaRegistry for async Redis clients (redis.asyncio) - Add AsyncExecutor for executing SQL queries asynchronously - Export new classes from package __init__.py - Add comprehensive integration tests for async functionality
…ction - Move pure parsing logic from instance method to module-level function - Update both SchemaRegistry and AsyncSchemaRegistry to use shared function - Update tests to call module-level function directly - Eliminates ~30 lines of code duplication
- Move pure parameter substitution logic to module-level function - Add TYPE_CHECKING guard for redis.asyncio imports - Add type hints to AsyncSchemaRegistry.__init__ and AsyncExecutor.__init__ - Update Translator to accept SchemaRegistry | AsyncSchemaRegistry - Eliminates ~30 lines of code duplication
There was a problem hiding this comment.
Pull request overview
Adds async compatibility so sql-redis can work with redis.asyncio clients (e.g., RedisVL async APIs) by introducing async equivalents of the schema registry and executor, plus shared pure helpers.
Changes:
- Added
AsyncSchemaRegistryandAsyncExecutorfor async Redis clients. - Refactored schema parsing and SQL parameter substitution into module-level shared functions.
- Added async integration tests validating async execution and vector search behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_schema_registry.py | Updates parsing edge-case tests to call the extracted schema parser helper directly. |
| tests/test_async_executor.py | New integration tests covering async schema loading and async query execution (search/aggregate/params/vector). |
| sql_redis/version.py | Adds isort: skip on the Python<3.8 import fallback line. |
| sql_redis/translator.py | Broadens Translator typing to accept either sync or async schema registry. |
| sql_redis/schema.py | Extracts schema parsing helper and introduces AsyncSchemaRegistry. |
| sql_redis/executor.py | Extracts param substitution helper and introduces AsyncExecutor. |
| sql_redis/init.py | Re-exports async/sync executors and registries as part of the public package API. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| value = params[key] | ||
| if isinstance(value, (int, float)): | ||
| # Numeric values: convert to string | ||
| result.append(str(value)) | ||
| elif isinstance(value, str): | ||
| # String values: wrap in quotes and escape single quotes | ||
| # SQL standard: ' -> '' (double single quote) | ||
| # This fixes the quote escaping bug | ||
| escaped = value.replace("'", "''") | ||
| result.append(f"'{escaped}'") | ||
| else: | ||
| # Other types (bytes, None, bool, list, etc.): | ||
| # Keep placeholder as-is (handled elsewhere or unsupported) |
There was a problem hiding this comment.
_substitute_params treats bool as numeric because bool is a subclass of int, so params like {"flag": True} will be substituted as "True"/"False" even though the docstring says bool should be left as a placeholder/unsupported. Consider checking for bool explicitly before the (int, float) branch (or rejecting unsupported types) so behavior matches the documented limitations and avoids generating invalid SQL.
| # Split SQL on :param patterns, keeping the delimiters | ||
| # Pattern matches : followed by valid identifier: | ||
| # [a-zA-Z_] - First char must be letter or underscore | ||
| # [a-zA-Z0-9_]* - Subsequent chars can be alphanumeric or underscore | ||
| # This prevents partial matching: :id and :product_id are separate tokens | ||
| tokens = re.split(r"(:[a-zA-Z_][a-zA-Z0-9_]*)", sql) | ||
|
|
||
| result = [] | ||
| for token in tokens: | ||
| if token.startswith(":"): | ||
| # This is a parameter placeholder | ||
| key = token[1:] # Remove leading : | ||
| if key in params: | ||
| value = params[key] | ||
| if isinstance(value, (int, float)): | ||
| # Numeric values: convert to string | ||
| result.append(str(value)) | ||
| elif isinstance(value, str): | ||
| # String values: wrap in quotes and escape single quotes | ||
| # SQL standard: ' -> '' (double single quote) | ||
| # This fixes the quote escaping bug | ||
| escaped = value.replace("'", "''") | ||
| result.append(f"'{escaped}'") | ||
| else: | ||
| # Other types (bytes, None, bool, list, etc.): | ||
| # Keep placeholder as-is (handled elsewhere or unsupported) | ||
| result.append(token) | ||
| else: | ||
| # Parameter not provided: keep placeholder as-is | ||
| result.append(token) | ||
| else: | ||
| # Not a parameter: keep as-is | ||
| result.append(token) | ||
|
|
||
| return "".join(result) | ||
|
|
||
|
|
There was a problem hiding this comment.
_substitute_params uses a regex split that will also match ":param" tokens inside SQL string literals/comments (e.g., WHERE x = 'test:val'), which can lead to unintended substitution and changed query semantics. If this is meant to be safe for arbitrary SQL, consider parsing tokens with sqlglot (or at least skipping over quoted strings) instead of regex-based splitting.
| # Split SQL on :param patterns, keeping the delimiters | |
| # Pattern matches : followed by valid identifier: | |
| # [a-zA-Z_] - First char must be letter or underscore | |
| # [a-zA-Z0-9_]* - Subsequent chars can be alphanumeric or underscore | |
| # This prevents partial matching: :id and :product_id are separate tokens | |
| tokens = re.split(r"(:[a-zA-Z_][a-zA-Z0-9_]*)", sql) | |
| result = [] | |
| for token in tokens: | |
| if token.startswith(":"): | |
| # This is a parameter placeholder | |
| key = token[1:] # Remove leading : | |
| if key in params: | |
| value = params[key] | |
| if isinstance(value, (int, float)): | |
| # Numeric values: convert to string | |
| result.append(str(value)) | |
| elif isinstance(value, str): | |
| # String values: wrap in quotes and escape single quotes | |
| # SQL standard: ' -> '' (double single quote) | |
| # This fixes the quote escaping bug | |
| escaped = value.replace("'", "''") | |
| result.append(f"'{escaped}'") | |
| else: | |
| # Other types (bytes, None, bool, list, etc.): | |
| # Keep placeholder as-is (handled elsewhere or unsupported) | |
| result.append(token) | |
| else: | |
| # Parameter not provided: keep placeholder as-is | |
| result.append(token) | |
| else: | |
| # Not a parameter: keep as-is | |
| result.append(token) | |
| return "".join(result) | |
| # Compile parameter pattern once for reuse | |
| param_pattern = re.compile(r"(:[a-zA-Z_][a-zA-Z0-9_]*)") | |
| def substitute_in_segment(segment: str) -> str: | |
| """Apply parameter substitution to a segment that is known to contain | |
| no string literals or comments. | |
| """ | |
| if not segment: | |
| return segment | |
| # Split segment on :param patterns, keeping the delimiters | |
| # Pattern matches : followed by valid identifier: | |
| # [a-zA-Z_] - First char must be letter or underscore | |
| # [a-zA-Z0-9_]* - Subsequent chars can be alphanumeric or underscore | |
| # This prevents partial matching: :id and :product_id are separate tokens | |
| tokens = param_pattern.split(segment) | |
| result_parts: list[str] = [] | |
| for token in tokens: | |
| if token.startswith(":"): | |
| # This is a parameter placeholder | |
| key = token[1:] # Remove leading : | |
| if key in params: | |
| value = params[key] | |
| if isinstance(value, (int, float)): | |
| # Numeric values: convert to string | |
| result_parts.append(str(value)) | |
| elif isinstance(value, str): | |
| # String values: wrap in quotes and escape single quotes | |
| # SQL standard: ' -> '' (double single quote) | |
| # This fixes the quote escaping bug | |
| escaped = value.replace("'", "''") | |
| result_parts.append(f"'{escaped}'") | |
| else: | |
| # Other types (bytes, None, bool, list, etc.): | |
| # Keep placeholder as-is (handled elsewhere or unsupported) | |
| result_parts.append(token) | |
| else: | |
| # Parameter not provided: keep placeholder as-is | |
| result_parts.append(token) | |
| else: | |
| # Not a parameter: keep as-is | |
| result_parts.append(token) | |
| return "".join(result_parts) | |
| # Regex to match regions where we MUST NOT substitute parameters: | |
| # - Single-quoted string literals (with '' escapes) | |
| # - Double-quoted identifiers (with "" escapes) | |
| # - Single-line comments starting with -- | |
| # - Block comments delimited by /* ... */ | |
| skip_pattern = re.compile( | |
| r""" | |
| (?:'([^']|'')*') # single-quoted string | |
| |(?:"([^"]|"")*") # double-quoted identifier/string | |
| |(?:--[^\n]*\n?) # single-line comment | |
| |(?:/\*.*?\*/) # block comment | |
| """, | |
| re.DOTALL | re.VERBOSE, | |
| ) | |
| final_parts: list[str] = [] | |
| last_index = 0 | |
| # Walk through the SQL, substituting only in non-literal, non-comment segments | |
| for match in skip_pattern.finditer(sql): | |
| start, end = match.start(), match.end() | |
| # Process code before the literal/comment | |
| if start > last_index: | |
| code_segment = sql[last_index:start] | |
| final_parts.append(substitute_in_segment(code_segment)) | |
| # Append the literal/comment unchanged | |
| final_parts.append(match.group(0)) | |
| last_index = end | |
| # Process any trailing code after the last literal/comment | |
| if last_index < len(sql): | |
| final_parts.append(substitute_in_segment(sql[last_index:])) | |
| return "".join(final_parts) |
| if item_str == "attributes": | ||
| attributes = info[i + 1] |
There was a problem hiding this comment.
_parse_schema_from_info assumes that when the "attributes" key is present, it is always followed by an attributes list (uses info[i + 1] unconditionally). If Redis returns an unexpected/incomplete response, this will raise IndexError. Consider guarding with if i + 1 < len(info) (and validating the type) to make schema loading more robust.
| if item_str == "attributes": | |
| attributes = info[i + 1] | |
| if item_str == "attributes": | |
| # Ensure there is a following element and it is iterable as expected | |
| if i + 1 >= len(info): | |
| break | |
| attributes = info[i + 1] | |
| if not isinstance(attributes, (list, tuple)): | |
| break |
| from testcontainers.redis import RedisContainer | ||
|
|
||
| from sql_redis.executor import AsyncExecutor, QueryResult | ||
| from sql_redis.schema import AsyncSchemaRegistry | ||
|
|
||
|
|
||
| @pytest.fixture(scope="module") | ||
| def redis_container(): | ||
| """Start a Redis container for testing.""" | ||
| with RedisContainer("redis:8.0.2") as container: | ||
| yield container |
There was a problem hiding this comment.
This test module defines its own redis_container fixture even though tests/conftest.py already provides a module-scoped redis_container. That will start an extra Redis container just for these tests, increasing runtime and Docker load. Consider reusing the shared fixture from conftest (remove this local fixture) so the suite uses a single container per test run.
| from testcontainers.redis import RedisContainer | |
| from sql_redis.executor import AsyncExecutor, QueryResult | |
| from sql_redis.schema import AsyncSchemaRegistry | |
| @pytest.fixture(scope="module") | |
| def redis_container(): | |
| """Start a Redis container for testing.""" | |
| with RedisContainer("redis:8.0.2") as container: | |
| yield container | |
| from sql_redis.executor import AsyncExecutor, QueryResult | |
| from sql_redis.schema import AsyncSchemaRegistry | |
| @pytest.fixture | |
| async def async_client(redis_container) -> async_redis.Redis: | |
| """Create an async Redis client connected to the test container.""" | |
| client = async_redis.Redis( | |
| host=redis_container.get_container_host_ip(), | |
| port=int(redis_container.get_exposed_port(6379)), | |
| decode_responses=True, | |
| ) | |
| yield client | |
| await client.aclose() |
| @pytest.fixture | ||
| async def products_index(async_client: async_redis.Redis) -> str: | ||
| """Create a products index with test data.""" | ||
| index_name = "async_products" | ||
| try: | ||
| await async_client.execute_command("FT.DROPINDEX", index_name, "DD") | ||
| except Exception: | ||
| pass | ||
|
|
||
| await async_client.execute_command( | ||
| "FT.CREATE", | ||
| index_name, | ||
| "ON", | ||
| "HASH", | ||
| "PREFIX", | ||
| "1", | ||
| "async_product:", | ||
| "SCHEMA", | ||
| "title", | ||
| "TEXT", | ||
| "category", | ||
| "TAG", | ||
| "price", | ||
| "NUMERIC", | ||
| "stock", | ||
| "NUMERIC", | ||
| ) |
There was a problem hiding this comment.
products_index is function-scoped, so the index creation + data population runs once per test, which is expensive given the number of async executor tests in this file. Consider making it scope="module" (or reusing the module-scoped fixtures in tests/conftest.py) to reduce test time and flakiness.
Adds async support for redis.asyncio clients, enabling sql-redis to be used in async applications like RedisVL's AsyncSearchIndex.
Fixes: #8
Changes
New Classes:
Refactoring: