fix: (schema) ensure field modifiers follow canonical order for RediSearch parser #434
Conversation
…earch parser Fix field modifier ordering to satisfy RediSearch parser requirements where INDEXEMPTY and INDEXMISSING must appear BEFORE SORTABLE in field definitions. This resolves index creation failures when using index_missing=True with sortable=True.
nkanu17
requested review from
abrookins,
bsbodden,
Copilot,
justin-cechmanek and
rbs333
and removed request for
Copilot
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a critical RediSearch parser limitation where INDEXEMPTY and INDEXMISSING modifiers must appear before SORTABLE in field definitions. The fix introduces a helper function _normalize_field_modifiers() that reorders field modifiers to match the required canonical order, preventing index creation failures when using index_missing=True with sortable=True.
Key Changes:
- Added
_normalize_field_modifiers()helper function with set-based optimization for reordering field modifiers - Refactored
TextField,TagField,NumericField, andGeoFieldto use the helper function - Added comprehensive unit and integration tests covering various modifier combinations
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
redisvl/schema/fields.py |
Core implementation: Added _normalize_field_modifiers() helper and updated field classes to normalize modifier ordering; includes minor PEP 8 spacing improvement |
tests/unit/test_field_modifier_ordering.py |
Comprehensive unit tests for the helper function and all field types with various modifier combinations |
tests/integration/test_field_modifier_ordering_integration.py |
Integration tests against live Redis to verify index creation succeeds with reordered modifiers |
💡 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 3 out of 3 changed files in this pull request and generated no new 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 10 out of 10 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
redisvl/index/index.py
Outdated
| BaseVectorQuery, | ||
| CountQuery, | ||
| FilterQuery, | ||
| TextQuery, |
There was a problem hiding this comment.
[nitpick] The TextQuery import is added but should be grouped with other query imports for better organization. Consider placing it alphabetically with BaseVectorQuery, CountQuery, and FilterQuery on the same line or ensuring consistent multi-line formatting.
nkanu17
force-pushed
the
RAAE-1199-redisvl-ft-create-fails-when-using-indexmissing-sortable-and-unf-togethertable-true-and-unf-true-fails-at-index-creation
branch
from
bce7e6f to
94e2ff2
Compare
rbs333
left a comment
rbs333
left a comment
There was a problem hiding this comment.
Thanks for adding test spent the most time looking through those.
Looks good ✅
Two improvements to field modifier ordering integration tests: 1. Remove try/except cleanup pattern - Replaced try/finally/except blocks with direct cleanup calls - Affects 9 test methods across all field types - Cleanup failures now visible instead of silently ignored - Matches pattern used in other integration tests 2. Rename for open-source clarity - TestMultipleCommandsScenarioIntegration → TestFieldModifierIntegration - test_mlp_commands_index_creation → test_index_creation_with_multiple_modifiers - Use general naming instead of MLP-specific terminology These changes improve test maintainability and make failures more visible.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address valid GitHub Copilot feedback: 1. Replace magic number index access in FT.INFO parsing - Changed info[7] to proper key-based lookup for 'attributes' - More resilient to Redis response format changes - Added helpful error message if key not found 2. Enhance time complexity documentation - Added detailed breakdown of O(n + m) complexity - Clarified set creation and lookup costs Rejected feedback about testing implementation details - unit tests appropriately test the _normalize_field_modifiers helper function directly, while integration tests verify end-to-end behavior.
Fix field modifier ordering to satisfy RediSearch parser requirements where
INDEXEMPTY and INDEXMISSING must appear BEFORE SORTABLE in field definitions.
This resolves index creation failures when using index_missing=True with
sortable=True.
Changes:
Fixes: issue #431
Related: Field 'INDEXMISSING' does not have a type error
BREAKING CHANGE: None - backward compatible, only changes internal ordering