Skip to content

Commit 771a6de

Browse files
committed
refactor: Address code review feedback for field modifier ordering
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.
1 parent 6342f2b commit 771a6de

File tree

2 files changed

+10
-2
lines changed

2 files changed

+10
-2
lines changed

redisvl/schema/fields.py

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,9 @@ def _normalize_field_modifiers(
114114
canonical_order: List of modifiers in desired canonical order
115115
want_unf: Whether UNF should be added after SORTABLE (default: False)
116116
117-
Time Complexity: O(n + m) where n = len(field.args_suffix), m = len(canonical_order)
117+
Time Complexity: O(n + m), where n = len(field.args_suffix), m = len(canonical_order).
118+
- O(n) to create the set from field.args_suffix
119+
- O(m) to iterate over canonical_order and perform set lookups (O(1) average case per lookup)
118120
Space Complexity: O(n)
119121
120122
Example:

tests/integration/test_field_modifier_ordering_integration.py

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -297,7 +297,13 @@ def test_mixed_field_types_with_modifiers(self, client, redis_url, worker_id):
297297
assert info is not None
298298

299299
# Verify all fields were created
300-
attrs_list = info[7]
300+
# Find the 'attributes' key in the FT.INFO response (flat list format)
301+
attrs_list = None
302+
for i in range(0, len(info) - 1, 2):
303+
if info[i] == b"attributes" or info[i] == "attributes":
304+
attrs_list = info[i + 1]
305+
break
306+
assert attrs_list is not None, "'attributes' key not found in FT.INFO response"
301307
assert len(attrs_list) == 4
302308

303309
# Cleanup

0 commit comments

Comments
 (0)