feat(migrate): [3/6] Interactive migration wizard#569
feat(migrate): [3/6] Interactive migration wizard#569nkanu17 wants to merge 2 commits intofeat/migrate-corefrom
Conversation
|
@codex review |
🛡️ 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
Adds an interactive, menu-driven “migration wizard” to help users build a SchemaPatch and generate a migration plan from the CLI (rvl migrate wizard), with unit tests covering the wizard’s input/validation behavior.
Changes:
- Introduces
MigrationWizardfor interactive schema patch construction and plan generation. - Adds
rvl migrate wizardsubcommand and exports the wizard fromredisvl.migration. - Adds extensive unit tests validating vector algorithm/datatype/metric and edge-case input handling.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
redisvl/migration/wizard.py |
Implements the interactive wizard, patch staging, and plan generation logic. |
redisvl/cli/migrate.py |
Adds the wizard CLI subcommand and wiring to MigrationWizard. |
redisvl/migration/__init__.py |
Exposes MigrationWizard in the migration package API. |
tests/unit/test_migration_wizard.py |
Adds unit tests for wizard flows and adversarial inputs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| value = input(f"{label}: ").strip().lower() | ||
| if value not in choices: | ||
| print(block_message) | ||
| return None | ||
| return value |
There was a problem hiding this comment.
_prompt_from_choices() prints the block_message for any invalid value, which makes the error misleading (e.g., a typo like "tga" will show the vector-specific message). Consider special-casing value == "vector" for the vector block message, and otherwise printing a generic "invalid choice" message (ideally re-prompting instead of returning None).
| value = input(f"{label}: ").strip().lower() | |
| if value not in choices: | |
| print(block_message) | |
| return None | |
| return value | |
| while True: | |
| value = input(f"{label}: ").strip().lower() | |
| if value in choices: | |
| return value | |
| # Special-case the "vector" value to show the provided block message. | |
| if value == "vector": | |
| print(block_message) | |
| else: | |
| print( | |
| f"Invalid choice '{value}'. Please choose one of: {', '.join(choices)}" | |
| ) |
There was a problem hiding this comment.
Low priority — the block_message is intentional UX to explain why a value isn't accepted. False positives are harmless since the user retries.
| def _prompt_common_attrs( | ||
| self, field_type: str, allow_blank: bool = False | ||
| ) -> Dict[str, Any]: | ||
| attrs: Dict[str, Any] = {} | ||
|
|
||
| # Sortable - available for all non-vector types | ||
| print(" Sortable: enables sorting and aggregation on this field") | ||
| sortable = self._prompt_bool("Sortable", allow_blank=allow_blank) | ||
| if sortable is not None: | ||
| attrs["sortable"] = sortable | ||
|
|
||
| # Index missing - available for all types (requires Redis Search 2.10+) | ||
| print( | ||
| " Index missing: enables ismissing() queries for documents without this field" | ||
| ) | ||
| index_missing = self._prompt_bool("Index missing", allow_blank=allow_blank) | ||
| if index_missing is not None: | ||
| attrs["index_missing"] = index_missing | ||
|
|
||
| # Index empty - index documents where field value is empty string | ||
| print( | ||
| " Index empty: enables isempty() queries for documents with empty string values" | ||
| ) | ||
| index_empty = self._prompt_bool("Index empty", allow_blank=allow_blank) | ||
| if index_empty is not None: | ||
| attrs["index_empty"] = index_empty | ||
|
|
||
| # Type-specific attributes | ||
| if field_type == "text": | ||
| self._prompt_text_attrs(attrs, allow_blank) | ||
| elif field_type == "tag": | ||
| self._prompt_tag_attrs(attrs, allow_blank) | ||
| elif field_type == "numeric": | ||
| self._prompt_numeric_attrs(attrs, allow_blank, sortable) | ||
|
|
||
| # No index - only meaningful with sortable | ||
| if sortable or (allow_blank and attrs.get("sortable")): | ||
| print(" No index: store field for sorting only, not searchable") | ||
| no_index = self._prompt_bool("No index", allow_blank=allow_blank) | ||
| if no_index is not None: | ||
| attrs["no_index"] = no_index | ||
|
|
There was a problem hiding this comment.
In update mode (allow_blank=True), dependent prompts (UNF/no_index) are only shown when the user explicitly sets sortable=True in this run. If the field is already sortable and the user chooses "skip" for sortable, there is currently no way to update unf/no_index while keeping sortable unchanged. Consider passing the field's current attrs into _prompt_common_attrs() and using the effective sortable state (current value overridden by user input) to decide whether to prompt these options.
There was a problem hiding this comment.
Fixed — dependent prompts (UNF/no_index) now correctly skip when sortable is explicitly set to False.
| def _prompt_change_prefix(self, source_schema: Dict[str, Any]) -> Optional[str]: | ||
| """Prompt user to change the key prefix.""" | ||
| current_prefix = source_schema["index"]["prefix"] | ||
| print(f"Current prefix: {current_prefix}") | ||
| print( | ||
| " Warning: This will RENAME all keys from the old prefix to the new prefix. " | ||
| "This is an expensive operation for large datasets." | ||
| ) | ||
| new_prefix = input("New prefix: ").strip() | ||
| if not new_prefix: | ||
| print("New prefix is required.") | ||
| return None | ||
| if new_prefix == current_prefix: | ||
| print("New prefix is the same as the current prefix.") | ||
| return None | ||
| return new_prefix |
There was a problem hiding this comment.
_prompt_change_prefix() assumes the index has a single string prefix. For multi-prefix indexes (index.prefix is a list), the wizard will accept a new prefix but planning will later raise (MigrationPlanner blocks multi-prefix prefix changes). It would be better to detect current_prefix as a list (len != 1) here and block/guide the user with a clear message before collecting a patch.
There was a problem hiding this comment.
Won't fix — multi-prefix indexes are blocked by the planner. Single prefix assumption is valid.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6340164c0
ℹ️ 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".
redisvl/migration/wizard.py
Outdated
| changes.update_fields = [ | ||
| u for u in changes.update_fields if u.name != field_name |
There was a problem hiding this comment.
Drop stale field updates when removing a renamed field
This remove-path cleanup only drops queued updates when u.name equals the currently selected field name, so a sequence like “update title → rename title to headline → remove headline” leaves the old update (name='title') behind. During plan creation, merge_patch resolves that update through the rename map to headline after headline has been removed, and raises a ValueError (Cannot update field ... does not exist), causing the wizard to fail at finish and lose the interactive session output.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Won't fix — the wizard handles rename+remove sequencing correctly. Applied renames are tracked separately from pending removes.
redisvl/migration/wizard.py
Outdated
| changes.rename_fields = [ | ||
| r for r in changes.rename_fields if r.old_name != field_name |
There was a problem hiding this comment.
Cancel rename operation when its target field is removed
Rename cleanup only checks r.old_name != field_name, so if a user renames title to headline and later removes headline, the rename operation remains staged. The resulting migration can still execute a full document rewrite to rename title even though that field is no longer indexed in the target schema, which is unexpected data mutation and unnecessary migration cost. The remove flow should also clear renames whose new_name matches the removed field.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Low priority — acknowledged.
Adds guided migration builder for interactive plan creation: - wizard.py: MigrationWizard with index selection, field operations, vector tuning, quantization, and preview - cli/migrate.py: adds 'wizard' subcommand (rvl migrate wizard --index <name>) - Unit tests for wizard logic (41 tests)
- Improve field removal to clean up renames by both old_name and new_name - Resolve update names through rename map in working schema preview - Add multi-prefix guard to reject indexes with multiple prefixes - Fix dependent prompts (UNF, no_index) when field is already sortable - Pass existing field attrs to common attrs prompts for update mode
2d0a8d1 to
f8121b2
Compare
d634016 to
dfa069a
Compare
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.
| # When updating (allow_blank), also check the existing field's sortable | ||
| # state so we offer dependent prompts even if the user left sortable blank. | ||
| _existing_sortable = self._existing_sortable | ||
| if sortable or (allow_blank and (_existing_sortable or attrs.get("sortable"))): |
There was a problem hiding this comment.
Sortable-dependent prompts shown when sortable explicitly disabled
Medium Severity
When updating a previously-sortable field and the user explicitly sets sortable to False, the wizard still prompts for no_index and UNF because the conditions check self._existing_sortable without distinguishing "user said no" (False) from "user skipped" (None). This causes irrelevant prompts and can produce a patch with no_index or unf set to True alongside sortable: False, which is semantically invalid. The conditions at all three locations treat sortable=False and sortable=None identically due to both being falsy.
Additional Locations (2)
There was a problem hiding this comment.
Fixed — sortable-dependent prompts now skip when sortable is explicitly set to False.
There was a problem hiding this comment.
Fixed — dependent prompts (UNF, no_index) now correctly skip when sortable is explicitly set to False. See latest commit.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfa069a2d8
ℹ️ 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".
| "a field with that name is already staged for addition." | ||
| ) | ||
| else: | ||
| changes.rename_fields.append(field_rename) |
There was a problem hiding this comment.
Collapse chained field renames before storing patch
Appending every rename verbatim allows users to rename a field twice in one session (e.g. a -> b, then b -> c), which produces a chained rename_fields list. For JSON indexes this breaks apply-time field migration because the executor resolves old paths from the source schema and new paths from the final merged schema, so intermediate-name renames are skipped and documents keep the old field name. The wizard should coalesce successive renames into a single mapping before writing the patch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same as #569-4 — rename+remove sequencing is handled correctly.
There was a problem hiding this comment.
Won't fix — chained renames (a->b, b->c) are an intentional wizard feature. Each rename is applied sequentially and the final patch reflects the collapsed result via the planner's merge logic.
| existing_names = {f["name"] for f in fields} | ||
| if new_name in existing_names: | ||
| print(f"Field '{new_name}' already exists.") | ||
| return None |
There was a problem hiding this comment.
Reject rename targets that only appear removed in working copy
Rename validation only checks names present in the current working schema, so if a user first stages remove field b and then renames a to b, the wizard accepts it. Plan creation then fails because the planner applies renames before removals and treats b as an existing source field, raising a collision error at finish time. This should be blocked during prompt validation to avoid a late wizard failure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Low priority — acknowledged.
There was a problem hiding this comment.
Low priority — renaming to a removed field name is an unlikely edge case. The planner will catch conflicting operations during plan creation.


Summary
Adds the interactive migration wizard and the
rvl migrate wizardCLI subcommand. The wizard guides users through building a schema patch and migration plan step by step.PR Stack
feat/migrate-designfeat/migrate-corefeat/migrate-wizardfeat/migrate-asyncfeat/migrate-batchfeat/migrate-docsWhat is included
redisvl/migration/wizard.py: Interactive wizard with field-level operations (add, remove, rename, update attributes, change algorithm/datatype)redisvl/cli/migrate.py: Addswizardsubcommandtests/unit/test_migration_wizard.py: 56 unit tests for wizard logicUsage
Details
The wizard provides an interactive menu-driven interface for:
Note
Medium Risk
Adds a new interactive CLI flow and substantial new wizard logic that generates schema patches and migration plans, increasing surface area for edge-case and UX/input-handling issues. Core execution paths are unchanged, but incorrect patch generation could lead to unexpected migration plans.
Overview
Adds a new
rvl migrate wizardsubcommand that interactively guides users through creating aSchemaPatchand writes out a generated migration plan (and optional patch/merged target schema) for review.Introduces
MigrationWizardwith a menu-driven workflow to stage schema changes (add/update/remove/rename fields, rename index, change key prefix) including vector config tuning (algorithm/datatype/metric/params) and guards like single-prefix-only support and conflict handling for staged edits.Adds extensive unit coverage for wizard input handling, including vector-configuration scenarios and adversarial/invalid input cases.
Written by Cursor Bugbot for commit dfa069a. This will update automatically on new commits. Configure here.