-
Notifications
You must be signed in to change notification settings - Fork 101
fix(migration): support RESP3 key enumeration #730
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,6 +37,7 @@ | |
| wait_for_index_ready, | ||
| ) | ||
| from redisvl.migration.validation import MigrationValidator | ||
| from redisvl.redis.utils import convert_bytes | ||
| from redisvl.types import SyncRedisClient | ||
| from redisvl.utils.log import get_logger | ||
|
|
||
|
|
@@ -144,6 +145,7 @@ def _map_keys_prefix( | |
|
|
||
| def _extract_prefixes_from_info(info: Any) -> List[str]: | ||
| """Extract Redis Search index prefixes from dict or list FT.INFO shapes.""" | ||
| info = convert_bytes(info) | ||
|
|
||
| def _prefixes_from_definition(definition: Any) -> Any: | ||
| if isinstance(definition, dict): | ||
|
|
@@ -216,6 +218,22 @@ def _checkpoint_identity_matches( | |
| ) | ||
|
|
||
|
|
||
| def _extract_aggregate_keys(results_data: Any) -> Generator[str, None, None]: | ||
| """Read keys from raw RESP2 rows or a RESP3 aggregate result map.""" | ||
| results_data = convert_bytes(results_data) | ||
| if isinstance(results_data, dict): | ||
| keys = (row["extra_attributes"]["__key"] for row in results_data["results"]) | ||
| else: | ||
| # Skip the leading RESP2 metadata; the remaining rows are field/value pairs. | ||
| keys = ( | ||
| row[1] | ||
| for row in results_data[1:] | ||
| if isinstance(row, (list, tuple)) and len(row) >= 2 | ||
| ) | ||
| for key in keys: | ||
| yield key.decode() if isinstance(key, bytes) else str(key) | ||
|
|
||
|
|
||
| class MigrationExecutor: | ||
| def __init__(self, validator: Optional[MigrationValidator] = None): | ||
| self.validator = validator or MigrationValidator() | ||
|
|
@@ -250,9 +268,10 @@ def _enumerate_indexed_keys( | |
| # condition means FT.AGGREGATE would miss documents, so fall | ||
| # back to SCAN for complete enumeration. | ||
| try: | ||
| info = client.ft(index_name).info() | ||
| info = convert_bytes(client.ft(index_name).info()) | ||
| failures = int(info.get("hash_indexing_failures", 0) or 0) | ||
| percent_indexed = float(info.get("percent_indexed", 1.0) or 1.0) | ||
| progress = info.get("percent_indexed") | ||
| percent_indexed = float(progress) if progress is not None else 1.0 | ||
|
Comment on lines
+273
to
+274
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The same expression survives at three sibling sites, all on the RESP3 path:
Those read
progress = snapshot.stats_snapshot.get("percent_indexed")
source_percent_indexed = float(progress) if progress is not None else 1.0
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed all three sites in affa15a. Both apply guards and the planner now default only when progress is None. The new schema-only apply tests use num_docs=0 with two stored keys and assert that expected_source_count=2 reaches validation. These sync/async cases and the planner's numeric-zero case failed before the fix; RESP2 string-zero and complete/null controls passed. After syncing main's scan_iter change, all 162 related tests pass on both redis-py 8.1.0 and 6.3.0. |
||
| if failures > 0: | ||
| logger.warning( | ||
| f"Index '{index_name}' has {failures} indexing failures. " | ||
|
|
@@ -331,11 +350,7 @@ def _enumerate_with_aggregate( | |
| while True: | ||
| results_data, cursor_id = result | ||
|
|
||
| # Extract keys from results (skip first element which is count) | ||
| for item in results_data[1:]: | ||
| if isinstance(item, (list, tuple)) and len(item) >= 2: | ||
| key = item[1] | ||
| yield key.decode() if isinstance(key, bytes) else str(key) | ||
| yield from _extract_aggregate_keys(results_data) | ||
|
|
||
| # Check if done (cursor_id == 0) | ||
| if cursor_id == 0: | ||
|
|
@@ -1092,9 +1107,8 @@ def apply( | |
| source_failures = int( | ||
| plan.source.stats_snapshot.get("hash_indexing_failures", 0) or 0 | ||
| ) | ||
| source_percent_indexed = float( | ||
| plan.source.stats_snapshot.get("percent_indexed", 1.0) or 1.0 | ||
| ) | ||
| progress = plan.source.stats_snapshot.get("percent_indexed") | ||
| source_percent_indexed = float(progress) if progress is not None else 1.0 | ||
| needs_exact_count = source_failures > 0 or source_percent_indexed < 1.0 | ||
| needs_enumeration = ( | ||
| needs_quantization | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the most consequential change in the diff, and the description does not mention it. Worth adding, because it changes how urgent the fix looks to anyone triaging.
Measured pre-PR on RESP3 with
decode_responses=False,_extract_prefixes_from_inforeturned[]against the byte-keyedFT.INFO.build_scan_match_patterns([])returns["*"](utils.py:86-92), so_enumerate_with_scanenumerated the entire keyspace as the index's documents._rename_field_in_hashat:640does not filter the key list it is handed, so an unrelated key carrying a field of the same name gets rewritten:Post-fix, prefix extraction returns the real prefix in all four combinations. Reaching the bad path needs
FT.AGGREGATEto fail with aResponseErrorfirst, which the_enumerate_with_aggregatedocstring treats as routine cursor expiry rather than an exotic condition.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expanded the description with the cross-prefix field-rename consequence and its trigger: RESP3 byte-keyed FT.INFO plus an aggregate ResponseError could turn the fallback into SCAN *. It now explicitly attributes the Redis 8.2.7 reproduction to your review. The existing sync/async fallback tests continue to assert the source-prefix match after the aggregate error.