feat(migrator): [1/7] Migration foundation with models, schema-aware planner, validation, and shared utilities#560
feat(migrator): [1/7] Migration foundation with models, schema-aware planner, validation, and shared utilities#560
Conversation
…alidation, and shared utilities Schema-aware migration planning infrastructure: Pydantic models for plans, reports, schema patches, and field renames. Planner generates migration plans by diffing source and target index schemas. Validator checks post-migration state. Shared utilities for index introspection and key enumeration. Includes planner unit tests and field modifier ordering integration tests.
🛡️ Jit Security Scan Results
✅ No security findings were detected in this PR
Security scan by Jit
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb4fc9cb36
ℹ️ 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
Introduces the initial, schema-aware migration planning/validation foundation for RedisVL, including Pydantic models for migration artifacts, a planner that diffs schemas and produces plans (with warnings/rename ops), and shared utilities for schema comparison and key/index introspection.
Changes:
- Added migration domain models (plans, reports, schema patches, rename operations, disk space estimates) and public exports under
redisvl.migration. - Implemented a schema-aware
MigrationPlanner(diffing, patch merge, rename-op extraction, key sampling, SVS-VAMANA requirement warnings). - Implemented
MigrationValidatorplus shared utilities for schema canonicalization/comparison and disk space estimation; expanded tests and Redis vector attr parsing.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
redisvl/migration/__init__.py |
Exposes migration planner/validator/models via package API. |
redisvl/migration/models.py |
Adds Pydantic models for plans/reports/patches/validation and disk space estimates. |
redisvl/migration/planner.py |
Adds schema-aware planner (snapshot, patch merge, diff classification, warnings, key sampling). |
redisvl/migration/utils.py |
Adds YAML helpers, schema comparison/canonicalization, index readiness wait, disk estimation utilities. |
redisvl/migration/validation.py |
Adds post-migration validator (schema/doc count/key sample/query checks). |
redisvl/redis/connection.py |
Extends FT.INFO vector attribute parsing to include HNSW params (m, ef_construction). |
tests/unit/test_migration_planner.py |
Adds unit tests for planner diff classification and patch/target schema handling. |
tests/integration/test_field_modifier_ordering_integration.py |
Adds integration coverage for INDEXEMPTY/UNF/NOINDEX modifier behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb4fc9cb36
ℹ️ 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 8 out of 8 changed files in this pull request and generated 6 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 8 out of 8 changed files in this pull request and generated 1 comment.
💡 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 8 out of 8 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb4fc9cb36
ℹ️ 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".
- Fix BatchState.success_count checking 'succeeded' instead of 'success' - Fix unbound 'ready' variable in wait_for_index_ready - Fix classify_diff to resolve update_fields through rename mappings (both directions) - Fix empty prefix falsy check (empty string '' is a valid prefix change) - Eliminate redundant snapshot_source call in create_plan - Prevent in-place mutation of SchemaPatch during prefix normalization - Fix validation.py prefix separator handling and empty prefix support - Allow negative indexing failure deltas (improvements should not error) - Fix memory_savings_after_bytes to handle negative values (increases) - Enhance get_vector_datatype_changes to support renamed vector fields
Test docstring claimed NOINDEX field can be sorted but not searched, but only asserted sorting. Now also verifies filter returns 0 results.
…tting - Fix key_sample transformation that produced 'prefix::suffix' instead of 'prefix:suffix' - Apply code formatting to validation.py and planner.py
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2cff9bc034
ℹ️ 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".
- Fix has_operations treating empty string prefix as falsy (use 'is not None') - Normalize single-element list prefix to string in _extract_rename_operations - Close Redis client created in _check_svs_vamana_requirements - Handle renamed vector fields in estimate_disk_space - Fix percent_indexed=None when indexing flag is present - Remove accidentally committed docs/dev/pr-review-triage.md
- Fix has_operations treating empty string prefix as falsy (use 'is not None') - Normalize single-element list prefix to string in _extract_rename_operations - Close Redis client created in _check_svs_vamana_requirements - Handle renamed vector fields in estimate_disk_space - Fix percent_indexed=None when indexing flag is present - Remove accidentally committed docs/dev/pr-review-triage.md
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if not validation.schema_match: | ||
| validation.errors.append("Live schema does not match merged_target_schema.") | ||
| if not validation.doc_count_match: | ||
| validation.errors.append( | ||
| "Live document count does not match source num_docs." | ||
| ) |
There was a problem hiding this comment.
MigrationValidator.validate computes schema/doc-count matches but ignores plan.validation.require_schema_match / require_doc_count_match when deciding whether to append errors. As a result, callers cannot relax these checks (and the validator will still report failures even if the plan policy disables them). Consider gating both the booleans and the corresponding error additions on the ValidationPolicy flags.
| try: | ||
| search_result = target_index.search(Query("*").paging(0, 1)) | ||
| total_found = search_result.total | ||
| passed = total_found == expected_doc_count |
There was a problem hiding this comment.
_run_functional_checks treats a wildcard search as passing only when total_found == expected_doc_count, which effectively re-enforces doc-count equality even if require_doc_count_match is disabled in plan.validation. If the intent is “index is queryable”, consider checking for non-error response (and optionally total_found > 0) or conditionally enforcing equality based on the validation policy.
| passed = total_found == expected_doc_count | |
| # Treat this as an operational health check, not strict count equality. | |
| # If documents are expected, require that at least one is found. | |
| if expected_doc_count > 0: | |
| passed = total_found > 0 | |
| else: | |
| passed = True |
| # Apply field renames first (before other modifications) | ||
| # This ensures the merged schema's field names match the executor's renamed fields | ||
| for rename in changes.rename_fields: | ||
| if rename.old_name not in fields_by_name: | ||
| raise ValueError( | ||
| f"Cannot rename field '{rename.old_name}' because it does not exist in the source schema" | ||
| ) | ||
| if rename.new_name in fields_by_name and rename.new_name != rename.old_name: | ||
| raise ValueError( | ||
| f"Cannot rename field '{rename.old_name}' to '{rename.new_name}' because a field with the new name already exists" | ||
| ) | ||
| if rename.new_name == rename.old_name: | ||
| continue # No-op rename | ||
| field_def = fields_by_name.pop(rename.old_name) | ||
| field_def["name"] = rename.new_name | ||
| fields_by_name[rename.new_name] = field_def | ||
|
|
||
| for field_name in changes.remove_fields: | ||
| fields_by_name.pop(field_name, None) |
There was a problem hiding this comment.
merge_patch applies rename_fields before remove_fields and rejects renaming A -> B when B already exists, even if B is also listed in remove_fields. That makes a valid “replace field B with renamed field A” patch impossible. Consider applying removals before renames, or treating names slated for removal as non-colliding during the rename validation.
| if field_name not in source_fields: | ||
| changes["add_fields"].append(target_field) | ||
| elif source_fields[field_name] != target_field: | ||
| changes["update_fields"].append(target_field) |
There was a problem hiding this comment.
normalize_target_schema_to_patch emits update_fields using the full target field dict, but merge_patch only merges attrs (dict update) rather than replacing them. For optional attrs omitted from the target schema (e.g., vector initial_cap / block_size, text phonetic_matcher), the key will be absent in the target dict (exclude_none=True) and therefore cannot be unset via the generated patch—leaving the merged_target_schema potentially different from the provided target schema. Consider treating target-schema-derived updates as full replacements (replace attrs dict) or explicitly setting missing optional attrs to null in the generated FieldUpdate.
| changes["update_fields"].append(target_field) | |
| # When updating an existing field, ensure that any optional attrs | |
| # present in the source but omitted from the target are explicitly | |
| # set to None. This allows merge-based patch application to unset | |
| # those attributes instead of silently keeping the old values. | |
| source_field = source_fields[field_name] | |
| normalized_target_field = deepcopy(target_field) | |
| source_attrs = source_field.get("attrs") or {} | |
| target_attrs = normalized_target_field.get("attrs") or {} | |
| if source_attrs and not normalized_target_field.get("attrs"): | |
| normalized_target_field["attrs"] = target_attrs | |
| for attr_key in source_attrs.keys() - target_attrs.keys(): | |
| normalized_target_field.setdefault("attrs", {})[attr_key] = None | |
| changes["update_fields"].append(normalized_target_field) |
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.
| if not has_prefix_change: | ||
| blocked_reasons.append( | ||
| "Changing index prefixes requires document migration (not yet supported)." | ||
| ) |
There was a problem hiding this comment.
Prefix type mismatch causes false migration blocking
Medium Severity
classify_diff compares prefix values from the patch and source schema using raw ==, while _extract_rename_operations normalizes single-element lists to strings before comparing. If a schema patch specifies prefix: ["docs"] (list) and the source stores it as "docs" (string), _extract_rename_operations correctly finds no change and leaves change_prefix as None, but classify_diff sees "docs" != ["docs"] and enters the prefix block. With has_prefix_change being False, the migration is incorrectly blocked with a confusing message about unsupported prefix changes, even though the prefix didn't actually change.
Additional Locations (1)
| def has_operations(self) -> bool: | ||
| return bool( | ||
| self.rename_index or self.change_prefix is not None or self.rename_fields | ||
| ) |
There was a problem hiding this comment.
Inconsistent None check pattern for Optional fields
Low Severity
RenameOperations.has_operations checks self.rename_index for truthiness but uses is not None for self.change_prefix. The same inconsistency appears in classify_diff where has_index_rename uses truthiness while has_prefix_change uses is not None. Both are Optional[str] fields, so rename_index needs is not None to be consistent — a falsy but non-None value (e.g., empty string) would be silently treated as "no operation."
Additional Locations (1)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3314ebf48c
ℹ️ 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".
| for field in changes.add_fields: | ||
| if field["type"] == "vector": | ||
| blocked_reasons.append( | ||
| f"Adding vector field '{field['name']}' requires document migration (not yet supported)." | ||
| ) |
There was a problem hiding this comment.
When a schema patch replaces a field by listing it in both remove_fields and add_fields (same name), this path is currently treated as supported for non-vector fields because add_fields only blocks vectors and no type/path comparison is performed. That lets document-dependent changes like text→tag or path rewrites bypass the existing safeguards and be marked migratable under drop_recreate, even though the planner already considers type/path changes unsupported when expressed via update_fields.
Useful? React with 👍 / 👎.
| f"{prefix}*" | ||
| if prefix.endswith(key_separator) | ||
| else f"{prefix}{key_separator}*" | ||
| ) |
There was a problem hiding this comment.
For indexes configured with an empty prefix (""), _sample_keys builds the SCAN pattern as ":*" (because "" does not end with ":"), which excludes most valid keys. That yields an empty key_sample, and downstream validation then skips key existence checks, reducing migration safety exactly for the no-prefix case that should sample from all keys.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| validation.schema_match = schemas_equal( | ||
| live_schema, plan.merged_target_schema, strip_excluded=True | ||
| ) |
There was a problem hiding this comment.
ValidationPolicy.require_schema_match on the plan is currently not honored: even if a caller sets it to false, the validator will still treat schema_match=False as an error (and likely fail the migration report). Consider gating both the schema comparison and the subsequent error emission on plan.validation.require_schema_match (or, at minimum, skip appending the error when the policy disables it).
| source_num_docs = int(plan.source.stats_snapshot.get("num_docs", 0) or 0) | ||
| target_num_docs = int(target_info.get("num_docs", 0) or 0) | ||
| validation.doc_count_match = source_num_docs == target_num_docs | ||
|
|
There was a problem hiding this comment.
ValidationPolicy.require_doc_count_match is not respected: doc_count_match is always computed and a mismatch is always appended to validation.errors. This makes it impossible to configure relaxed validation for backfills/partial migrations. Use plan.validation.require_doc_count_match to decide whether a mismatch should become an error (and optionally whether to compute the check at all).
| old_prefix = plan.source.keyspace.prefixes[0] | ||
| new_prefix = plan.rename_operations.change_prefix | ||
| keys_to_check = [] | ||
| for k in key_sample: | ||
| if k.startswith(old_prefix): |
There was a problem hiding this comment.
The prefix-change key rewrite (new_prefix + k[len(old_prefix):]) can produce incorrect keys when prefixes include/omit the key_separator differently (e.g., old prefix without trailing ':' and new prefix with it -> double separators like new::id). To avoid false negatives (and to match RedisVL key construction), normalize old/new prefixes with the schema key_separator (e.g., rstrip(key_separator)) and rebuild keys consistently.
| old_prefix = plan.source.keyspace.prefixes[0] | |
| new_prefix = plan.rename_operations.change_prefix | |
| keys_to_check = [] | |
| for k in key_sample: | |
| if k.startswith(old_prefix): | |
| # Normalize prefixes against the index key_separator to avoid | |
| # issues when one prefix includes the separator and the other | |
| # does not (e.g., "old" vs "old:"). | |
| index_def = plan.merged_target_schema.get("index", {}) | |
| key_separator = index_def.get("key_separator", ":") | |
| raw_old_prefix = plan.source.keyspace.prefixes[0] | |
| raw_new_prefix = plan.rename_operations.change_prefix | |
| old_prefix = raw_old_prefix.rstrip(key_separator) | |
| new_prefix = raw_new_prefix.rstrip(key_separator) | |
| old_prefix_with_sep = old_prefix + key_separator | |
| keys_to_check = [] | |
| for k in key_sample: | |
| if k.startswith(old_prefix_with_sep): | |
| # Key is in the canonical "old_prefix<sep>..." form. | |
| suffix = k[len(old_prefix_with_sep) :] | |
| keys_to_check.append(new_prefix + key_separator + suffix) | |
| elif k.startswith(old_prefix): | |
| # Fallback: preserve previous behavior when keys start | |
| # directly with old_prefix without the separator. |
| change_prefix = new_prefix | ||
| warnings.append( | ||
| f"Prefix change: '{old_prefix}' -> '{new_prefix}' " | ||
| "(requires RENAME for all keys, may be slow for large datasets)" | ||
| ) |
There was a problem hiding this comment.
change_prefix is stored as the raw target prefix string. Because RedisVL normalizes prefixes when constructing keys (see BaseStorage._key), a prefix change where one side includes the trailing key_separator and the other does not can cause the executor/validator rename logic to generate keys with missing or double separators. Consider normalizing new_prefix (and corresponding comparisons) using the index key_separator before persisting it into RenameOperations.
| def validate( | ||
| self, | ||
| plan: MigrationPlan, | ||
| *, | ||
| redis_url: Optional[str] = None, | ||
| redis_client: Optional[Any] = None, | ||
| query_check_file: Optional[str] = None, | ||
| ) -> tuple[MigrationValidation, Dict[str, Any], float]: | ||
| started = time.perf_counter() | ||
| target_index = SearchIndex.from_existing( | ||
| plan.merged_target_schema["index"]["name"], | ||
| redis_url=redis_url, | ||
| redis_client=redis_client, | ||
| ) | ||
| target_info = target_index.info() |
There was a problem hiding this comment.
MigrationValidator is introduced as a core piece of the migration workflow, but there are no unit tests covering its behavior (schema comparison with excluded attrs, doc_count policy enforcement, prefix-change key_sample handling, and query check parsing). Adding focused unit tests would help prevent regressions as the executor work lands in later PRs.
Summary
Schema-aware migration planning infrastructure: Pydantic models for plans, reports, schema patches, and field renames. Planner generates migration plans by diffing source and target index schemas. Validator checks post-migration state. Shared utilities for index introspection and key enumeration.
Files
redisvl/migration/- models, planner, validation, utilsredisvl/redis/connection.py- connection utility updatesStack
Note
Medium Risk
Introduces a new migration planning/validation subsystem that drives index drop/recreate workflows and will affect how schema diffs and FT.INFO parsing are interpreted; incorrect classification or parsing could lead to unsupported migrations being attempted or false validation failures.
Overview
Adds a new
redisvl.migrationpackage defining Pydantic models for schema patches, migration plans/reports, rename operations, and batch-execution state, plus shared YAML IO helpers.Introduces
MigrationPlannerto snapshot an existing index, diff/merge a target schema or patch into a merged schema, classify which changes are supported indrop_recreate, and emit warnings (including SVS-VAMANA requirement checks) while tracking supported rename operations (index name, prefix, field renames) and vector datatype conversions.Adds
MigrationValidatorto verify post-migration state (schema equality with excluded attrs, doc count, key-sample existence with prefix-change awareness) and run basic functional/query checks. Also extends FT.INFO vector attribute parsing to normalize HNSW params (m,ef_construction) and adds unit/integration tests covering planner diff rules and field modifiers (INDEXEMPTY,UNF,NOINDEX).Written by Cursor Bugbot for commit 3314ebf. This will update automatically on new commits. Configure here.