Repository navigation
fix: keep vector fields in from_existing on RESP2 unified FT.INFO replies #742
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
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 |
|---|---|---|
|
|
@@ -318,6 +318,34 @@ def normalize_index_definition(index_info: dict[str, Any]) -> dict[str, Any]: | |
| return _normalize_index_info_mapping(index_info.get("index_definition")) | ||
|
|
||
|
|
||
| # Vector parameter names that parse_vector_attrs reads from FT.INFO. In a | ||
| # RESP2 unified ``flags`` list, only these are paired with the next token, so | ||
| # an unknown bare token costs one position instead of shifting every pair. | ||
| _VECTOR_PARAM_KEYS = frozenset( | ||
| { | ||
| "algorithm", | ||
| "data_type", | ||
| "datatype", | ||
| "type", | ||
| "dim", | ||
| "dims", | ||
| "distance_metric", | ||
| "initial_cap", | ||
| "block_size", | ||
| "m", | ||
| "ef_construction", | ||
| "ef_runtime", | ||
| "epsilon", | ||
| "compression", | ||
| "reduce", | ||
| "graph_max_degree", | ||
| "construction_window_size", | ||
| "search_window_size", | ||
| "training_threshold", | ||
| } | ||
| ) | ||
|
|
||
|
|
||
| def normalize_index_fields(index_info: dict[str, Any]) -> list[dict[str, Any]]: | ||
| """Return FT.INFO field entries in mapping form.""" | ||
| return [ | ||
|
|
@@ -373,6 +401,18 @@ def parse_vector_attrs(attrs): | |
| for key, value in attrs.items() | ||
| if key not in {"identifier", "attribute", "type", "flags"} | ||
| } | ||
| # redis-py 8 on RESP2 with legacy_responses=False puts the vector | ||
| # params in "flags" as key/value pairs, possibly followed by | ||
| # valueless modifiers: ["algorithm", "FLAT", "dim", 4, "INDEXMISSING"] | ||
| flags = attrs.get("flags") or [] | ||
| i = 0 | ||
| while i < len(flags): | ||
| key = str(flags[i]).lower() | ||
| if key not in _VECTOR_PARAM_KEYS or i + 1 >= len(flags): | ||
| i += 1 | ||
| continue | ||
| vector_attrs.setdefault(key, flags[i + 1]) | ||
| i += 2 | ||
| else: | ||
| # Check if we have any attributes beyond the type declaration | ||
| if len(attrs) <= 6: | ||
|
|
@@ -408,10 +448,18 @@ def parse_vector_attrs(attrs): | |
| normalized = {} | ||
|
|
||
| # Handle dims/dim field - REQUIRED for vector fields | ||
| if "dim" in vector_attrs: | ||
| normalized["dims"] = int(vector_attrs.pop("dim")) | ||
| elif "dims" in vector_attrs: | ||
| normalized["dims"] = int(vector_attrs["dims"]) | ||
| if "dim" in vector_attrs or "dims" in vector_attrs: | ||
| raw_dims = ( | ||
| vector_attrs.pop("dim") | ||
| if "dim" in vector_attrs | ||
| else vector_attrs["dims"] | ||
| ) | ||
| try: | ||
| normalized["dims"] = int(raw_dims) | ||
| except (ValueError, TypeError): | ||
| # Unparseable dims: fall through to the required-dims check | ||
| # below, which skips the field with a warning. | ||
| pass | ||
| else: | ||
| # If dims is missing from normal parsing, try scanning the raw attrs | ||
| # This handles edge cases where the format is unexpected | ||
|
|
@@ -604,6 +652,14 @@ def parse_attrs(attrs, field_type=None): | |
| if attrs is None: | ||
| # Vector field attributes cannot be parsed on this Redis version | ||
| # Skip this field - it cannot be properly reconstructed | ||
| logger.warning( | ||
| "Skipping vector field %r of index %r: could not parse its " | ||
| "attributes from FT.INFO. The recovered schema is incomplete, " | ||
| "and recreating the index from it (e.g. create(overwrite=True)) " | ||
| "drops this field.", | ||
| name, | ||
| index_name, | ||
| ) | ||
|
Comment on lines
+655
to
+662
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. Exactly what the issue asked for, but it won't reach the user it's for — and that isn't your fault.
So the skip is still silent for the issue's own reproduction, and the Optional: the message could name the consequence — the schema is incomplete, and rebuilding from it with
Contributor
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. Thanks for tracking the |
||
| continue | ||
| field["attrs"] = attrs | ||
| else: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -241,3 +241,123 @@ def test_convert_index_info_default_stopwords(): | |
|
|
||
| assert result["index"]["name"] == "test_default_stopwords" | ||
| assert "stopwords" not in result["index"] # Should not be present | ||
|
|
||
|
|
||
| def test_convert_index_info_resp2_unified_vector_flags(): | ||
|
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. Good tests, and the first vector coverage this file has had. But they pin today's redis-py wire shape as a literal, so if it changes again they'll pass while the library breaks. There's already an integration test for exactly this configuration that can't fire: Could you add one param to each? pytest.param(
{"protocol": 2, "legacy_responses": False},
id="resp2-new-response-format",
marks=pytest.mark.skipif(
int(redis.__version__.split(".")[0]) < 8,
reason="legacy_responses requires redis-py 8",
),
),On
Contributor
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. Done in d4d7a2e. I added the |
||
| """Vector params packed into ``flags`` (redis-py 8, RESP2, legacy_responses=False).""" | ||
| index_info = { | ||
| "index_name": "test_index", | ||
| "index_definition": {"key_type": "HASH", "prefixes": ["doc"]}, | ||
| "attributes": [ | ||
| { | ||
| "identifier": "emb", | ||
| "attribute": "emb", | ||
| "type": "VECTOR", | ||
| "flags": [ | ||
| "algorithm", | ||
| "HNSW", | ||
| "data_type", | ||
| "FLOAT32", | ||
| "dim", | ||
| 8, | ||
| "distance_metric", | ||
| "L2", | ||
| "M", | ||
| 24, | ||
| "ef_construction", | ||
| 300, | ||
| "INDEXMISSING", | ||
|
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. This expected dict has no It can't be fixed here — So leave the behaviour — but a one-line comment noting the discard is deliberate would stop the next reader assuming it was missed. Separate issue coming.
Contributor
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. Added a comment above the expected dict in d4d7a2e. It says |
||
| ], | ||
| } | ||
| ], | ||
| } | ||
|
|
||
| result = convert_index_info_to_schema(index_info) | ||
|
|
||
| # INDEXMISSING is deliberately not recovered as ``index_missing``: a vector | ||
| # field can't be recreated with it yet, so restoring it here would turn a | ||
| # quiet loss into a failure on ``create()``. | ||
| assert result["fields"] == [ | ||
| { | ||
| "name": "emb", | ||
| "type": "vector", | ||
| "attrs": { | ||
| "dims": 8, | ||
| "distance_metric": "l2", | ||
| "algorithm": "hnsw", | ||
| "datatype": "float32", | ||
| "m": 24, | ||
| "ef_construction": 300, | ||
| }, | ||
| } | ||
| ] | ||
|
|
||
|
|
||
| def test_convert_index_info_warns_on_unparseable_vector_field(caplog): | ||
| """A vector field without params is skipped with a warning, not silently.""" | ||
| index_info = { | ||
| "index_name": "test_index", | ||
| "index_definition": {"key_type": "HASH", "prefixes": ["doc"]}, | ||
| "attributes": [ | ||
| {"identifier": "emb", "attribute": "emb", "type": "VECTOR", "flags": []} | ||
| ], | ||
| } | ||
|
|
||
| with caplog.at_level("WARNING"): | ||
| result = convert_index_info_to_schema(index_info) | ||
|
|
||
| assert result["fields"] == [] | ||
| assert "'emb'" in caplog.text and "'test_index'" in caplog.text | ||
|
|
||
|
|
||
| def test_convert_index_info_resp2_unified_flags_skip_unknown_tokens(): | ||
| """An unknown bare token in ``flags`` does not shift the key/value pairs.""" | ||
| index_info = { | ||
| "index_name": "test_index", | ||
| "index_definition": {"key_type": "HASH", "prefixes": ["doc"]}, | ||
| "attributes": [ | ||
| { | ||
| "identifier": "emb", | ||
| "attribute": "emb", | ||
| "type": "VECTOR", | ||
| "flags": [ | ||
| "algorithm", | ||
| "FLAT", | ||
| "data_type", | ||
| "FLOAT32", | ||
| "dim", | ||
| 4, | ||
| "NEWMOD", | ||
| "distance_metric", | ||
| "IP", | ||
| ], | ||
| } | ||
| ], | ||
| } | ||
|
|
||
| result = convert_index_info_to_schema(index_info) | ||
|
|
||
| assert result["fields"][0]["attrs"]["distance_metric"] == "ip" | ||
| assert result["fields"][0]["attrs"]["dims"] == 4 | ||
|
|
||
|
|
||
| def test_convert_index_info_skips_vector_field_with_unparseable_dims(caplog): | ||
| """A non-numeric ``dim`` skips the field with a warning instead of raising.""" | ||
| index_info = { | ||
| "index_name": "test_index", | ||
| "index_definition": {"key_type": "HASH", "prefixes": ["doc"]}, | ||
| "attributes": [ | ||
| { | ||
| "identifier": "emb", | ||
| "attribute": "emb", | ||
| "type": "VECTOR", | ||
| "flags": ["algorithm", "FLAT", "dim", "FUTUREMOD"], | ||
| } | ||
| ], | ||
| } | ||
|
|
||
| with caplog.at_level("WARNING"): | ||
| result = convert_index_info_to_schema(index_info) | ||
|
|
||
| assert result["fields"] == [] | ||
| assert "'emb'" in caplog.text | ||
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 loop is where
dimstops coming from a trusted top-level key and starts coming fromflags, which makes line 375 reachable — the one numeric conversion here that isn't wrapped, unlikem(418),ef_construction(424) and the SVS params through 476:Worth wrapping so a gap in
dimtakes thereturn None-> warn-and-skip path instead of escapingfrom_existing. The allowlist above doesn't cover this —dimis a known key, so it still consumes whatever follows.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.
Wrapped in d4d7a2e.
dim/dimsnow go through the sametry/except (ValueError, TypeError)as the other numeric params. An unparseable value leavesdimsunset, so the field takes the existingreturn None→ warn-and-skip path. A new unit test covers["algorithm","FLAT","dim","FUTUREMOD"]: the field is skipped with the warning, and nothing escapesconvert_index_info_to_schema.